Split out a sled-hardware-types crate
#5245
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
Break's wicketd's dependency on libnvme
Break's wicketd's dependency on libnvme
papertigers
approved these changes
Mar 12, 2024
There was a problem hiding this comment.
lgtm thanks again for the assist on this.
For future reference the way we were checking where this library was needed was with elfedit -r -e 'dyn:tag NEEDED' /path/to/file.
Then combined with find:
mike - atrium ~/src/omicron (git:john/sled-hardware-types) $ fd --max-depth=1 -t f . ./target/release/ -x bash -c "elfedit -r -e 'dyn:tag NEEDED' \{} 2>&1 | rg -v elfedit | rg libnvme && echo \{}"
[1] NEEDED 0x8b956a libnvme.so.1
./target/release/sled-agent
[1] NEEDED 0x7e2b7a libnvme.so.1
./target/release/services-ledger-check-migrate
[0] NEEDED 0x4d1be1 libnvme.so.1
./target/release/installinator
[1] NEEDED 0x895f0f libnvme.so.1
./target/release/sled-agent-sim
[9] NEEDED 0x17d5ceb libnvme.so.1
./target/release/omicron-dev
[0] NEEDED 0x3f5431 libnvme.so.1
./target/release/omicron-package
This was referenced Mar 13, 2024
papertigers
added a commit
that referenced
this pull request
Mar 22, 2024
After #5158 was integrated @Rain noticed that attempting to run a build of `omdb` in the switch zone suddenly stopped working and filed oxidecomputer/helios-omicron-brand#15. @jgallagher ended up fixing this by splitting out the sled-hardware types into their own crate in #5245. We decided it would be good if we added some sort of CI check to omicron to catch these library leakages earlier. This PR introduces that check and adds it to the helios build and test buildomat job. I have also added some notes to the readme for others that may end up adding a new library dependency. Locally I modified the allow list so that it would produce errors, those errors end up looking like: ``` $ cargo xtask verify-libraries Finished dev [unoptimized + debuginfo] target(s) in 0.42s Running `target/debug/xtask verify-libraries` Finished dev [unoptimized + debuginfo] target(s) in 4.11s Error: Found library issues with the following: installinator UNEXPECTED dependency on libipcc.so.1 omicron-dev UNEXPECTED dependency on libipcc.so.1 UNEXPECTED dependency on libresolv.so.2 sp-sim UNEXPECTED dependency on libipcc.so.1 UNEXPECTED dependency on libresolv.so.2 sled-agent NEEDS libnvme.so.1 but is not allowed mgs UNEXPECTED dependency on libipcc.so.1 UNEXPECTED dependency on libresolv.so.2 If depending on a new library was intended please add it to xtask.toml ```
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.
On main as of #5158, we unexpectedly get some binaries depending on
libnvmethat shouldn't. In release builds:and in debug builds, all of the above plus
omdb. We don't really care about this for binaries that don't run on the rack, so stripping the list down to binaries we do care about:It's correct and expected that installinator and sled-agent depend on libnvme, but omdb shouldn't (and doesn't in release), and wicketd must not, as libnvme isn't available in the switch zone.
This PR fixes that incorrect dependency by splitting the parts of the
sled-hardwarecrate that wicketd and omdb depended on (directly or transitively) into a newsled-hardware-typescrate. On this PR, we are left with only the following needing libnvme, in both debug and release:I assume with a bit more work we could trim out everything but
installinatorandsled-agent, and that might be worth doing, but we'd like to land this ASAP so as to not break any updates performed off ofmain. Separately, we could also imagine a CI check that we don't have unexpected library dependencies present in the final binaries; @papertigers is going to work on that.