-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Gracefully handle cargo version conflicts #3213
Conversation
0c5be18
to
38ff4e0
Compare
Are there additional steps to take to test this against test projects in the dsp-testing org? |
maud = { git = "https://github.com/lambda-fairy/maud" } | ||
|
||
[build-dependencies] | ||
ructe = { git = "https://github.com/kaj/ructe" } |
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.
Wondering, could we cut this down to just ructe
and askma
which seem to have the conflicting deps? Had a go at testing this locally and the first time the test ran it took over 2 mins to run so might be due to the large poject
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.
Ah, yes. I remember you mentioned that we should whittle this down to just the important bits. I'll try to reduce this and the .lock file to just the relevant bits.
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.
Nice one, you might be able to just edit the Cargo.toml
and run cargo generate-lockfile
@@ -237,6 +235,8 @@ def handle_cargo_errors(error) | |||
return nil | |||
end | |||
|
|||
raise Dependabot::DependencyFileNotResolvable, error.message if resolvability_error?(error.message) |
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.
This is probably the right thing considering all existing specs pass but can't say I understand what all possible versions conflict
actually means 😅
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 was hoping that a failing test would guide me. I quick dive into the history lead me to a PR where the rubocop rule for line length changed from 80 to 120. I didn't go deeper than that. I'll do a deeper dive and see what source of additional context that I can discover.
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.
Thanks for putting this together and sorry for the slow review, got swamped by the bundler work.
I think this change makes sense and existing tests pass so 👍 on the change. I think the way we'll gain confidence in cargo is by making more changes and responding to issues that crop up.
Just had a comment about slimming down the test fixtures if possible, wouldn't spend ages on it but if it would work by just removing all deps except ructe
and askma
seems like it could speed up tests a fair bit?
I would usually trust tests and dry-runs for these kinds of changes. Testing it end to end in staging would add hours to the rollout right now 😬 You could test the repo in question with: |
760fc9b
to
ae7dafd
Compare
Do you mind taking another look @feelepxyz? |
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.
Nice one 🙌 LGTM
v0.135.0, 4 March 2021 * Gracefully handle cargo version conflicts #3213 * Pull request updater for azure client #3153 (@milind009) [v0.134.2...v0.135.0-release-notes](v0.134.2...v0.135.0-release-notes)
Why is this needed?
This addresses a failure that occurs when two direct cargo dependencies have
conflicting constraints against a common indirect dependency.
What does this do?
This change pushes down a line that raises an error before the check
for a version constraint conflict has occurred.
Fixes https://github.com/github/dependabot-updates/issues/1104
Before:
After:
Type of change