Skip to content
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

suggest unlocking locked pkgs that cause dep resolution failures #3970

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

raquentin
Copy link
Contributor

@raquentin raquentin commented Dec 8, 2024

Resolves #3622

compiler-cli/src/dependencies.rs Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
@raquentin raquentin marked this pull request as ready for review January 2, 2025 00:16
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

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.

compiler-cli/src/dependencies.rs Show resolved Hide resolved
compiler-core/src/error.rs Outdated Show resolved Hide resolved
})
ResolutionError::Failure(err) => {
let default_msg = format!("Dependency resolution was cancelled. {err}");
if err.contains(", but it is locked to") {
Copy link
Member

Choose a reason for hiding this comment

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

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.

@lpil lpil marked this pull request as draft January 9, 2025 14:21
@raquentin
Copy link
Contributor Author

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.

For sure, ty for review. And yeah I'll revisit the error logic and remove the WithLocked variant

@raquentin raquentin marked this pull request as ready for review January 16, 2025 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offer to attempt again with conflicting deps unlocked when version resolution fails
2 participants