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

Pub get "pre-compiles" packages without entry points in bin/ #21777

Closed
mkustermann opened this issue Dec 3, 2014 · 15 comments
Closed

Pub get "pre-compiles" packages without entry points in bin/ #21777

mkustermann opened this issue Dec 3, 2014 · 15 comments
Labels
closed-as-intended Closed as the reported issue is expected behavior

Comments

@mkustermann
Copy link
Member

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)

  • collection 1.1.0
  • crypto 0.9.0
  • googleapis_auth 0.2.2
  • http 0.11.1+1
  • http_parser 0.0.2+5
  • path 1.3.0
  • source_span 1.0.2
  • stack_trace 1.1.1
  • string_scanner 0.1.2
    Changed 9 dependencies!
    Precompiling dependencies...
    Loading source assets...
    Precompiled googleapis_auth.

It "Precompiled googleapis_auth" here, even though googleapis_auth

  • does not have any entrypoints in bin/
  • does not have any transformers which would affect other packages depending on it (i.e. files inside lib/ stay the same)

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:

  • $dart2js:
        $exclude:
        - test/oauth2_flows/auth_code_test
@mkustermann
Copy link
Member Author

cc @sgjesse.

@DartBot
Copy link

DartBot commented Dec 3, 2014

This comment was originally written 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 di transformer section for the examples it contains. pub get fails for myapp because the di transformer can't find the SDK directory (a stupid issue with the DI transformer, it requires to add a sdkDirectory: pathToSDK setting).
How can I set the SDK path (hardcoded) in the pubspec.yaml of a dependency so it works on every system I want to use myapp on - it's not possible.

@DartBot
Copy link

DartBot commented Dec 3, 2014

This comment was originally written by @zoechi


Might also be related to http://dartbug.com/21765

@munificent
Copy link
Member

It "Precompiled googleapis_auth" here, even though googleapis_auth

  • does not have any entrypoints in bin/
  • does not have any transformers which would affect other packages depending on it (i.e. files inside lib/ stay the same)

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.

That does sound like a bug to me.

@nex3
Copy link
Member

nex3 commented Dec 3, 2014

More specifically it looks like it pre-compiles packages which have a transformers section in their pubspec.yaml.

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.

It disables the built-in "$dart2js" transformer for certain files where it does not work.

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.

Yes, this built-in transformer is unfortunately an explicit opt-out instead of an opt-in solution -- as mentioned many times.

An opt-in solution would suck for users doing web development, who are the vast majority of users of "pub serve" and "pub build".

gzoechi wrote:
pub get fails for myapp because the di transformer can't find the SDK directory (a stupid issue with the DI transformer, it requires to add a sdkDirectory: pathToSDK setting).

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.

@DartBot
Copy link

DartBot commented Dec 3, 2014

This comment was originally written by @zoechi


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.

Just precompile fails but I have an error output when I build the Docker image and I want to prevent that.

This sounds like a bug in the package that uses di to me.

The bug is definitively in the DI transformer.
See dart-archive/angular.dart#1270 (comment) for more details.

I can't see how it makes sense that pub get does this pre-compile when only some dependency has a transformer configuration in its pubspec.yaml.
If the pubspec.yaml of my app had a transformer setting I could understand it.

@nex3
Copy link
Member

nex3 commented Dec 3, 2014

I can't see how it makes sense that pub get does this pre-compile when only some dependency has a transformer configuration in its pubspec.yaml.

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".

@DartBot
Copy link

DartBot commented Dec 4, 2014

This comment was originally written by @zoechi


I can't see how pre-compiled transformer code can be of any use when there is no transformer configuration in my pubspec.yaml.
Maybe I missed something but as far I am aware, transformers specified only in a dependency are not run, or are they?

@mkustermann
Copy link
Member Author

It disables the built-in "$dart2js" transformer for certain files where it does not work.

There's no reason at all to do this. ... Even if you had executables in bin/, it wouldn't be run on them either.

Proof by counter example:

$ cat pubspec.yaml
name: foo
version: 0.1.0

$ cat bin/foo.dart
import 'dart:io';
main() => stdout.writeln('hello world');

$ pub build --all
Loading source assets...
Building foo...
[Info from Dart2JS]:
Compiling foo|bin/foo.dart...
[Info from Dart2JS]:
Took 0:00:02.121472 to compile foo|bin/foo.dart.
Built 1 file to "build".

As you can see it runs the $dart2js transformer on bin/foo.dart.

More specifically it looks like it pre-compiles packages which have a transformers section in their pubspec.yaml.
This is intentional. ...

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/)
b) To prevent a), one needs to opt-out of this behavior explicitly via a transformers section in pubspec.yaml (as e.g. package:googleapis_auth does)
c) If an application depends on a package which has issue a) [like package:googleapis_auth] and solves it via solution b) it will cause "pub get" to "Precompile" the googleapis_auth package.

=> There is no usage of a real transformer in any of the packages involved (only a opt-out of the $dart2js transformer)
=> The only transformer section (opt-out of $dart2js) applies to files inside a bin/ or test/ directory.
=> If a package depends on another package which only applies transformers to non lib/ files (e.g. only to bin/ or test/), there is no reason at all for precompilation.

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
y) only opt-out of the built-in $dart2js compiler
z) don't do transformations on files inside the lib/ directory

In the anticipation that at least y) gets fixed (and maybe z) as well), I'll re-open this bug.


Added op label.

@mkustermann
Copy link
Member Author

Added Triaged label.

@nex3
Copy link
Member

nex3 commented Dec 4, 2014

Maybe I missed something but as far I am aware, transformers specified only in a dependency are not run, or are they?

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.

kustermann wrote:
Proof by counter example:

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.

When executing "pub get" it should not "Precompile" packages which
y) only opt-out of the built-in $dart2js compiler
z) don't do transformations on files inside the lib/ directory

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.

@DartBot
Copy link

DartBot commented Dec 5, 2014

This comment was originally written by @zoechi


I see. I found http://dartbug.com/17306 which would solve my problem.

@mkustermann
Copy link
Member Author

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.

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.

When executing "pub get" it should not "Precompile" packages which
y) only opt-out of the built-in $dart2js compiler
z) don't do transformations on files inside the lib/ directory

As I mentioned before, there's a real cost to proving that no transformers affect files in the lib/ directory ...

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)!

@nex3
Copy link
Member

nex3 commented Dec 8, 2014

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).

That's true. We've talked about dev_transformers in the past (issue #14187) and it ended up getting merged into issue #20267 which is now about something completely different. I'm still not sure they provide enough value to be worthwhile, though, especially with $include/$exclude.

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)!

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.

@DartBot
Copy link

DartBot commented Jun 5, 2015

This issue has been moved to dart-lang/pub#1200.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-as-intended Closed as the reported issue is expected behavior
Projects
None yet
Development

No branches or pull requests

5 participants