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

dedup progenitor, and update crucible and propolis revs #2005

Merged

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Dec 2, 2022

This is PR 3 of N in this series. For the earlier ones, see:

Currently, we specify the progenitor dependency as both:

progenitor-client = { git = "https://github.com/oxidecomputer/progenitor", branch = "main" }

And

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.

Created using spr 1.3.4
@sunshowers sunshowers requested a review from ahl December 2, 2022 21:41
@sunshowers sunshowers marked this pull request as draft December 2, 2022 23:27
@sunshowers
Copy link
Contributor Author

This is going to need oxidecomputer/propolis#262 as well.

Created using spr 1.3.4
Copy link
Contributor

@leftwo leftwo left a comment

Choose a reason for hiding this comment

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

You will also need to update the packaging side as well, with new links to the CI produced
archives for propolis and crucible, as we want the compiled Omicron to match the tar.gz file that becomes part of the installed image.

Check out the package-manifest.toml file for details.
You will have to wait for both crucible and propolis to finish their CI runs on main/master before you will be able to get the zip archive and SHA hash needed here.

Created using spr 1.3.4
Created using spr 1.3.4
@sunshowers sunshowers changed the title dedup progenitor, and update crucible rev dedup progenitor, and update crucible and propolis revs Dec 3, 2022
Copy link
Contributor

@ahl ahl left a comment

Choose a reason for hiding this comment

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

looks good

@zephraph
Copy link
Contributor

zephraph commented Dec 5, 2022

Is the helios failure just a flake?

@sunshowers
Copy link
Contributor Author

Is the helios failure just a flake?

I think it might be genuine. I'm not entirely sure. I'm going to dig into it later today.

@luqmana
Copy link
Contributor

luqmana commented Dec 5, 2022

Looks like bytecodealliance/rustix#467

@sunshowers
Copy link
Contributor Author

Ah OK, that's easy enough to patch in omicron. Will put up a fix for it.

Created using spr 1.3.4
Created using spr 1.3.4
@sunshowers sunshowers marked this pull request as ready for review December 5, 2022 22:14
Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

Thanks!

@sunshowers sunshowers merged commit 2ceaf76 into main Dec 5, 2022
@sunshowers sunshowers deleted the sunshowers/spr/dedup-progenitor-and-update-crucible-rev branch December 5, 2022 22:38
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.

5 participants