always specify branch in git dependencies #552
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Created using spr 1.3.4
Created using spr 1.3.4
|
There are no functional changes here so I'm just going to land this to unblock omicron. |
sunshowers
deleted the
sunshowers/spr/always-specify-branch-in-git-dependencies
branch
|
The one tricky non obvious dependency here is being sure that both Omicron and Propolis can build with In this case, nothing has changed recently, so it should be fine. And usually when we are in these intermediate |
|
@leftwo yeah! I did run into this in oxidecomputer/propolis#262. |
sunshowers
added a commit
to oxidecomputer/propolis
that referenced
this pull request
Dec 3, 2022
This is PR 2 of N to make sure that omicron doesn't get two separate dependencies, one for progenitor with main and one for progenitor without main. For PR 1, see oxidecomputer/crucible#552. Without this fix, omicron ends up with two separate versions of crucible, which causes build failures in omicron. The update caused some breakage from oxidecomputer/crucible#516, which I've tried to address (cc @leftwo to confirm that my changes look good). Reviewers: gjcolombo Reviewed By: gjcolombo Pull Request: #262
sunshowers
added a commit
to oxidecomputer/omicron
that referenced
this pull request
Dec 5, 2022
This is PR 3 of N in this series. For the earlier ones, see: * oxidecomputer/crucible#552 * oxidecomputer/propolis#262 Currently, we specify the progenitor dependency as both: ```toml progenitor-client = { git = "https://github.com/oxidecomputer/progenitor", branch = "main" } ``` And ```toml progenitor-client = { git = "https://github.com/oxidecomputer/progenitor" } ``` Cargo (due to rust-lang/cargo#7497) doesn't dedup these dependencies even though they resolve to the same branch and hash. This causes issues with `cargo doc` colliding with itself, as it tries to document both: ``` Documenting progenitor-impl v0.2.1-dev (https://github.com/oxidecomputer/progenitor#4b5cef4b) Documenting progenitor-impl v0.2.1-dev (https://github.com/oxidecomputer/progenitor?branch=main#4b5cef4b) ``` Fix this by always using the branch name. (We choose this option over not specifying the branch so to maintain uniformity in cases where a non-default branch must be used.) This also requires a crucible update, since that didn't specify a branch either. And the crucible update necessitates a propolis update to get the versions aligned. In the future it would be nice to have a lint which ensures that git dependencies always have the branch name in them.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is PR 1 of 3 to make sure that omicron doesn't get two separate
dependencies, one for progenitor with main and one for progenitor
without main.
We choose this strategy over not doing so to maintain uniformity in
cases where a non-default branch must be used.