Skip to content

Add flag controlling creation of .packages file. #2757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 8 commits into from
Jan 31, 2022
Merged

Add flag controlling creation of .packages file. #2757

merged 8 commits into from
Jan 31, 2022

Conversation

lrhn
Copy link
Member

@lrhn lrhn commented Nov 16, 2020 •
edited by mit-mit
Loading

WDYT?

Related to #2756

@lrhn
Add flag controlling creation of .packages file.
cf7627c 
Created in anticipation of switching the default to *not* generate the file.
@lrhn lrhn requested a review from jonasfj November 16, 2020 15:16
@google-cla google-cla bot added the cla: yes label Nov 16, 2020
@jonasfj
Copy link
Member

jonasfj commented Nov 16, 2020

Hmm, I think we have to update some of the code that asserts state is up-to-date.

You're suggesting, we: add a flag that allows disabling generation of .packages and we later flip the default for the flag.

I can see the argument for starting to remove .packages now that we're doing null-safety.

@sigurdm
Copy link
Contributor

sigurdm commented Nov 19, 2020

Hmm, I think we have to update some of the code that asserts state is up-to-date.

I have landed: #2764 that removes the assert that .packages is up to date in case it doesn't exist, it also removes several other places where we wrongly relied on .packages.

@sigurdm sigurdm closed this Nov 19, 2020
@sigurdm sigurdm reopened this Nov 19, 2020
@mit-mit
Copy link
Member

mit-mit commented Jan 8, 2021

@lrhn @sigurdm do we still need this?

@sigurdm
Copy link
Contributor

sigurdm commented Jan 8, 2021

Hmm - I don't remember where we ended up last we discussed this.

But IMO we should just continue creating this file until dart 3.
No reason to make unnecessary breaking changes before a breaking version shift.
On the other hand we could generate a comment inside the .packages file explaining that it is deprecated.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 25, 2022

I merged this with upstream and turned the flag to be off by default.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 27, 2022

ping @jonasfj

@mit-mit mit-mit mentioned this pull request Jan 27, 2022
Closed
@mit-mit
Copy link
Member

mit-mit commented Jan 27, 2022 •
edited
Loading

@sigurdm can you clarify the flag behavoir? Is the flag --packages-file to create the .packages file & it's off by default (i.e. when this lands we'll no longer create the file by default)?

@sigurdm
Copy link
Contributor

sigurdm commented Jan 27, 2022

Is the flag --packages-file to create the .packages file

yes

it's off by default (i.e. when this lands we'll no longer create the file by default)?

yes

@mit-mit
Copy link
Member

mit-mit commented Jan 27, 2022

OK. We would have to send the breaking change notice for this first then.

This is a slightly faster approach than the one discussed in #2756 (which suggested first having a flag you must opt-in to using to skip the generation of .packages), but given how old this file is, I'm OK with moving ahead here. @lrhn WDYT?

@sigurdm
Copy link
Contributor

sigurdm commented Jan 27, 2022

My thinking was that now we had the deprecation notice in the file we should be able to take this step.

But I'm also ok with having the .packages generation still be on by default.

@lrhn
Copy link
Member Author

lrhn commented Jan 28, 2022 •
edited
Loading

We introduced package_config.json in Dart 2.8. I think almost two years should be enough warning!
Ship it!

jonasfj
jonasfj approved these changes Jan 28, 2022
View reviewed changes
mit-mit
mit-mit reviewed Jan 28, 2022
View reviewed changes
lib/src/command/add.dart Outdated
@@ -93,7 +93,7 @@ class AddCommand extends PubCommand {
argParser.addOption('directory',
abbr: 'C', help: 'Run this in the directory <dir>.', valueHelp: 'dir');
argParser.addFlag('packages-file',
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to cal this flag legacy-package-file just to be extraordinarily clear?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

@sigurdm sigurdm merged commit a246141 into master Jan 31, 2022
@mit-mit mit-mit deleted the packages-file-flag branch February 1, 2022 13:07
@sigurdm sigurdm mentioned this pull request Mar 7, 2022
Closed
sigurdm added a commit to sigurdm/pub that referenced this pull request Mar 24, 2022
@sigurdm
Revert "Add flag controlling creation of .packages file. (dart-lang…
9e3e111 
…#2757)"

This reverts commit a246141.
sigurdm added a commit that referenced this pull request Mar 24, 2022
@sigurdm
Revert "Add flag controlling creation of .packages file. (#2757)" (#…
d1c0e3f 
…3357)

This reverts commit a246141.
sigurdm added a commit to sigurdm/pub that referenced this pull request May 4, 2022
@sigurdm
Revert "Revert "Add flag controlling creation of .packages file. (d…
1858680 
…art-lang#2757)" (dart-lang#3357)"

This reverts commit d1c0e3f.
@sigurdm sigurdm mentioned this pull request May 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sigurdm sigurdm sigurdm left review comments

@mit-mit mit-mit mit-mit left review comments

@jonasfj jonasfj jonasfj approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lrhn @jonasfj @sigurdm @mit-mit