Remove duplicate global invocation on Windows

[Levi-Lesches]

In the Batch file defined in _createBinStub, the script attempts to call the package using the dart snapshot. If it fails, the Batch script uses dart pub global directly. However, the dart pub global command is still present even when using the snapshot works. This PR removes the second call so that package is only run once. This is especially important/urgent because a lot of executable packages focus on project management, and make sensitive changes to the files in a user's project. Having those changes run twice can cause errant behavior.

EDIT: Adding a call before the dart command in the generated binstub fixes an error where, if the user has flutter\bin\dart.bat ahead of flutter\bin\cache\dart-sdk\bin\dart.exe in their PATH, the script gets called twice. I tried tracking down the cause but couldn't. All I know is that something happens in flutter's dart.bat that really messes with the binstub, and won't even let the simple goto error statement run properly. Using call fixes that... somehow.

Fixes #2934

[Levi-Lesches]

pub/lib/src/global_packages.dart

Lines 752 to 765 in 26576f6

	if exist "$snapshot" (
	dart "$snapshot" %*
	rem The VM exits with code 253 if the snapshot version is out-of-date.
	rem If it is, we need to delete it and run "pub global" manually.
	if not errorlevel 253 (
	goto error
	)
	) else (
	dart pub global run ${package.name}:$script %*
	)
	goto eof
	:error
	exit /b %errorlevel%
	:eof

Adding the CALL command in line 753 instead of deleting the line I did also fixes this issue. I'll look into which one is preferable.

[Levi-Lesches]

Okay, I figured it out. The error is not the duplicate dart pub line, it's the need for the call command in Dart. I added that now. I also added a test (that previously would have failed) that catches when the program runs twice. All the tests currently use emits which won't check duplicate events. So I added a neverEmits directly after.

All tests in \test\global\binstubs now pass.

[jonasfj]

If dart from PATH is actually dart.bat then using call makes a difference, because invoking a bat script from another bat script will stop the original bat script from working.

How that triggers this to be run twice I don't understand.

hmm, can this cause problems when dart is not a dart.bat file, which is the case when installed from the Dart SDK.

[Levi-Lesches]

I have no clue how but from several hours of investigating, I found that even when the if branch of the errorlevel 253 if statement runs, the goto error line doesn't run. I put an echo before and after and both are printed -- it's almost like the goto is skipped. Using call somehow prevents this from happening. My only guess is that something in dart.bat, or one of the batch files it calls, must be changing something or setting a conflicting label, and that using call tells CMD to use Dart in a separate context. I couldn't find anything concrete though.

When dart is really dart.exe (in flutter/bin/cache/dart-sdk/bin/dart.exe, the place dart.bat forwards to), call still works. I ran all the binstub tests for both versions of PATH, making sure to close cmd.exe.

[Levi-Lesches]

To clarify, it isn't the dart snapshot that runs twice. Instead, the goto error line is skipped, even when the if statement is run. That causes the first dart pub global run command to run as well, effectively running the executable twice.

That's why so many fixes out there (and my initial draft for this PR) simply deleted the first dart pub global run line. But that broke some tests, this is the proper solution.

[Levi-Lesches]

The review for this change is here:

https://dart-review.googlesource.com/c/pub/+/208620

[Levi-Lesches]

@jonasfj, @sigurdm, is this PR good? There are many issues across many repos (almost any packages you'd install with dart pub global activate) that would be fixed by this.

[sigurdm]

This LGTM - @jonasfj do you have anything more?

[jonasfj]

LGTM

[kevmoo]

Are we going to land this?

[sigurdm]

I added a specific regression test.