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

[native_assets_builder] build.dart dependencies cannot be dev dependencies. #160

Closed
dcharkes opened this issue Oct 25, 2023 · 8 comments
Closed

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented Oct 25, 2023

TODO for closing this issue: Add a check somewhere, or add documentation.

Consider package:my_app depends on package:foo, and package:foo has a build.dart using package:http to download a dylib from a CIDN.

A. If package:foo would only add package:http as a dev dependency, then

  1. (current behavior) the native assets builder will invoke the build.dart with the root package (package:my_app) package_config.json, and the build.dart invocation will fail, or
  2. (possible alternative behavior) the native assets builder will run pub get inside the pub cache containing package:foo and use the resulting package config to run build.dart.

B. If package:foo adds package:http as a normal dependency, then

  1. (current behavior) the native assets builder will invoke the build.dart with the root package (package:my_app) package_config.json, and the build.dart invocation will succeed.

So with the current setup, build.dart dependencies must be added as normal dependencies, not dev dependencies.

@jonasfj would probably not enjoy us running pub get inside the pub cache.

Switching to the alternative behavior A.2. enables various build.dart scripts to rely on incompatible versions of package:http.

However, build.dart does not need all the other dev dependencies, nor possibly the main dependencies. So this sounds like another hint in the direction of introducing build_dependencies.

Thanks for reporting @craiglabenz!

@dcharkes
Copy link
Collaborator Author

@jonasfj Do we have logic for checking somewhere that if you use a dev-dependency in lib/ instead of tool/ or test/ you get an error?

@dcharkes dcharkes added this to the Native Assets v1.0 milestone Aug 30, 2024
@jonasfj
Copy link
Member

jonasfj commented Sep 2, 2024

Yes, dart pub publish --dry-run should run some validators that check if dev_dependencies are used in lib/, etc.

See:
https://github.com/dart-lang/pub/blob/master/lib/src/validator/strict_dependencies.dart

@jonasfj
Copy link
Member

jonasfj commented Sep 2, 2024

@dcharkes it's possible that we do what to check this for hook/ too :D

@dcharkes
Copy link
Collaborator Author

dcharkes commented Sep 2, 2024

@dcharkes it's possible that we do what to check this for hook/ too :D

I was going to suggest this!

@jonasfj
Copy link
Member

jonasfj commented Sep 2, 2024

Filed dart-lang/pub#4365

@jonasfj
Copy link
Member

jonasfj commented Sep 2, 2024

lol 🤣

@HosseinYousefi
Copy link
Member

2a4e996ab3fb9ab2196211537a14e4a4

@sigurdm
Copy link

sigurdm commented Sep 3, 2024

I ...think we have something corresponding in the analyzer as well...

Specifically https://dart.dev/tools/linter-rules/depend_on_referenced_packages most likely need adjusting too.

I think the definition of isInPublicDir that is defined here:

https://github.com/dart-lang/sdk/blob/4babb9deb1c74e3fa23587a0cd2f9aa7c607fffa/pkg/linter/lib/src/ast.dart#L163

and referenced here:

https://github.com/dart-lang/sdk/blob/1f4f0d386a9095fc788f53b0a98d0069bcabc800/pkg/linter/lib/src/rules/pub/depend_on_referenced_packages.dart#L78

needs an update to include files in the hooks folder.

@dcharkes dcharkes self-assigned this Sep 3, 2024
copybara-service bot pushed a commit to dart-lang/sdk that referenced this issue Sep 3, 2024
`hook/` is a reserved directory.
#54334

And the two hooks currently in existence require to be invoked with
the same dependencies as the root package. Hence the dependencies of
the hooks should be normal dependencies and not dev dependencies.

Bug: dart-lang/native#160
Change-Id: I7cf48659bd240c2e46b5bdede2a336763301b8c5
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/383301
Reviewed-by: Samuel Rawlins <[email protected]>
Commit-Queue: Daco Harkes <[email protected]>
Reviewed-by: Hossein Yousefi <[email protected]>
@dcharkes dcharkes closed this as completed Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants