suggest unlocking locked pkgs that cause dep resolution failures #3970
Conversation
raquentin
force-pushed
the
suggest-unlock-deps
branch
from
fc51146 to
d40fb24
Compare
| .iter() | ||
| .map(|(name, package)| (name.clone(), package.to_hex_package(name))) | ||
| .collect(); | ||
|
|
||
| let resolved = dependency::resolve_versions( | ||
| let root_requirements_clone = root_requirements.clone(); |
There was a problem hiding this comment.
Looking for suggestions of better designs for this section. Cloning for use in calling the function recursively on the should_try_unlock page feels a bit gross
| vec
There was a problem hiding this comment.
Looking good! Thank you!
I think the logic for deciding when to offer isn't quite right at the moment, I've left comment inline.
Could you also remove the new error variants that have been added please- we don't want to change anything about the errors shown in this case.
| .iter() | ||
| .map(|(name, package)| (name.clone(), package.to_hex_package(name))) | ||
| .collect(); | ||
|
|
||
| let resolved = dependency::resolve_versions( | ||
| let root_requirements_clone = root_requirements.clone(); |
| ) | ||
| ) | ||
| } | ||
| } // end [`ResolutionError::NoSolution`] arm |
| }) | ||
| ResolutionError::Failure(err) => { | ||
| let default_msg = format!("Dependency resolution was cancelled. {err}"); | ||
| if err.contains(", but it is locked to") { |
There was a problem hiding this comment.
I don't think this logic is correct. That error message is created when a locked version falls outside the requested version constraint, but that's not the situation in which we want to offer to unlock packages. We want to unlock if there's a conflict and any of the packages in the conflict are locked to specific versions.
I don't think such logic should be in the error module as it is only concerned with the definition, construction, and displaying of errors. It doesn't know anything about the wider context of the program or why errors would be emitted.
For sure, ty for review. And yeah I'll revisit the error logic and remove the |
Resolves #3622
Changes:
Error::dependency_resolution_failedto detect dependency resolution failures due to locked packages in two waysResolutionError::NoSolution. These are errors where a locked package version is incompatible with a new package added viagleam addor via a manualgleam.tomlupdate andgleam deps downloadAND the locked package is not constrained inmanifest.toml. More manual testing is needed for the larger cli interactions.resolution_locked_version_doesnt_satisfy_requirements_indirecttests this case.lockedmap is not in the range specified inrequirements.resolution_locked_version_doesnt_satisfy_requirementstests this case.DependencyResolutionFailedWithLockedto allow pattern matching later to start the CLI action (waiting for user to [y/n] the suggestion)Questions for reviewers:
Ty