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

Order packages topologically #1097

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Order packages topologically #1097

merged 1 commit into from
Jan 15, 2024

Conversation

smklein
Copy link
Contributor

@smklein smklein commented Jan 12, 2024

This doesn't make much of a difference for main, but it matters for an upcoming PR which adds more complexity to the Crucible build process: #1096 (reasonably so! the network service will be great to have)

Previously, Crucible packages were ordered according to the parsed order of the TOML file (which appears to have been alphabetical). With this PR, packages are now sorted topologically, so that "dependencies are built first".

Also, improved logging for error cases where dependencies cannot be found.

@smklein
Copy link
Contributor Author

smklein commented Jan 12, 2024

@karencfv : This should hopefully unblock you, I filed oxidecomputer/omicron-package#56 to pull this out as a more general-purpose utility in the omicron-package crate, so other repos don't get hit here too

Copy link
Contributor

@karencfv karencfv left a comment

Choose a reason for hiding this comment

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

Ha! That's what was missing, mystery solved. Thanks a million for looking into this 🙇‍♀️ !

@smklein smklein merged commit 4e4d18a into main Jan 15, 2024
18 checks passed
@smklein smklein deleted the topological-sort branch January 15, 2024 19:35
smklein added a commit to oxidecomputer/omicron-package that referenced this pull request Jan 15, 2024
This is basically oxidecomputer/crucible#1097 , but generalized and with tests

Adds support for topologically sorting packages, so they can be built in dependency-first order.

Fixes #56
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.

3 participants