Skip to content
This repository was archived by the owner on Aug 28, 2024. It is now read-only.

correctly parse package_config files on windows with relative root URI #371

Merged
natebosch merged 1 commit into from
Mar 17, 2022
Merged

correctly parse package_config files on windows with relative root URI #371

natebosch merged 1 commit into from
Mar 17, 2022

Conversation

@jonahwilliams
Copy link

@jonahwilliams jonahwilliams commented Mar 17, 2022

When running flutter test --coverage on windows, we noticed that forcing package:coverage to use the package_config.json file would lead to errors like:

00:01 +1 -1: loading C:\Users\Jonah\flutter\examples\hello_world\test\hello_test.dart [E]
  Invalid argument(s): Uri c:/lib/ must have scheme 'file:'.
  package:path/src/style/windows.dart 86:7                       WindowsStyle.pathFromUri
  package:path/src/context.dart 1000:32                          Context.fromUri
  package:path/path.dart 417:32                                  fromUri
  package:coverage/src/resolver.dart 68:29                       Resolver.resolve
  package:coverage/src/hitmap.dart 60:31                         HitMap.parseJson
  ===== asynchronous gap ===========================
  package:flutter_tools/src/test/coverage_collector.dart 114:16  CoverageCollector.collectCoverage
  ===== asynchronous gap ===========================
  package:flutter_tools/src/test/coverage_collector.dart 33:5    CoverageCollector.handleFinishedTest
  ===== asynchronous gap ===========================
  package:flutter_tools/src/test/flutter_platform.dart 515:11    FlutterPlatform._startTest.<fn>
  ===== asynchronous gap ===========================
  dart:async/future.dart 611:5                                   Future.any.onValue

It turns out that if the root URI in a package config file was relative, it was not parsed correctly since resolving the provided packagesPath was not working correctly on windows (resolving to wrong dir).

To fix this, we first convert the provided packages path to a URI and then resolve that. Tests are updated to all pass on windows as well.

Unblocks flutter/flutter#99677 / https://dart-review.googlesource.com/c/sdk/+/231101

@jonahwilliams jonahwilliams mentioned this pull request Mar 17, 2022
Merged
8 tasks
jonahwilliams
jonahwilliams commented Mar 17, 2022
View reviewed changes
test/resolver_test.dart
{
"name": "foo",
"rootUri": "file:///${d.sandbox}/foo",
"rootUri": "../",
Copy link
Author

@jonahwilliams jonahwilliams Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This style of URI is used for the package itself and is what coverage was having problems resolving. Since the packagesPath would resolve to an incorrect directory, this would resolve to something like C:/lib which doesn't exist or isn't valid

@coveralls
Copy link

coveralls commented Mar 17, 2022

Coverage Status

Coverage increased (+0.02%) to 90.577% when pulling 9bbb073 on jonahwilliams:fix_windows_paths into ba4e575 on dart-lang:master.

@jonahwilliams
Copy link
Author

jonahwilliams commented Mar 17, 2022

@natebosch

@natebosch
Copy link
Contributor

natebosch commented Mar 17, 2022

cc @liamappelbe - based on this it looks like we already support package_config.json using the existing flag. I don't think we need to do any major changes.

I suppose we could consider updating the option description from "package spec" to "package config"...

https://github.com/dart-lang/coverage/blob/e2613263f893898e25ac28b4b3f74b199aca2388/bin/format_coverage.dart#L118

@natebosch natebosch requested a review from liamappelbe March 17, 2022 21:41
natebosch
natebosch approved these changes Mar 17, 2022
View reviewed changes
Copy link
Contributor

@natebosch natebosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM but it would be good for @liamappelbe to double check

@liamappelbe
Copy link
Contributor

liamappelbe commented Mar 17, 2022

LGTM

cc @liamappelbe - based on this it looks like we already support package_config.json using the existing flag. I don't think we need to do any major changes.

I suppose we could consider updating the option description from "package spec" to "package config"...

https://github.com/dart-lang/coverage/blob/e2613263f893898e25ac28b4b3f74b199aca2388/bin/format_coverage.dart#L118

It's true that no major changes are needed for the .packages breaking change. We're already migrated in fact. I really just wanted to add a --package flag so the users can omit the /.dart_tool/package_config.json from their path, since it's unnecessary (since we can use findPackageConfig) and it's weird that users need to know about this hidden file. Then Michael wanted me to delete the --packages flag at the same time. Anyway, that's not really relevant to this PR, which LGTM.

@natebosch natebosch merged commit aab92ee into dart-archive:master Mar 17, 2022
@jonahwilliams jonahwilliams deleted the fix_windows_paths branch March 17, 2022 21:53
mosuem pushed a commit to dart-lang/tools that referenced this pull request Aug 28, 2024
correctly parse package_config files on windows with relative root URI (
c85e0be 
dart-archive/coverage#371)
@liamappelbe liamappelbe mentioned this pull request May 12, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@liamappelbe liamappelbe liamappelbe approved these changes

@natebosch natebosch natebosch approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jonahwilliams @coveralls @natebosch @liamappelbe