Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[new feature] build_dependencies #3794

Open
dcharkes opened this issue Feb 22, 2023 · 13 comments
Open

[new feature] build_dependencies #3794

dcharkes opened this issue Feb 22, 2023 · 13 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Feb 22, 2023

Introduce build_dependencies: in pubspec.yaml, and let dart files in build/native.dart only depend on these dependencies.

Consider the following scenario:

  • package:a depends on package:b, but the build/native.dart in package:a does not depend on package:b.
  • package:c and package:d circularly depending on each other, but both of their build/native.dart builds do not depend on the other package.
  • All four packages depend on package:clang and use it in their build/native.dart.

Specifying build_dependencies has the following advantages:

  • Native builds can be run more in parallel, all packages (a, b,c, and d) can be built in parallel.
  • Native builds are less often invalidated. Updating package:a to a newer version does not trigger a rebuild of package:bs’ native assets.
  • Native builds can work for circular dependencies. Packages c and d have no valid build order if we don’t know that both of their native builds do not depend on each other's native dependencies.
  • Native builds can depend on different versions of packages for their native builds. For example package:a could use a different major version of package:clang than package:b. This would not be possible with their dependencies being resolved in one go. Dependency resolution would fail.

This means that the .dart_tool/package_config.json would contain a resolution set per package for its build_dependencies.
This means we probably need another dart <...> command, because dart run resolves to the bin/ folder, not the build/ folder.
This would require the IDE to understand having different versions of dependencies in a project in the build/.

It is unclear whether any of the benefits outweigh the downsides, so we leave this for when this becomes relevant in the ecosystem.

Workaround: split your package into two packages, and have the native assets in a small package that the rest of the code in the other package depends on.

Context:

Taken from go/dart-native-asset-caching to have a tracking bug.

@dcharkes
Copy link
Contributor Author

This design would be similar to Rust/Cargo's build.rs, it also has a separate list of build dependencies: https://doc.rust-lang.org/cargo/reference/build-scripts.html#build-dependencies

@sigurdm sigurdm added the type-enhancement A request for a change that isn't a bug label Feb 23, 2023
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue May 15, 2023
This package contains the logic for building native assets.

This package is the backend that invokes toplevel `build.dart` scripts.
For more info on these scripts see https://github.com/dart-lang/native.

This is a separate package so that dartdev and flutter_tools can reuse
the same logic without flutter_tools having to import dartdev.

Some design decisions:

* We don't yet have `build_dependencies`, so we use the ordinary
  dependency graph for ordering of native assets builds. (If there is
  a cycle we refuse to run.)
  Bug: dart-lang/pub#3794
* Builds are cached based on all the configuration provided by the
  caller. Environment variables are ignored in caching. This CL also
  contains a unit test that invokes the build by not passing through
  environment variables. However, for Windows we need to pass through
  at least `SYSTEMROOT` for MSVC to run correctly. So we might need
  to further explore if we can/want to lock env variables down.
  Bug: dart-lang/native#32
  Bug: dart-lang/native#33

Run tests:
```
dart tools/generate_package_config.dart && \
tools/test.py -n unittest-asserts-release-linux pkg/native_assets_builder
```

Bug: #50565
Change-Id: I133052d7195373e87d20924d61e1e96e3d34ce8f
Cq-Include-Trybots: luci.dart.try:pkg-linux-debug-try,pkg-linux-release-try,pkg-mac-release-arm64-try,pkg-mac-release-try,pkg-win-release-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/300203
Reviewed-by: Liam Appelbe <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Martin Kustermann <[email protected]>
Reviewed-by: Hossein Yousefi <[email protected]>
@devoncarew
Copy link
Member

devoncarew commented Dec 5, 2023

Is this issue mostly about decoupling the build dependencies (the package deps in dependencies: contributed from the build script) from the regular packages deps (those consumed by code in lib/)?

If so, an option to support this that does not require a new xx_dependencies: section in the pubspec is to put build.dart scripts in a new, well-known sub-folder, and optionally, for that sub-folder to have its own pubspec (similar to the example/ directory).

Call this directory build/ for now (irrespective of flutter's use of that directory name). So, the build script would go in build/build.dart and any link script would go in build/link.dart. If they needed improved dependency hygiene - separate deps from what they would get from the parent package - that directory could have its own, separate pubspec file. That file would just use regular dependencies: deps.

Perhaps assemble/ would be a good name for the dir? script/? packaging/? build_scripts/? dev/? Dart Asset Collation and Organization - daco/?

@dcharkes
Copy link
Contributor Author

Dart Asset Collation and Organization - daco/?

Haha, I completely missed this one earlier. 😆 How'd you come up with this! 🤣

Considering the developments in dart-lang/sdk#54334, this should be hook/.

Call this directory build/ for now (irrespective of flutter's use of that directory name). So, the build script would go in build/build.dart and any link script would go in build/link.dart. If they needed improved dependency hygiene - separate deps from what they would get from the parent package - that directory could have its own, separate pubspec file. That file would just use regular dependencies: deps.

I like it, but we'd need to do a pub get in a subdir in the pub cache though, before running some of your root-package dependency's hook/build.dart. I don't think @jonasfj would approve modifying the pub cache.

@jonasfj
Copy link
Member

jonasfj commented Jan 25, 2024

I don't think @jonasfj would approve modifying the pub cache.

Yeah, modifying $PUB_CACHE is a big no no.

But also you really want your build-dependencies covered by pubspec.lock.
Otherwise, you risk that your build dependencies change between dev machines, CI env, QA builds and release builds. As a result application developers might accidentally push untested code into production -- this what pubspec.lock aims to prevent.


I think there are other reasons for not wanting entirely separate resolutions. If some of your dependencies are in dependencies and other dependencies are in build_dependencies (which makes sense) -- after all that's how dev_dependencies work. Then you might be surprised if at build-time packages from your dependencies section have a different version than at runtime.

IMO, a safer bet here would be private_dependencies (my proposal is not ready yet, and there is no SDK support for rewriting import-paths / renaming packages).

@jonasfj
Copy link
Member

jonasfj commented Jan 25, 2024

In particular, I can imagine that given how packages can pass meta-data and build assets between each other, it could cause problems if foo depends on bar, and both have build-dependencies, but bar uses different package versions at build-time than foo does.

Because foo can consume the binary assets from bar. And those binary assets might be affected by transitive dependencies of bar, or there may be Dart libraries in bar used at build-time by foo to read the binary assets from bar. This is very reasonable.

It's very reasonable for foo to assume that the transitive dependencies under bar at build-time for foo and bar are the same (when foo has a build-time dependency on bar).

@dcharkes
Copy link
Contributor Author

dcharkes commented Jan 25, 2024

Right we talked about a similar thing yesterday. I tried to remember it this morning but couldn't come up with it.

So there are multiple places where version skew between different packages build.dart/link.dart should not happen.

  • The metadata flowing from one build.dart to another if one package uses a helper package to unparse an object into json and the other package parses that metadata again. Version skew between the two build.dart scripts depending on that metadata parsing fails.
  • The data files flowing from on packages build.dart to another packages link.dart if they are for example json serialization of a format comes from a helper package. These two package should not have a different package version for this helper package. @mosuem

@dcharkes
Copy link
Contributor Author

private_dependencies

@jonasfj Private dependencies is not entirely safe either. Because you could not leak your dependency types to the public API but leak the json-serialized format to metadata or a json encoded asset. So if we introduce private_dependencies, we need to document clearly that you cannot add packages to private_dependencies when you rely on serialized formats (assuming that the serialized formats are versioned by semver).

@jonasfj
Copy link
Member

jonasfj commented Jan 25, 2024

So if we introduce private_dependencies, we need to document clearly that you cannot add packages to private_dependencies when you rely on serialized formats

You can rely on private_dependencies, you must not re-export anything from a private-dependency.

But yes, there be dragons here 🐉 -- including assetIds that might be hard to figure out.


I think it's important to recognize that multiple versions of the same package is difficult to do. Even if things run in a different context, data or JSON meta-data might flow between them.

It kind of only works, if you promise that packages (and transitive dependencies) of every in build_dependencies won't affect your public interface -- the public interface being API, but also assets, meta-data for said assets, etc.

That can work, if we make people explicitly promise this. It's complicated. Not perfect. And it doesn't not alleviate us from having to lock everything in the pubspec.lock. But could be made to work one day.

I think all of this is something we can explore if/when version conflicts become a huge issue.

@mosuem
Copy link
Member

mosuem commented Jun 21, 2024

Reviving this discussion.

My worry is that by not having build_dependencies, we pin the version of any package used link/build hooks for all users of the package. For example, we might want to store the treeshaking information as protobuf, which would mean that we would pin protobuf (at least the major version) for all users of link or build hooks, which might be many users if looking at the transitive closure of package deps.

Is there any plan to move forward with some kind of different dependencies for hooks?

@jonasfj
Copy link
Member

jonasfj commented Jun 21, 2024

@mosuem Why would you pin dependencies?

@sigurdm
Copy link
Contributor

sigurdm commented Jun 21, 2024

@mosuem Why would you pin dependencies?

In my understanding the point is that a dependency used for building will constrain the same dependency for run-time while they really are running in separate contexts, and therefore could use separate versions.

@jonasfj
Copy link
Member

jonasfj commented Jun 21, 2024

Okay, I understood it as pinning a dependency to the same version.

In the case of protobuf (or similar), I would suspect that there easily is a runtime component to it (some library used by the generated code). And said runtime component would need to be compatible.

@mosuem
Copy link
Member

mosuem commented Jun 21, 2024

In my understanding the point is that a dependency used for building will constrain the same dependency for run-time while they really are running in separate contexts, and therefore could use separate versions.

Exactly :) Thanks for the clarification.

In the case of protobuf (or similar), I would suspect that there easily is a runtime component to it (some library used by the generated code). And said runtime component would need to be compatible.

In the case of storing tree-shaking information, this is only used in the link hook, so no runtime components.

I would even argue that hooks can't have runtime components, as they consume a configuration and produce assets, so there can be no code leakage. So the dependencies are actually separate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

5 participants