VS Code should never run dart pub without asking

[scheglov]

Specifically, I think it should not do this in Dart SDK at all.
See https://dart-review.googlesource.com/c/sdk/+/295661

@bwilkerson @DanTup

[DanTup]

@stereotype441

I'm finding that VSCode is a lot more aggressive about running dart pub get than IntelliJ is.

Have you noticed any pattern to when this happens? Dart-Code does try to detect the Dart SDK and disable any automatic running of pub:

https://github.com/Dart-Code/Dart-Code/blob/ae5b6ed9c5969955e1e01ba69237c5b4ef12c150/src/shared/utils/workspace.ts#L6-L7

It's possible this isn't covering all cases, but it's also possible we're triggering some other command that does an implicit pub get (like dart run or dart test).

I did hit these bogus errors a month or two back (the fix is to delete the .dart_tool folders that have been created inside a pkg/ subfolder) but was unable to reproduce it and concluded I must have accidentally run dart test or similar from the command line. It seems that assumption might have been incorrect.

I'll do some testing and see if I can find a way to trigger this. If anyone knows of any steps that might lead to it, please let me know!

[bwilkerson]

I believe that the automatic running of pub get from the dart command has been reverted, though there's discussion about whether that was the right choice. It's possible that the behavior has changed since the last time you tested it.

[DanTup]

Yeah, changes for #50422 were reverted. Although I think dart test already (and still does) implicitly do a pub get because it just delegates to pub run test?

It's possible the recent instance of this I saw was related to those (now-reverted) changes though.

It would be nice if there was some flag in the SDK that just preventing pub from doing this, rather than tools (and users) needing to know to never invoke these commands. Special-casing the Dart SDK in Dart-Code to protect it from an SDK tool breaking things feels a little awkward (not to mention isn't available to other projects that might have similar constraints).

[stereotype441]

I'm using a locally-built SDK that was definitely from after the revert, so I'm pretty sure it's not dart run that's causing pub get to happen. @DanTup, I suspect that you're right about your suspicion that the delegation to pub run test is the problem I'm seeing.

[stereotype441]

Some more information: this morning I added a new SDK folder (pkg/analysis_server) to my VSCode workspace for the first time. VSCode immediately gave me the message "Some packages are missing or out of date, would you like to get them now?", and I responded "no". I checked and there was no .dart_tool directory in pkg/analysis_server, so this confirms that the VSCode plug-in respected my "no".

Then I opened the file pkg/analysis_server/test/src/services/correction/fix/add_missing_switch_cases_test.dart and clicked the Run link above void main(). The following message appeared in my debug console: Because analysis_server depends on analyzer_utilities any which doesn't exist (could not find package analyzer_utilities at https://pub.dev), version solving failed. So it looks like clicking Run triggered pub.

I then added a dependency_overrides section to the analysis server pubspec. Once again, VSCode offered to do a pub get on my behalf, and once again I said no. I double checked and there was still no .dart_tool directory in pkg/analysis_server after this.

Then I clicked the Run link again, and the tests ran successfully. After running the tests, I checked, and there was a .dart_tool directory under pkg/analysis_server, containing package_config.json, as well as subfolders pub and test.

Now, if I touch the analysis server pubspec file, the VSCode plug-in automatically runs pub get without asking. But I'm not sure if this was caused by running the test or by some other action I took since running this experiment.

All of this is using the SDK as of fa3a72f (which is after the changes for #50422 were reverted).

So, to summarize:

• The first time VSCode encountered a package directory, it asked for permission before running pub get, and respected the user's answer.
• Attempting to run a test (by clicking the Run link above the main function) caused a pub get to happen automatically.
• This was not caused by the changes for dart cli commands should run pub get #50422 (since I'm using an SDK after those changes were reverted).
• Now, further changes to the package's pubspec.yaml caused VSCode to automatically rerun pub get. But I'm not sure what caused this.

[DanTup]

@stereotype441 thanks! I wouldn't have expected you to have even seen the "Some packages are missing or out of date" prompt - it should all be disabled.

I'll do some testing, but could you confirm what your .code-workspace looks like? Where does the file live, and what folders do you have in it? I presume there's nothing you'd consider unusual about your SDK checkout (eg. it's complete and has a .git folder at the root?)

Edit:

further changes to the package's pubspec.yaml caused VSCode to automatically rerun pub get

This definitely suggests we didn't detect your workspace as being the Dart SDK. I'll take a look.

[stereotype441]

@stereotype441 thanks! I wouldn't have expected you to have even seen the "Some packages are missing or out of date" prompt - it should all be disabled.

I'll do some testing, but could you confirm what your .code-workspace looks like? Where does the file live, and what folders do you have in it? I presume there's nothing you'd consider unusual about your SDK checkout (eg. it's complete and has a .git folder at the root?)

The workspace settings JSON file lives in my home directory (/usr/local/google/home/paulberry/dart1.code-workspace).

The contents of that file look like this:

{
	"folders": [
		{
			"path": "dart1/sdk/pkg/_fe_analyzer_shared"
		},
		{
			"path": "dart1/sdk/pkg/analysis_server"
		},
		{
			"path": "dart1/sdk/pkg/analyzer"
		},
		{
			"path": "dart1/sdk/pkg/front_end"
		},
	],
	"settings": {}
}

My SDK checkout is slightly unusual in that it's a linked worktree (see git worktree documentation). This means that instead of having a .git folder at the root, it has a .git file that points to the main worktree.

Edit:

further changes to the package's pubspec.yaml caused VSCode to automatically rerun pub get

This definitely suggests we didn't detect your workspace as being the Dart SDK. I'll take a look.

[DanTup]

it has a .git file

Aha, that'll be it. I see I'm explicitly checking for a folder.

I've opened Dart-Code/Dart-Code#4501 for fixing this, and also Dart-Code/Dart-Code#4502 to track whether we can relax the restrictions on disabling the test runner for the SDK.

I also just noticed that in the analysis server to suppress pub we only check for the presence of a DEPS file up the tree. I wonder whether we should change Dart-Code to do exactly the same. This would be slightly more generic than looking specifically for a git root with README.dart-sdk, although I don't know who else might have DEPS and whether it would be correct or not to do the same? 🤔

[stereotype441]

I also just noticed that in the analysis server to suppress pub we only check for the presence of a DEPS file up the tree. I wonder whether we should change Dart-Code to do exactly the same. This would be slightly more generic than looking specifically for a git root with README.dart-sdk, although I don't know who else might have DEPS and whether it would be correct or not to do the same? 🤔

That's interesting. On the one hand, I can see the benefit to consistency between the analysis server and the VSCode plug-in. On the other hand, DEPS is a part of Google open-source infrastructure that's potentially used by other teams, so there's a risk that if we check for it, the check will be overly broad.

On the other other hand, I really like it when we can make the experience of the SDK developers as consistent as possible with the experience of our customers, because it means we do a better job of "eating our own dog food" and finding bugs that customers will care about. So even though I do agree that it's a little weird for tools to be automatically run dart pub without asking, I personally think we should make a decision about whether or not to run pub based on customer needs, and then whatever we decide, I don't want there to be any exception for the SDK. It looks like by adding dependency_overrides sections in the SDK's pubspecs, we'll make it safe for the tools to run pub within the SDK repo. Once we've done that, I won't care terribly much about whether or not VSCode or dart test runs pub under the hood, and I won't care very much how VSCode detects that it's editing the SDK repo, because those things will have ceased to have so much impact on the development experience 😃

[DanTup]

For now I've left the check as-is, but allow .git files as well as folders. Changing the behaviour for non-SDK trees with a DEPS file feels a bit more risky.

I don't think there's anything more to do here now that's fixed (via Dart-Code/Dart-Code#4501), although it would definitely be nice if we could agree an official way to mark (perhaps in a pubspec.yaml?) that a project is not using pub for managing the dependencies so it's clear what projects can do to opt-out and all tools can have exactly the same rules.

@scheglov are you happy for me to close this, or do you think we need more control of this for non-SDK projects too? (there are some existing settings to disable things like pub-get-on-save, though it's on by default for convenience since most users probably want it).