Open mit-mit opened 1 year ago
cc @bkonyi @devoncarew @jonasfj
Is there some way we can make pub get
be faster? Say a --quick-check
flag which makes it no do anything if the .dart_tool/package_config.json
exists and was created later than the latest modification of pubspec.yaml
(or however it can quickly get a hint that it shouldn't do anything).
Running dart pub get
in a very minimal project, which just depends on test
, takes ~three second for me.
Doing that every time I try to do dart run
would be very annoying.
(I assume it wouldn't print the output of dart pub get
, because then it would be extra annoying.)
Yes, for run
maybe we just check that:
pubspec.lock
and .dart_tool/package_config.json
exist on diskpubspec.yaml
generatorVersion
tag in matches the version of the current SDK?
We have an assertUpToDate
which has some rather subtle logic for testing if package_config.json
is reflection of pubspec.lock
, and whether pubspec.lock
is a resolution to pubspec.yaml
.
Doing that every time I try to do dart run would be very annoying.
dart run
already has this behavior, when the input is dart run <package>[:<command>]
, for some reason it doesn't happen when doing dart run file.dart
.
$ dart create myfoo
$ cd myfoo
$ rm -rf .dart_tool/
$ dart run myfoo
Resolving dependencies in /tmp/myfoo...
Got dependencies in /tmp/myfoo!
Building package executable...
Built myfoo:myfoo.
Hello world: 42!
Running dart test
also has this behavior.
The downside of doing this everywhere is that: using dart for things that don't have a pubspec.yaml
will become increasingly hard.
Today, we have reports that dart test
won't work in the pkg/<package>/
folder of the Dart SDK. Are we confident we can further break these work-flows :see_no_evil:
What is the semantics if there is no pubspec.yaml
? should we look in parent directories? I don't think we do this today.
Is cd test/; dart foo_test.dart
allowed (there is no pubspec.yaml
in current directory)? Today, I don't think dart run <package>[:<command>]
works if not running next to the pubspec.yaml
.
Should we have this behavior for dart file.dart
too?
We should probably special cases commands where a --packages
flag is passed to not call get
in that case (passing the file implies that you already have one).
The flutter command always runs a pub get when a sub-command that needs a resolution is executed. The dart command doesn't.
I'd like to tweak that to say that the change in behavior we want is to:
dart get
automatically for more of the commands, (as above) anddart get
automatically after a SDK major version upgradeIt's that 2nd part that would help with https://github.com/google/file.dart/issues/204 - getting a new version of package:file after an SDK upgrade. Does that sounds right?
Here's the list of the current dart commands:
Command | Description | Needs pub get if out-of-date | Needs pub get on SDK upgrade |
---|---|---|---|
analyze | Analyze Dart code in a directory. | ? | ? |
compile | Compile Dart to various formats. | yes | yes |
compiler-server-shutdown | Shut down the Resident Frontend Compiler. | no | no |
create | Create a new Dart project. | no | no |
devtools | Open DevTools (optionally connecting to an existing application). | no | no |
doc | Generate API documentation for Dart projects. | yes | yes |
fix | Apply automated fixes to Dart source code. | yes | yes |
format | Idiomatically format Dart source code. | no | no |
migrate | Perform null safety migration on a project. | yes | yes |
pub | Work with packages. | no | no |
run | Run a Dart program. | yes | yes |
test | Run tests for a project. | yes | yes |
Some other thoughts:
When running pub get
in the above scenarios, we should:
pub get
text (so people can use it to diagnose issues)I was thinking that those are two orthogonal dimensions, but that we'd want the same behavoir in both cases. And it does look like you have the same values in all our rows.
There is a possible complication around flutter. If you use dart format
with flutter, or dart analyze
with flutter, then the dart pub get
we run won't necessarily do all of the plugin registration setup steps -- ie. it's not the same as flutter pub get
.
Here's the list of the current dart commands:
There's language_server
and debug_adapter
too (which I think should not attempt to run anything here).
Today, we have reports that dart test won't work in the pkg/
/ folder of the Dart SDK
Yes, I've seen issues with this. In VS Code we have to use dart test
(or for reasons I don't remember, dart run test:test
) to run individual tests, but running it in the SDK repo seems to mess things up (I think it produces .dart_tool/packages_config.json
inside the packages folder, and the errors are really confusing and don't make it easy to figure out what happened and how you fix it).
So in VS Code we currently detect the SDK and disable the functionality of running individual tests (which makes me sad when working on some packages 🙃). While this works, it's not ideal, and if anybody else has projects with a similar setup to the Dart SDK they won't be handled specially and may have issues. It'd be good if there was some more general way this could be detected/handled (by dart
or pub
and not the editor).
If you use dart format with flutter, or dart analyze with flutter, then the dart pub get we run won't necessarily do all of the plugin registration setup steps -- ie. it's not the same as flutter pub get.
Hmm. Maybe the root issue here is having the behavoir depend on whether you run pub get
or flutter get
rather than what is declared in the pubspec.yaml
. I think the right resolution here is to run the Flutter steps whenever resolving a pubspec.yaml
that is a Flutter pubspec, i.e. has flutter: sdk: flutter
in it.
The present issue seems even more critical in Dart 3. Consider a pubspec that doesn't support 3, e.g. sdk: '>=2.9.0 <3.0.0'
. This will not resolve in Dart 3, but if you just run a Dart command like dart analyze
or dart compile exe
, then those commands will continue to run, but now with Dart 3 as the language version. They really should run pub get
and produce an error instead.
@mit-mit are we still targeting this for Dart 3? It seems like a nice to have.
We'd like it in 3 as it's a breaking change in tools behavoir
Is there any way to disable this with maybe some flags?
Flutter has a flutter run --no-pub
I have some tests which try to use Process.run('dart', ['run', 'my_cli'])
on a temporary project created with a package_config.json already setup in a very specific manner.
The command trying to run pub get
is fundamentally the opposite of what I want in my case.
It also makes my test perform network requests when it could work fully offline.
run --no-pub
SGTM -- wdyt @sigurdm ?
Is there any way to disable this with maybe some flags? Flutter has a flutter run --no-pub
Yes, we will have a flag --no-pub
!
I have some tests which try to use Process.run('dart', ['run', 'my_cli'])
You probably want to do Process.run('dart', ['bin/my_cli.dart'])
to avoid going into dartdev entirely...
If this will be merged for Dart 3, are commands like dart language_server
and dart debug_adapter
going to be excluded? I don't think it will ever make sense to start running Pub things for these, they are to start servers that provide functionality to editors, and the directory they are run from is not necessarily indicative of the project(s) they will be used for analyzing/debugging.
If this will be merged for Dart 3, are commands like dart language_server and dart debug_adapter going to be excluded?
Yes, see: https://dart-review.googlesource.com/c/sdk/+/291500
@sigurdm ah, I see it's opt-in per-command and not run for everything. Thanks! :-)
It appears that dart file.dart
also implicitly runs pub get
.
But as opposed to other commands like dart run cmd
, there is no mean to do dart --no-pub file.dart
Is that an oversight?
Reopening, since this was reverted.
It appears that dart file.dart also implicitly runs pub get.
That sounds like a bug. Can you reproduce this?
It appears that dart file.dart also implicitly runs pub get.
That sounds like a bug. Can you reproduce this?
I switched back to stable in the meantime.
What triggered it is, I followed your suggestion of using dart bin/file.dart
instead of dart run cmd
.
But to my surprise, dart bin/file.dart
also overwrote the package_config.json, which breaks my tests (I was at a commit before these changes were reverted but after the introduction of --no-pub
)
I switched back to the stable channel, and my tests are now passing.
The tests were also passing when using dart run --no-pub cmd
on master.
Since this was reverted, I'll check if things work again on master.
Did you run with any other flags? Otherwise dartdev should be skipped, and .dart_tool/package_config.json
should stay intact...
No other flag, no.
But it's not impossible that my assumption that this was an implicit pub get
is wrong.
In any case with the current state of master
, I've just tested and dart file.dart
works fine. I guess I'd have to try again after this is reimplemented
If that would be helpful, I also don't mind trying my test suite on a specific commit ID for the Dart SDK to try and investigate.
Not sure which Flutter commit exactly had this enabled. If you know, I can easily check again
The change landed in dart sdk commit: 645511d7f427ea1456b3d9edfb7b8fbf039d6af4
I'm running it through Flutter – my test suite involves Flutter too. Do you know which Flutter commit has 645511d7f427ea1456b3d9edfb7b8fbf039d6af4?
Ah - sorry. You said the flutter revision.
I believe https://github.com/flutter/flutter/commit/7725f5965b9ceb1bb3fd37aa12eb05ccda8da1c6 should have the change.
I can't seem to reproduce the issue with dart file.dart
at that commit.
Are you sure that this is the right revision?
While playing around with
https://github.com/flutter/flutter/commit/7725f5965b9ceb1bb3fd37aa12eb05ccda8da1c6, I tried dart run --no-pub
a bit. And the flag doesn't seem to quite work.
The command doesn't complain about an unrecognized flag. But the command still seems to perform a pub check.
I tried running dart run --no-pub my_command
on a project in two situations:
1) Before running the command, I edited the pubspec to include a package which is not published on pub (so pub get
would fail).
The command then fails with:
% dart run --no-pub custom_lint
Resolving dependencies in /Users/remirousselet/dev/dart_custom_lint/packages/custom_lint/example...
Because custom_lint_example_app depends on package_not_on_pub any which doesn't exist (could not find package package_not_on_pub at https://pub.dev), version solving
failed.
Since --no-pub
is passed, I don't see why the command should "resolve dependencies" :)
2) To check if the issue with 1)
was only a validation check or if a complete pub get
was performed, I reverted the pubspec change and edited the package_config.json before running the command: I replaced the "packages" list with an empty list.
What's unexpected is, the package_config.json was overwritten as if dependencies were resolved again.
But somehow the command fails anyway:
% dart run --no-pub custom_lint
Resolving dependencies in /Users/remirousselet/dev/dart_custom_lint/packages/custom_lint/example...
Got dependencies in /Users/remirousselet/dev/dart_custom_lint/packages/custom_lint/example.
Could not find package `custom_lint` or file `custom_lint`
Maybe the package_config.json
was parsed by the command before the file was regenerated – hence the error?
Because if I run the command twice, the second attempt works fine (since the first run restores the package_config.json
somehow).
If the command recognizes --no-pub
it is the right revision. That was introduced in the same commit.
But what you are pointing out is indeed a bug. The resolution logic for dart run
is broken here. The --no-pub
only applies to dart run path/to/file.dart
not to dart run command
. Thanks for spotting this. It should probably be fixed if we decide to reland this.
@sigurdm what was the reason for reverting this, and how likely is it to re-land? I'm wondering whether it may be related to discussions at https://github.com/dart-lang/sdk/issues/52067 (pub
being run in sdk/pkg
and breaking things). If it's likely to re-land, I'll do some testing with the change included and see if I can find what might be triggering it.
I think the chances for relanding this for dart 3.0 are pretty slim.
In my understanding it was reverted mainly because how it affected the sdk workflow, but also because there was some doubts about failure modes - what happens eg. in case of no network.
(and also some discussions if it would be a ui improvement at all).
If we're talking about UX, I personally dislike Flutter doing this.
Most IDEs already offer running pub get
if out of date or on pubspec change anyway.
So the only thing it really does is cause the analyzer to restart an analysis whenever a CLI is run.
From asking around I've heard these arguments against:
1) General concern that we were too late in the 3.0 release cycle. This is probably better to do early in a cycle
2) Doesn't fail graciously when the network connection fails -- tracked in https://github.com/dart-lang/sdk/issues/52066
3) Some issues when developing in the Dart SDK itself. I believe those have been resolved, or are resolvable
4) Some suggestions for us to make sure this is aligned with the general expectation of the user coming from other languages
So the only thing it really does is cause the analyzer to restart an analysis whenever a CLI is run.
@rrousselGit can you elaborate on this? -- if you have an up-to-date resolve, the implicit pub get
is expected to be a no-op, so I'd not expect the analyzer to restart.
@mit-mit
- Some issues when developing in the Dart SDK itself. I believe those have been resolved, or are resolvable
Do you know how this was/will be resolved? Dart-Code has some detection of the SDK try and avoid running pub get
, but it would be nice if this was handled by pub
(or at least, there was an official way to opt-out of pub
) so it's harder to accidentally break things (as seems to have been happening in https://github.com/dart-lang/sdk/issues/52067, although I'm not sure of the exact trigger yet).
@mit-mit
Here's a video of me running pub get
n a project with up-to-date dependencies.
As we can see, the lints disappear for a second and reappear
Note that I'm also using analyer plugins in this workspace. That could be the cause (maybe the analyzer restarts to restart plugins. Wild guess). Also the project where I'm running pub get
doesn't have enabled plugins (but another one in a different folder does).
I can reproduce what @rrousselGit sees. In the analyzer log I can see that both pubspec.lock
and .dart_tool/package_config.json
are modified, triggering the analysis context to be re-created:
1681814088892:Watch:<unknown>:/Users/danny/Desktop/dart_sample/pubspec.lock:modify
1681814088893:Watch:<unknown>:/Users/danny/Desktop/dart_sample/.dart_tool/package_config.json:modify
I can't see any obvious changes in pubspec.lock
, but package_config.json
has a timestamp that is updated:
"generated": "2023-04-18T10:34:48.792316Z",
Also we'd had issues with this in the past with Melos.
It's a custom command line that used to edit the package_config.json for the sake of bootstrapping mono-repositories with the local code of packages instead of the remote pub.dev implementation – without having to edit the pubspec.yaml (since having to do a back-and-forth between path dependencies and version dependencies is a pain).
Commands running pub get automatically meant that they would override the package_config, reverting any change made by melos.
Melos later switched to a newer pub feature (pubspec_overrides.yaml files), so this is no longer an issue for that package. But the problem is worth mentioning.
Running dart pub get
directly is expected to try to do something.
It would be neat if dart pub get
avoided touching the pubspec.lock
and .dart_tool/package_config.json
files if it resolved to the same package versions again - but it's also valuable to be able to overwrite those files in case manual edits have clobbered them, so always writing new files is the safest approach.
Automatically running of pub get
for other commands, the feature which is being discussed here, should only happen if there is no (or not up-to-date) .dart_tool/package_config.json
file, because that file is needed for running or analyzing Dart code. Doing dart test
without a .dart_tool/package_config.json
file just won't run (or, worse, might find a different package_config.json
in a parent directory).
Doing it with an out-of-date package_config.json
file is ignoring changes made to pubspec.yaml
, which can again mean you're not testing what you think you're testing.
If your IDE has already run pub get
, there should be no need to do an extra run, and nothing should happen.
I'm not sure which criteria would be used to see if the package_config.dart
file is up-to-date (but a modified-time later than pubspec.yaml
modified time would be the simplest one.)
We have decent logic for detecting if we need to run a full resolution.
pubspec.yaml
, pubspec.lock
and .dart_tool/package_config.json
.pubspec.lock
for path:
indicating a path-dependency (this is necessary because path dependencies require extra checks)pubspec.lock
satisfies pubspec.yaml
.dart_tool/package_config.json
provides packages from pubspec.lock
dart pub get
semantics.We have decent logic for detecting if we need to run a full resolution. ...
Implemented here: https://github.com/dart-lang/pub/blob/master/lib/src/entrypoint.dart#L624
The implicit pub get
also has the added side-effect of stopping any pending build_runner watch
commands.
build_runner stops if a pub get
is detected. So running a Dart command in a world where the command automatically does pub get
causes build_runner
to stop, which is a major inconvenience IMO.
The implicit pub get also has the added side-effect of stopping any pending build_runner watch commands.
So, if the resolution is up-to-date (and the time-stamps are in the right order) that should not happen.
But it is a good point, that might be an undesirable side-effect.
Would it maybe make sense to offer a way to globally disable the pub get
?
Like maybe:
dart config RUN_PUB_GET=false
which sets the default for --pub
to false for all Dart commands?
Would it maybe make sense to offer a way to globally disable the pub get?
I don't think this is the level of configuration we want. That would create a cognitive overhead, and you could not look at a command and know what it is doing out of context. Also this would most likely be something that you want on a project basis (eg. you might not want it in the sdk, but always for a pub project).
The
flutter
command always runs apub get
when a sub-command that needs a resolution is executed. Thedart
command doesn't. I think the Flutter behavoir is most user friendly, so suggest we align with that.Flutter
Analyze:
Build:
Run:
Dart
The
dart
command does this for test:But not for
analyze
:Or for
compile
:Or for migrate:
Or for run: