-
Notifications
You must be signed in to change notification settings - Fork 233
dart pub get --enforce-lockfile #3637
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.
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
Conversation
lib/src/solver/report.dart
Outdated
|
|
||
| log.message(_output); | ||
| if (enforceLockfile && hasChanges) { | ||
| dataError('Could not enforce the lockfile.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be better if these error messages mentioned:
- Resolution aborted: Unable to satisfy
pubspec.yamlusing the existingpubspec.lock - Try without
--enforce-lockfileto updatepubspec.lock. - X number of dependencies changed in
pubspec.lock. pubspec.lockdoes not exist (if that is the case).
Just some ideas for information that should go into this message.
Interesting side note:
It's unclear if we actually should allow resolution to go ahead. I think it's the best implementation for us.
But it leaves the user with weird behavior, where --enforce-lockfile can cause a dependency conflict.
Which should never be possible, because --enforce-lockfile implies that I don't want resolution to be taking place at all.
But I'm not sure we want the number of hacks necessary to work around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
X number of dependencies changed in pubspec.lock.
This is mentioned by the report.summary.
pubspec.lock does not exist (if that is the case).
That case is handled separately before even attempting to resolve.
lib/src/entrypoint.dart
Outdated
| await report.summarize(); | ||
| if (enforceLockfile && hasChanges) { | ||
| dataError( | ||
| 'Could not enforce the lockfile$suffix.${onlyReportSuccessOrFailure ? forDetails() : ''}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, shouldn't this say:
Cannot not enforce
pubspec.lock, trydart pub getwithout--enforce-lockfileto resolve dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discrepancies will have been output by report.show+summarize at this point.
But adding the suggestions to try without --enforce-lockfile is good.
| : ' --directory ${root.dir}'; | ||
| throw ApplicationException( | ||
| 'Resolving dependencies$suffix failed. For details run `$topLevelProgram pub ${type.toString()}$directoryOption`'); | ||
| 'Resolving dependencies$suffix failed.${forDetails()}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think forDetails can be reused out of this flow. So why not move it in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using it also below when the resolution succeeded, but something changed from the pubspec.lock.
Co-authored-by: Jonas Finnemann Jensen <jopsen@gmail.com>
| 'enforce-lockfile', | ||
| negatable: false, | ||
| help: | ||
| 'Enforce pubspec.lock. Fail resolution if pubspec.lock does not satisfy pubspec.yaml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'Enforce pubspec.lock. Fail resolution if pubspec.lock does not satisfy pubspec.yaml', | |
| 'Enforce pubspec.lock, fails resolution if pubspec.lock does not satisfy pubspec.yaml', |
Don't know, maybe I'm just crazy -- English is not my first language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both are valid. Landing as is - we can follow up with a fix if you disagree.
Fixes: #2890