Handle absolute paths on Windows

[DanTup]

Since the latest package:test rolled into the SDK, some DAP tests started failing to run tests with dart test. They use absolute paths on Windows: dart-lang/sdk#51348

This seems to fix the issue for me - it avoids using the URI to compute the file path if it's not a file:// URI and instead uses the raw input (but chops it at the ? to handle query params on non-URI paths).

@jakemac53

[DanTup]

There are some bot failures because we now print paths using backslashes on Windows:

                    * 00:00 +0: .\1_test.dart: test 1.2
                    * 00:00 +1: .\1_test.dart: test 1.3
                    * 00:00 +2: .\1_test.dart: test 1.1
                    * 00:00 +3: .\2_test.dart: test 2.2
                    * 00:00 +4: .\2_test.dart: test 2.3
                    * 00:00 +5: .\2_test.dart: test 2.1

I can change this, although I'm not certain that it isn't a good change. I suspect it was using forward slashes before because on Windows the file path was being read from Uri.path which meant it always had forward slashes.

@jakemac53 I might not be around too much after this evening before Monday so feel free to make changes to this PR (or fix in another) if that's preferable (there are some bot failures in the SDK because of this but I don't think what they're doing is unreasonable so think we should leave them as-is).

[jakemac53]

As a part of my recent change I updated these tests to always expect forward slashes, so we could just change the tests back again, but I don't think that is the ideal fix?

It looks to me like the specific issue here is that URIs are being passed with a drive letter but no file: scheme. In that case, we end up with a scheme of c and then an absolute path, and then our trimming of the preceding / which is intended to fix weirdness with parsing file:///C:/foo/bar style paths ends up doing the wrong thing.

I will look at trying some other fixes here.

[jakemac53]

I updated this to check if it looks like we are getting a path that starts with drive letter, and in that case prepend it with file:/// before parsing. I also made the logic for stripping out a leading / more robust (only do it if it really looks like a drive letter path).

[jakemac53]

cc @devoncarew I can't figure out from the logs why the publish bot is failing, is it because of the warnings about the tight constraints?

[DanTup]

Should the slashes be swapped too? This could result in file:///D:\foo\bar.dart which seems a little odd (even if it works).

(Edit: That might also remove the need for decodeComponent?)

Probably should only do this on Windows too, since I think colons may be valid on other platforms in filenames? (I can touch a:b.dart on mac Mac and it creates that file fine 😄)

[jakemac53]

Should the slashes be swapped too? This could result in file:///D:\foo\bar.dart which seems a little odd (even if it works).

It works just fine (it is a bit weird, but it works, I am trying to avoid as much manual munging of things as possible here, and just leave it up to the core libs).

Probably should only do this on Windows too, since I think colons may be valid on other platforms in filenames? (I can touch a:b.dart on mac Mac and it creates that file fine 😄)

Wow, lol. I guess so.

[devoncarew]

cc @devoncarew I can't figure out from the logs why the publish bot is failing, is it because of the warnings about the tight constraints?

It looks like it's failing while doing a dart pub publish --dry-run of test_core; it doesn't like the matcher: any constraint.

I imagine these warnings are known to you (intentional for this package) and wouldn't block a publish.

I'm not sure if we should make a publish dry-run not fail a build? Have it so that you can configure to allow failures per-package? Or if dart pub publish returns information vs error validations via a status code, and we could allow some types of dry-run issues?

[jakemac53]

I imagine these warnings are known to you (intentional for this package) and wouldn't block a publish.

Right, this is intentional (although a bit weird, for sure).

I'm not sure if we should make a publish dry-run not fail a build? Have it so that you can configure to allow failures per-package? Or if dart pub publish returns information vs error validations via a status code, and we could allow some types of dry-run issues?

These are just warnings - not errors. I don't know how easily you can distinguish from the tool.

I do think some packages would want to fail on warnings, so I suppose it would need to be configurable? Or we could over engineer the issue a bit and add support for some sort of ignore syntax to pub so that the warnings didn't show up at all.

[natebosch]

it doesn't like the matcher: any constraint.

FWIW we should be getting rid of that dependency whenever we can find the time to work out the breaking change in test and flutter_test.

[DanTup]

@jakemac53 is it reasonable for me to roll the latest revision into the SDK to fix dart-lang/sdk#51348 or do you want it aligned with a release of the package or to include any other changes?

[jakemac53]

This should be published, feel free to roll it into the SDK

[natebosch]

or do you want it aligned with a release of the package

FWIW we don't generally have any restriction that SDK rolls for this package are aligned with a published version. We often (and prefer to) roll into the SDK before publish.

For the most part you should fee free to roll (or ask us to roll) the test repo in to the SDK any time.

[DanTup]

Cool, thanks for the info! I've opened https://dart-review.googlesource.com/c/sdk/+/282840 - although I don't have permission to run bots or land, so if it's good someone will need to do that for me.

Thanks!