Migrate .packages -> package_config.json by sigurdm · Pull Request #99677 · flutter/flutter

sigurdm · 2022-03-07T12:33:24Z

Replace .packages with .dart_tool/package_config.json in BuildInfo.packagesPath.

Fixes: #99678

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

sigurdm · 2022-03-10T11:28:34Z

Not sure about the failing windows test - is it a regular timeout or is something else going on...

jonahwilliams

I think the test itself might be flaky right now, though @christopherfujino might know more.

Otherwise LGTM, if LG to @christopherfujino

christopherfujino · 2022-03-10T20:13:01Z

packages/flutter_tools/test/general.shard/flutter_tester_device_test.dart

        BuildMode.debug,
        '',
        treeShakeIcons: false,
-        packagesPath: '.dart_tool/package_config.json',


why was this field removed?

because this is now the default argument

christopherfujino · 2022-03-10T20:13:19Z

Re-ran the tool integration test.

christopherfujino · 2022-03-10T20:27:24Z

packages/flutter_tools/lib/src/build_info.dart

    required this.treeShakeIcons,
    this.performanceMeasurementFile,
-    this.packagesPath = '.packages', // TODO(zanderso): make this required and remove the default.
+    this.packagesPath = '.dart_tool/package_config.json', // TODO(zanderso): make this required and remove the default.


Maybe this hard-coded path separator is leading to coverage collection failing on Windows?

odd that it is failing only for windows. Really, you should be able to use / conditionally ...

Perhaps the coverage code is doing some string matching instead of using package:path ?

Using the platform separator will make the constructor non-const (which might not be a problem).

Cursory inspection doesn't show anything in package:coverage that should depend on backslash on windows...

I'll debug this one today, I use a windows machine

Gentle ping

This is blocking https://dart-review.googlesource.com/c/sdk/+/231101 .

jonahwilliams · 2022-03-17T17:18:24Z

So from investigation, it looks like the coverage resolver in package:coverage wants the packages path to be both a file path and a URI, which only works on Linux/macOS:

https://github.com/dart-lang/coverage/blob/master/lib/src/resolver.dart#L87

I'm going to work out how to fix this

jonahwilliams · 2022-03-17T17:49:48Z

Fix is dart-archive/coverage#371 , which needs to be rolled into flutter first

christopherfujino · 2022-03-24T21:11:03Z

Fix is dart-lang/coverage#371 , which needs to be rolled into flutter first

That PR was merged, we are now blocked on a new published pub version. @bkonyi or @natebosch do you know the process for publishing package:coverage? it looks like there are several batched changes.

bkonyi · 2022-03-24T21:15:22Z

Fix is dart-lang/coverage#371 , which needs to be rolled into flutter first

That PR was merged, we are now blocked on a new published pub version. @bkonyi or @natebosch do you know the process for publishing package:coverage? it looks like there are several batched changes.

Either @natebosch or @liamappelbe (maybe) should be able to publish a new version.

christopherfujino · 2022-03-24T21:50:10Z

Fix is dart-lang/coverage#371 , which needs to be rolled into flutter first

That PR was merged, we are now blocked on a new published pub version. @bkonyi or @natebosch do you know the process for publishing package:coverage? it looks like there are several batched changes.

Either @natebosch or @liamappelbe (maybe) should be able to publish a new version.

Pretty please :)

liamappelbe · 2022-03-24T21:56:50Z

Fix is dart-lang/coverage#371 , which needs to be rolled into flutter first

That PR was merged, we are now blocked on a new published pub version. @bkonyi or @natebosch do you know the process for publishing package:coverage? it looks like there are several batched changes.

Either @natebosch or @liamappelbe (maybe) should be able to publish a new version.

Pretty please :)

Done

christopherfujino · 2022-03-26T00:33:49Z

@sigurdm if you rebase to ef8e357 or later, you should pick up the fix to package:coverage

…in_build_info

Until recently, BuildInfo.packagesPath defaulted to `.packages` however, the .packages file has been deprecated [1] and is being removed. In flutter#99677 the default was changed to `.dart_tool/package_config.json` but the doc comment was accidentally updated to `.dart_tool/packages`. This corrects it to the true default value. 1: dart-lang/sdk#48272 Spotted in the midst of some related cleanup relating to flutter#103775

Until recently, BuildInfo.packagesPath defaulted to `.packages` however, the .packages file has been deprecated [1] and is being removed. In #99677 the default was changed to `.dart_tool/package_config.json` but the doc comment was accidentally updated to `.dart_tool/packages`. This corrects it to the true default value. 1: dart-lang/sdk#48272 Spotted in the midst of some related cleanup relating to #103775

Replace .packages with .dart_tool/package_config.json in BuildInfo

98f9450

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Mar 7, 2022

sigurdm added 4 commits March 7, 2022 12:59

Fix more tests

c091ed8

Fix lint

b211571

Fix expectation

9710562

Fix expectation

13cd70b

sigurdm changed the title ~~Replace .packages with .dart_tool/package_config.json in BuildInfo~~ Migrate .packages -> package_config.json Mar 7, 2022

Fix expectation

4847d81

sigurdm closed this Mar 10, 2022

sigurdm reopened this Mar 10, 2022

sigurdm requested a review from jonahwilliams March 10, 2022 11:28

jonahwilliams requested a review from christopherfujino March 10, 2022 16:52

jonahwilliams approved these changes Mar 10, 2022

View reviewed changes

christopherfujino reviewed Mar 10, 2022

View reviewed changes

jonahwilliams mentioned this pull request Mar 17, 2022

correctly parse package_config files on windows with relative root URI dart-archive/coverage#371

Merged

sigurdm mentioned this pull request Mar 24, 2022

Revert "Add flag controlling creation of .packages file. (#2757)" dart-lang/pub#3357

Merged

liamappelbe mentioned this pull request Mar 24, 2022

Prepare for 1.2.0 release dart-archive/coverage#373

Merged

christopherfujino mentioned this pull request Mar 24, 2022

[flutter_tools] update pub packages #100726

Merged

Merge remote-tracking branch 'origin/master' into use_package_config_…

96ba4d4

…in_build_info

sigurdm merged commit 66ed64b into flutter:master Mar 30, 2022

engine-flutter-autoroll mentioned this pull request Mar 30, 2022

Roll Flutter from f07f6fef9ae9 to 66ed64be4fd8 (6 revisions) flutter/plugins#5128

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Mar 30, 2022

66ed64b Migrate .packages -> package_config.json (flutter/flutter#99677)

efca1cb

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Apr 4, 2022

66ed64b Migrate .packages -> package_config.json (flutter/flutter#99677)

6a86120

sigurdm mentioned this pull request May 4, 2022

Reland "Add flag controlling creation of .packages file." dart-lang/pub#3413

Merged

cbracken mentioned this pull request May 14, 2022

[tool] Fix BuildInfo.packagesPath doc comment #103785

Merged

8 tasks

aebrahim mentioned this pull request Jun 5, 2022

Flutter tool should only create its own files in a .flutter directory #105374

Closed

Conversation

sigurdm commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

sigurdm commented Mar 10, 2022

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Mar 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Mar 17, 2022

Uh oh!

jonahwilliams commented Mar 17, 2022

Uh oh!

christopherfujino commented Mar 24, 2022

Uh oh!

bkonyi commented Mar 24, 2022

Uh oh!

christopherfujino commented Mar 24, 2022

Uh oh!

liamappelbe commented Mar 24, 2022

Uh oh!

christopherfujino commented Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sigurdm commented Mar 7, 2022 •

edited

Loading

christopherfujino commented Mar 26, 2022 •

edited

Loading