ls-apis should use package-manifest.toml to figure out which version of related repos to use by davepacheco · Pull Request #10220 · oxidecomputer/omicron

davepacheco · 2026-04-03T22:30:06Z

(depends on #10217)

This change causes ls-apis to parse package-manifest.toml to figure out what commits of related repos (like Crucible, Dendrite, etc.) will actually be deployed (from the current Omicron workspace). It then uses this information to choose the correct clone of the repo to use for its analysis.

One other change I made here was to tie lldpd-client and its protocol package to the version that's deployed in package-manifest.toml. This ought to fix #10361. (A previous version of this PR updated package-manifest.toml instead, but I opted for the smaller change here.)

Background

ls-apis needs access to checked-out repos for Omicron as well as related components like Dendrite, LLDP, Crucible, Propolis, etc. It wants the versions of these repos that get deployed on real systems (based on the Omicron workspace that it's running in), since the goal is to analyze the runtime API dependencies between these components. It could create its own clones of these repos, but instead, it leverages the fact that just running cargo metadata in Omicron requires having downloaded copies of all of these repos already. How does ls-apis find these copies? It uses Cargo to locate a package that's known to be in that repo. Generally, it picks the package of a client that Omicron already from that repo, like dpd-client to find Dendrite.

But it's not quite so simple: Omicron can reference multiple versions of a given repo. More specifically: Omicron may reference dpd-client from multiple versions of Dendrite. This happens with dpd-client specifically:

$ cargo tree -e normal -i dpd-client
error: There are multiple `dpd-client` packages in your project, and the specification `dpd-client` is ambiguous.
Please re-run this command with one of the following specifications:
  git+https://github.com/oxidecomputer/dendrite?branch=main#dpd-client@0.1.0
  git+https://github.com/oxidecomputer/dendrite?rev=44a949c9bedf4fcd4d280337fa1965b4293c88d1#dpd-client@0.1.0
  git+https://github.com/oxidecomputer/dendrite?rev=cc8e02a0800034c431c8cf96b889ea638da3d194#dpd-client@0.1.0

$ cargo tree -e normal -i git+https://github.com/oxidecomputer/dendrite?branch=main#dpd-client@0.1.0
dpd-client v0.1.0 (https://github.com/oxidecomputer/dendrite?branch=main#f20f786e)
└── lldpd-common v0.1.0 (https://github.com/oxidecomputer/lldp?rev=c3305fd1a7ea7aba31f3834757a6b931e4f59fe6#c3305fd1)
    └── lldpd-client v0.1.0 (https://github.com/oxidecomputer/lldp?rev=c3305fd1a7ea7aba31f3834757a6b931e4f59fe6#c3305fd1)
        └── omicron-nexus v0.1.0 (/home/dap/omicron-review/nexus)
            └── omicron-dev v0.1.0 (/home/dap/omicron-review/dev-tools/omicron-dev)

$ cargo tree -e normal -i git+https://github.com/oxidecomputer/dendrite?rev=44a949c9bedf4fcd4d280337fa1965b4293c88d1#dpd-client@0.1.0
dpd-client v0.1.0 (https://github.com/oxidecomputer/dendrite?rev=44a949c9bedf4fcd4d280337fa1965b4293c88d1#44a949c9)
├── nexus-test-utils v0.1.0 (/home/dap/omicron-review/nexus/test-utils)
│   └── omicron-dev v0.1.0 (/home/dap/omicron-review/dev-tools/omicron-dev)
├── omicron-nexus v0.1.0 (/home/dap/omicron-review/nexus)
│   └── omicron-dev v0.1.0 (/home/dap/omicron-review/dev-tools/omicron-dev)
├── wicket-common v0.1.0 (/home/dap/omicron-review/wicket-common)
│   ├── wicket v0.1.0 (/home/dap/omicron-review/wicket)
│   │   └── wicket-dbg v0.1.0 (/home/dap/omicron-review/wicket-dbg)
│   ├── wicketd v0.1.0 (/home/dap/omicron-review/wicketd)
│   ├── wicketd-api v0.1.0 (/home/dap/omicron-review/wicketd-api)
│   │   ├── omicron-dropshot-apis v0.1.0 (/home/dap/omicron-review/dev-tools/dropshot-apis)
│   │   └── wicketd v0.1.0 (/home/dap/omicron-review/wicketd)
│   └── wicketd-client v0.1.0 (/home/dap/omicron-review/clients/wicketd-client)
│       ├── wicket v0.1.0 (/home/dap/omicron-review/wicket) (*)
│       └── wicketd v0.1.0 (/home/dap/omicron-review/wicketd)
└── wicketd v0.1.0 (/home/dap/omicron-review/wicketd)

$ cargo tree -e normal -i git+https://github.com/oxidecomputer/dendrite?rev=cc8e02a0800034c431c8cf96b889ea638da3d194#dpd-client@0.1.0
dpd-client v0.1.0 (https://github.com/oxidecomputer/dendrite?rev=cc8e02a0800034c431c8cf96b889ea638da3d194#cc8e02a0)
└── omicron-sled-agent v0.1.0 (/home/dap/omicron-review/sled-agent)
    ├── end-to-end-tests v0.1.0 (/home/dap/omicron-review/end-to-end-tests)
    └── nexus-test-utils v0.1.0 (/home/dap/omicron-review/nexus/test-utils)
        └── omicron-dev v0.1.0 (/home/dap/omicron-review/dev-tools/omicron-dev)

This is almost certainly not great. But it shouldn't cause ls-apis to break. Right now if this happens, ls-apis picks one of these arbitrarily, which can cause it to analyze the wrong version of our software and draw wrong conclusions. This is the real cause of #10214.

Again: we want ls-apis to be looking at the version of these things that gets deployed. How can it know which one it is? The authoritative version is the one in package-manifest.toml. Hence the solution here: parse that file, find the commit being used there, and choose the version of the package that corresponds to that commit.

Other notes

This is still a little cheesy in a few ways:

to determine if it's the right commit, we do a string comparison on the "source", which is basically a git URL
it's still using this sort of goofy heuristic (find a known package referenced by Omicron but contained in the other repo) -- just to be able to re-use Cargo's clone of the repo. It's pretty nice to not have to manage a separate set of clones (and make sure they're correct, have no local changes, etc.), but it's kind of a weird assumption to make and it would break if we ever had a repo we cared about where Omicron doesn't reference one of its packages.

but I think it's a meaningful improvement.

One other note: this will break in the future if:

Omicron has no reference at all to a package in the other repo (the case above -- this would already have broken before)
Omicron has no reference to a package in the other repo at the same Git commit as the one in package-manifest.toml. This would be unlikely, since we usually move all of our deps forward at the same time. But there's a notable exception today: one of the dpd-client paths above is deliberately fixed to an old version for upgrade-related reasons. This works out fine though because there's another reference to dpd-client that is the right version.
package-manifest.toml references the same repo multiple times with different commits. Again, this seems unlikely, and the problem is deeper than it looks. That means we have two different versions of something deployed and ls-apis needs to analyze both?

…of related repos to use

davepacheco · 2026-04-03T22:31:34Z

            Arc::into_inner(omicron).expect("no more Omicron Arc references"),
        );

-        // To load Dendrite, we need to look something up in Maghemite (loaded


This appears to have been totally superfluous after #7907, which added "dendrite" to the block above instead.

davepacheco · 2026-05-06T00:37:20Z

-[[intra_deployment_unit_only_edges]]
-server = "lldpd"
-client = "gateway-client"
-note = """
-lldpd defaults to localhost for gateway (main.rs:194), and the SMF start
-script doesn't override it.
-"""
-permalinks = [
-    "https://github.com/oxidecomputer/lldp/blob/d22509dfdb051321b859e924948605115691b93c/lldpd/src/main.rs#L148-L154",
-    "https://github.com/oxidecomputer/lldp/blob/d22509dfdb051321b859e924948605115691b93c/lldpd/misc/svc-lldpd",
-]
-


Interestingly, I added this block as part of the PR that introduced IDU-only metadata, specifically as a result of a merge:
#9707 (comment)

What I think happened here is that:

When I started working on ls-apis needs to detect cycles in dependency unit graph #9707, lldpd didn't depend on MGS.

While working on it, enable lldp to be aware of what switch it is managing lldp#41 landed in the lldp repo that added a dependency from lldpd on MGS. This happened around February 26.

There was no immediate impact on Omicron:

To this day, package-manifest.toml in Omicron points at an lldp commit from October, 2025.

lldpd-client is a different story. As described in pin lldp client #10361, Omicron's Cargo.toml only refers to lldpd-client coming from the lldp repo's main branch, without a specific commit. However, at this point, Cargo.lock remained pinned to an earlier commit.

Around March 2, pull in dendrite PR 220 #9898 landed, which updated Omicron's Cargo.lock so that lldpd-client now came from the lldp commit where lldpd has a dependency on MGS. package-manifest.toml was not updated. This is basically what introduced the API version mismatch that resulted in pin lldp client #10361.

When I sync'd up with that change in ls-apis needs to detect cycles in dependency unit graph #9707, I dug into this dependency, looked at lldpd main, and added this block to the API manifest. I didn't notice the mismatch within Omicron (which is an argument for this PR). I believe this block is actually correct -- it just doesn't belong on Omicron main yet. It will belong here once we update lldp in package-manifest.toml.

In summary: due to a combination of #10361 and the ls-apis bug that I'm fixing here, ls-apis prematurely picked up the lldp -> MGS dependency and I prematurely added this block. Fixing this bug, ls-apis no longer identifies this dependency, and the rule has to go because it's now superfluous.

Re-added this block because LLDP has been updated in package-manifest.toml now. This PR now has no changes to this file.

davepacheco · 2026-05-06T00:38:43Z

 live-tests-macros = { path = "live-tests/macros" }
-lldpd_client = { git = "https://github.com/oxidecomputer/lldp", package = "lldpd-client" }
-lldp_protocol = { git = "https://github.com/oxidecomputer/lldp", package = "protocol" }
+lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "61479b6922f9112fbe1e722414d2b8055212cb12", package = "lldpd-client" }


This is basically rolling back lldpd-client, but I believe it's correct. See #10361.

This is no longer a rollback because I updated to match LLDP in package-manifest.toml on main.

sunshowers

Thanks for doing this -- overall, looks great. Just have a few questions and comments.

sunshowers · 2026-05-11T21:01:10Z

+fn find_repo_commit(
+    package_manifest: &omicron_zone_package::config::Config,
+    repo_name: &str,
+) -> Result<String> {


Worth a newtype around a string for a commit hash?

Done in ebdf01f

sunshowers · 2026-05-11T21:18:28Z

+        // This is cheesy, but it works okay for now and fails safely.
+        if source.repr.contains(expected_commit) {
+            found_pkg = Some(pkginfo);
+            break;
+        }


Heh, guppy would be able to handle this reliably using ExternalSource, but I think this is okay for now. Thoughts on matching against a more precise ends_with("#<hash>"), though? Or maybe using rsplit_once('#')?

Fixed in a9d15dd

sunshowers · 2026-05-11T21:22:06Z

+        eprintln!(
+            "warn: looking up {pkgid:?}: looking for git commit \
+             {expected_commit} (based on package-manifest.toml), found \
+             source {source:?}"
+        );
+        eprintln!(
+            "If another version of package {pkgname:?} is found corresponding \
+             with this commit, then it may be suspicious to have multiple version \
+             of this package, but it will not break this tool."
+        );
+        eprintln!(
+            "If not, there's a mismatch between commits in package-manifest.toml \
+             and Cargo.toml or there is a bug in this tool."
+        );


Under what circumstances would this be hit? I'm specifically wondering if you're going to hit this if you have a [patch] section in your local copy of the workspace Cargo.toml.

I was a little bit concerned the eprintln!s are nondeterministic depending on iteration order, but it looks like we consistently use BTreeMaps internally so that isn't an issue. But I'm still concerned that we're dependent on lexicographic order of keys here, so that if the matching key is first we don't print out this warning, while if the matching key comes later, we do. Maybe this isn't a huge deal though, if the cases where we'll hit this are rare? What do you think?

Under what circumstances would this be hit? I'm specifically wondering if you're going to hit this if you have a [patch] section in your local copy of the workspace Cargo.toml.

I don't think so, based on the testing I did here: #10220 (comment)

I was a little bit concerned the eprintln!s are nondeterministic depending on iteration order, but it looks like we consistently use BTreeMaps internally so that isn't an issue. But I'm still concerned that we're dependent on lexicographic order of keys here, so that if the matching key is first we don't print out this warning, while if the matching key comes later, we do. Maybe this isn't a huge deal though, if the cases where we'll hit this are rare? What do you think?

This doesn't seem like a big deal to me. The case where this comes up is the dpd-client one, where we wind up pulling in multiple distinct versions through different paths. Admittedly, the warning is aimed at helping debug a situation where we don't find any package with the right commit: you'll have messages for each wrong one we found. I'll see how hard it would be to clean up the UX here.

I think I've improved this in e340010. Let me know what you think.

sunshowers · 2026-05-11T21:24:33Z

+        let Some(source) = &pkginfo.source else {
+            eprintln!(
+                "warn: looking up {pkgid:?}: unexpectedly found source `None`"
+            );
+            continue;
+        };


I believe, going by this reference, that source: None means either a path dependency or a workspace package. But that does mean that (I believe) we'll run into this if you [patch] one of the git dependencies here with a path dependency. Definitely worth testing, at least.

What would we want to happen in that case?

I'm not sure tbh -- but I assume people do patch in some dependencies while working locally. Probably worth doing something reasonable there (or failing if that isn't possible).

Yes, you do. I tested this by patching Cargo.toml like this:

--- a/Cargo.toml +++ b/Cargo.toml @@ -1146,3 +1146,6 @@ path = "workspace-hack" [patch."https://github.com/oxidecomputer/omicron"] omicron-uuid-kinds = { path = "uuid-kinds" } omicron-common = { path = "common" } + +[patch."https://github.com/oxidecomputer/dendrite"] +dpd-client = { path = "/home/dap/dendrite/dpd-client" }

The result is the same whether /home/dap/dendrite points at the correct commit or not:

$ cargo xtask ls-apis apis Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.40s Running `target/debug/xtask ls-apis apis` Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.67s Running `target/debug/ls-apis apis` loading metadata for workspace omicron from current workspace warn: looking up PackageId { repr: "path+file:///home/dap/dendrite/dpd-client#0.1.0" }: unexpectedly found source `None` loading metadata for workspace lldp from /home/dap/.cargo/git/checkouts/lldp-d47de417041f191b/54b2661/Cargo.toml loading metadata for workspace maghemite from /home/dap/.cargo/git/checkouts/maghemite-c0236f0fd3d582b6/dc84a6c/Cargo.toml loading metadata for workspace crucible from /home/dap/.cargo/git/checkouts/crucible-0a48bd218bc2bbbc/2bfe090/Cargo.toml loading metadata for workspace propolis from /home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/979b728/Cargo.toml Error: found 1 error while loading API metadata: - loading Cargo workspace metadata caused by: found no versions of package "dpd-client" matching the git commit found in package-manifest.toml (cc0c307c617f2988aafdca4e3bd35ea178b64801)

Any suggestions? I don't immediately see a great way to handle this case. The whole idea is to make sure that the Git SHA matches up between package-manifest.toml and the client package, but that presupposes that the client package is specified by Git SHA -- otherwise, in general, I don't think we have a way to know what SHA the package corresponds to.

One option would be to assume that you're patching and simply assume it matches. That would let the rest of the tool continue to work if you'd done things correctly, but it's easy to get wrong and I don't like the way things fail in that case.

Another option would be to fail explicitly if we found no commit SHA match and one of them had source: None. It would look almost the same as above except that it would append something like ("note: a package was found that appears to be a local patch; this tool does not support configurations where client packages are patched."). We could do this and support an override flag, if we think it's likely people might locally patch a client package that comes from another repo and need to use this tool at the same time.

Another option would be to fail explicitly if we found no commit SHA match and one of them had source: None. It would look almost the same as above except that it would append something like ("note: a package was found that appears to be a local patch; this tool does not support configurations where client packages are patched."). We could do this and support an override flag, if we think it's likely people might locally patch a client package that comes from another repo and need to use this tool at the same time.

This seems fine to me, given that path dependencies ultimately can't make it to main. (You can also [patch] a dependency with a Git dep, but I think the tool will correctly detect that as a mismatched hash?)

I went ahead with this last option:

always warn when we run into this

if we find some other candidate with the right SHA, we simply use that

if we don't, by default, we fail with an error message that explains what probably happened (you've got a local patch that we can't verify)

there's a flag --assume-patched-deps-match that, in this specific case, proceeds instead of failing

Now with the local patch, by default I get this:

dap@ivanova omicron-fix $ cargo xtask ls-apis apis Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.45s Running `target/debug/xtask ls-apis apis` Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.81s Running `target/debug/ls-apis apis` loading metadata for workspace omicron from current workspace warn: looking up PackageId { repr: "path+file:///claude/dendrite/dpd-client#0.1.0" }: found package with no source, which usually means it has been overridden with a Cargo [patch] pointing at a local path. This tool cannot verify that this checkout matches the commit pinned in package-manifest.toml (cc0c307c617f2988aafdca4e3bd35ea178b64801). loading metadata for workspace lldp from /home/dap/.cargo/git/checkouts/lldp-d47de417041f191b/54b2661/Cargo.toml loading metadata for workspace propolis from /home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/979b728/Cargo.toml loading metadata for workspace crucible from /home/dap/.cargo/git/checkouts/crucible-0a48bd218bc2bbbc/2bfe090/Cargo.toml loading metadata for workspace maghemite from /home/dap/.cargo/git/checkouts/maghemite-c0236f0fd3d582b6/dc84a6c/Cargo.toml Error: found 1 error while loading API metadata: - loading Cargo workspace metadata caused by: found no versions of package "dpd-client" matching the git commit found in package-manifest.toml (cc0c307c617f2988aafdca4e3bd35ea178b64801). A candidate with no source (likely a local Cargo [patch]) was found, but this tool cannot verify that it matches. If you believe the patched version is correct, re-run with --assume-patched-deps-match. dap@ivanova omicron-fix $ echo $? 1

and with the override:

dap@ivanova omicron-fix $ cargo xtask ls-apis --assume-patched-deps-match apis Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.49s Running `target/debug/xtask ls-apis --assume-patched-deps-match apis` Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.80s Running `target/debug/ls-apis --assume-patched-deps-match apis` loading metadata for workspace omicron from current workspace warn: looking up PackageId { repr: "path+file:///claude/dendrite/dpd-client#0.1.0" }: found package with no source, which usually means it has been overridden with a Cargo [patch] pointing at a local path. This tool cannot verify that this checkout matches the commit pinned in package-manifest.toml (cc0c307c617f2988aafdca4e3bd35ea178b64801). note: package "dpd-client": no candidate matched the commit pinned in package-manifest.toml (cc0c307c617f2988aafdca4e3bd35ea178b64801), but a candidate with no source (likely a local [patch]) was found. Proceeding with that candidate because --assume-patched-deps-match was given. loading metadata for workspace lldp from /home/dap/.cargo/git/checkouts/lldp-d47de417041f191b/54b2661/Cargo.toml loading metadata for workspace propolis from /home/dap/.cargo/git/checkouts/propolis-d68c8bd1bc59c9bd/979b728/Cargo.toml loading metadata for workspace dendrite from /claude/dendrite/Cargo.toml loading metadata for workspace crucible from /home/dap/.cargo/git/checkouts/crucible-0a48bd218bc2bbbc/2bfe090/Cargo.toml loading metadata for workspace maghemite from /home/dap/.cargo/git/checkouts/maghemite-c0236f0fd3d582b6/dc84a6c/Cargo.toml note: ignoring Cargo dependency from crucible-pantry -> ... -> crucible-control-client note: ignoring Cargo dependency from omicron-sled-agent -> dns-server Bootstrap Agent (client: bootstrap-agent-client) consumed by: omicron-sled-agent (omicron/sled-agent) via 1 path consumed by: wicketd (omicron/wicketd) via 1 path Bootstrap Agent Lockstep API (client: bootstrap-agent-lockstep-client) consumed by: wicketd (omicron/wicketd) via 2 paths ... dap@ivanova omicron-fix $ echo $? 0

This is implemented in 57f4fde. Let me know what you think.

sunshowers · 2026-05-11T21:38:22Z

Does this comment need to be updated? I think "we have to respect the dependencies" is slightly out of date now thanks to the "To load Dendrite, we need to look something up in Maghemite" block you removed below.

Yeah, good catch. Updated in d5e6e91.

sunshowers · 2026-05-11T21:40:08Z

+        let handles: Vec<_> = RELATED_REPOS
+            .iter()
+            .map(|repo_config| {
+                let RelatedRepoConfig {
+                    repo_name,
+                    expected_pkg_name,
+                    extra_cargo_features,
+                } = repo_config;
+                let mine = omicron.clone();
+                let my_ignored = ignored_non_clients.clone();
+                // unwrap(): we loaded a commit for each repo in the loop above
+                let expected_commit =
+                    (*related_repo_commits.get(repo_name).unwrap()).clone();


nit: related_repo_commits could also store the &RelatedRepoConfig, and then you could iterate on that rather than doing a map lookup.

Fixed in 59d201d.

sunshowers · 2026-05-11T21:41:27Z

+    // It's possible to have more than one non-workspace package with a given
+    // name.  For example, Omicron references `dpd-client` in multiple ways:
+    // from Nexus and through lldpd-client.  So which version do we want?  Well,


I found the example here to be a little confusing. I think what this is trying to say is that dpd-client can get pulled in in multiple different ways, through multiple different git dependencies (correct me if I'm wrong?)

Yeah. I suspect what might be confusing is that usually we'd say that dpd-client is getting pulled in via different paths, and so we might have different versions of the one package called dpd-client. But in this code, those are represented as distinct "packages" that happen to have the same name (dpd-client).

I'm not sure how to make this clearer, but if you want to suggest an edit to the comment I'll be happy to take it.

sunshowers · 2026-05-11T21:41:43Z

+        );
+        eprintln!(
+            "If another version of package {pkgname:?} is found corresponding \
+             with this commit, then it may be suspicious to have multiple version \


nit:

Suggested change

with this commit, then it may be suspicious to have multiple version \

with this commit, then it may be suspicious to have multiple versions \

Thanks -- fixed in 4e80668.

sunshowers · 2026-05-11T21:43:52Z

+            extra_cargo_features: Some(CargoOpt::SomeFeatures(vec![
+                String::from("omicron-build"),
+            ])),


nit, take it or leave it: if you switch this to be a &'static [&'static str] you could avoid LazyLock

I like this a lot! Done in c6848cf.

davepacheco · 2026-07-09T20:28:42Z

Since I last looked at this, LLDP was updated in package-manifest.toml on main. As a result, I re-added the block to api-manifest.toml (this PR now has no changes for that) and updated the lldpd pins in Cargo.toml.

davepacheco · 2026-07-15T00:09:35Z

Thanks @sunshowers! I believe this is ready for re-review.

sunshowers

thanks!

sunshowers · 2026-07-15T21:44:55Z

+/// How the tool should treat a related-repo dependency whose resolved package
+/// has no source (typically because the developer has overridden it with a
+/// local Cargo `[patch]`).
+///
+/// When a dependency has been patched to a local path, there is no reliable
+/// way for this tool to check that the local copy corresponds to the commit
+/// pinned in `package-manifest.toml`.
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum PatchedDepPolicy {
+    /// Report an error if the only candidate for a related-repo dependency
+    /// looks like a local `[patch]` override.
+    Reject,
+    /// Assume that a local `[patch]` override corresponds to the commit
+    /// pinned in `package-manifest.toml` and use it.
+    AssumeMatch,
 }


I'd maybe be clearer here that this policy is only consulted when at both of the following conditions are true:

No resolved package matches the pinned commit in the Cargo.toml.

There is at least one resolved package without a source.

Otherwise "the only candidate" is slightly misleading.

Clarified in 9270b26.

sunshowers · 2026-07-15T21:46:40Z

+lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "lldpd-client" }
+lldp_protocol = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "protocol" }


👀

do we need to update the tools/update_lldp.sh script?

Filed #10831

sunshowers · 2026-07-15T21:48:42Z

+            if !mismatched.is_empty() {
+                eprintln!(
+                    "note: found the following versions of package \
+                     {pkgname:?}, none of which matched the git commit in \
+                     package-manifest.toml ({expected_commit}):"
+                );
+                for (pkgid, source) in &mismatched {
+                    eprintln!("  - {pkgid:?}: {source:?}");
+                }
+                eprintln!(
+                    "There may be a mismatch between commits in \
+                     package-manifest.toml and Cargo.toml or a bug in this \
+                     tool."
+                );
+            }
+            bail!(
+                "found no versions of package {pkgname:?} matching the git \
+                 commit found in package-manifest.toml ({expected_commit})",
+            );


nit: do we want the mismatched-not-empty and mismatched-empty cases to bail with different messages?

I'm going to leave this as-is. I think the output is pretty clear.

sunshowers · 2026-07-15T21:51:20Z

+    /// Assume that any related-repo dependency that has been overridden by a
+    /// local Cargo `[patch]` corresponds to the commit pinned in
+    /// `package-manifest.toml`.  Without this flag, the tool will report an
+    /// error if it encounters such a dependency and cannot find a match by
+    /// commit.
+    #[arg(long)]
+    assume_patched_deps_match: bool,


one worry I have is that a local [patch] will break a full cargo nextest run. What do you think of adding a clap env override for this?

I'm on the fence about this. I can see wanting to do anything with a local patch that you can do without it. I'm also a little worried this env var will sneak into CI and defeat the check altogether. I guess that's not a good reason not to have it.

Done in 9760484.

sunshowers · 2026-07-15T21:56:43Z

+        // Next, load the top level package manifest.  This will tell us for
+        // each related component (like Crucible, Maghemite, etc.), which commit
+        // of that component's repo Omicron actually deploys to running systems.


api-manifest.toml has a bunch of TODOs at the top of the file that can now be updated:

# TODO It would be nice to collect a bunch of this information from the same # sources that drive the actual build process (e.g., package-manifest.toml). # For non-Omicron components, the deployment units (zone images and tarballs # that get unpacked into the switch zone or global zone) come from buildomat # jobs on other repositories. In at least some components those come from those # components' package-manifest.toml files, which we probably have available, so # we could still incorporate that information.

I think this has now been addressed?

# TODO The following items from package-manifest.toml are currently ignored # because they are assumed not to contain servers or clients that we care about: # # - faux_mgs

faux_mgs is part of the mgs repo, right?

# If we do wind up processing package-manifest.toml, we may want to maintain an # explicit list of items that we ignore so that we can fail when something is # neither included nor ignored so that we can be sure we're not missing # anything.

This seems like the MANIFEST_PREBUILT_REPOS?

api-manifest.toml has a bunch of TODOs at the top of the file that can now be updated:

# TODO It would be nice to collect a bunch of this information from the same # sources that drive the actual build process (e.g., package-manifest.toml). # For non-Omicron components, the deployment units (zone images and tarballs # that get unpacked into the switch zone or global zone) come from buildomat # jobs on other repositories. In at least some components those come from those # components' package-manifest.toml files, which we probably have available, so # we could still incorporate that information.

I think this has now been addressed?

This is definitely better, but there's more information we could get from package-manifest.toml that we're not, like what the deployment units are and what servers are contained in them.

# TODO The following items from package-manifest.toml are currently ignored # because they are assumed not to contain servers or clients that we care about: # # - faux_mgs

faux_mgs is part of the mgs repo, right?

I'm going to strike the comment because I'm not sure what it could be talking about any more. package-manifest does not mention crucible_dtrace explicitly. faux_mgs and mg-ddm are in repos that we now inspect, yes, but I'm not sure they're encoded in deployment units that the tool will analyze (and I don't think that's worth noticing here).

# If we do wind up processing package-manifest.toml, we may want to maintain an # explicit list of items that we ignore so that we can fail when something is # neither included nor ignored so that we can be sure we're not missing # anything.

This seems like the MANIFEST_PREBUILT_REPOS?

Good catch!

Done in 29363a0.

sunshowers · 2026-07-15T21:57:22Z

+    PrebuiltRepoConfig {
+        repo_name: "management-gateway-service",
+        behavior: PrebuiltRepoBehavior::Inspect {
+            expected_pkg_name: "gateway-messages",
+            extra_cargo_features: &[],
+        },
+    },


do we have a script to update all the MGS hashes atomically, like update_dendrite.sh etc? worth adding one in a followup?

It doesn't look like we have one. I can file an issue? I don't really know the standard practice around any of the updater scripts or how we do it with MGS.

Yeah I'd file an issue for this.

sunshowers · 2026-07-15T21:57:57Z

+/// must appear here.  The reverse must also hold: any entry here must
+/// correspond to a `prebuilt` package in `package-manifest.toml`.  Both
+/// directions are checked at runtime.
+static MANIFEST_PREBUILT_REPOS: [PrebuiltRepoConfig; 8] = [


super duper nit: this can be &[PrebuiltRepoConfig] since the number of elements itself isn't part of the stable contract.

Fixed in c6314e6.

sunshowers · 2026-07-15T21:58:48Z

+        // This implementation is cheesy, but it works okay for now and fails
+        // safely.


I'd consider the implementation to no longer be cheesy ;)

It's definitely better! But it's still relying on the way the dep is encoded to figure out the SHA. That's probably as good as we can do but I don't love it.

sunshowers · 2026-07-15T22:06:41Z

nit: we use crucible-agent-client -- can also drop the "might" since that's how we actually do this.

Fixed in c6314e6.

davepacheco added 4 commits April 3, 2026 13:26

retire falcon_runner

c1e92fa

hakari

2c14397

fix test

b2cbeb7

ls-apis should use package-manifest.toml to figure out which version …

eadc3de

…of related repos to use

davepacheco requested a review from sunshowers April 3, 2026 22:30

davepacheco commented Apr 3, 2026

View reviewed changes

Base automatically changed from dap/rm-falcon-runner to main April 3, 2026 23:09

davepacheco mentioned this pull request May 5, 2026

pin lldp client #10361

Closed

davepacheco added 5 commits May 5, 2026 16:31

Merge commit 'f232730' into dap/ls-apis-slightly-less-brittle

ce3ad7c

Merge branch 'main' into dap/ls-apis-slightly-less-brittle

1a3ba68

do not update lldpd

a234e1f

Cargo updates

a7d33d0

remove prescient IDU-only edge from API manifest

d9551de

davepacheco commented May 6, 2026

View reviewed changes

davepacheco marked this pull request as ready for review May 6, 2026 00:38

fix test

7419d2c

sunshowers reviewed May 11, 2026

View reviewed changes

davepacheco added 5 commits July 9, 2026 11:54

Merge branch 'main' into dap/ls-apis-slightly-less-brittle

29adec8

Now that lldp has been updated, this block needs to be re-added.

e17ea7d

fix test

2d57084

hakari

d76bf99

clippy

b1681d4

davepacheco added 6 commits July 9, 2026 14:45

use newtype for GitCommit

ebdf01f

use more precise commit hash check

a9d15dd

clarify error message

556603a

update README

bb2f3f4

add unit tests for find_repo_commit

a5dc868

update old comment

d5e6e91

davepacheco added 9 commits July 9, 2026 16:48

error message typo

4e80668

bye bye, lazylock

c6848cf

avoid unwrap

59d201d

fix long line

73ecb02

better handle patched deps

57f4fde

dont spam so many useless warnings

e340010

match up package manifest with static constants

da0122d

check MGS too

40f00a2

Merge branch 'main' into dap/ls-apis-slightly-less-brittle

0a8b8c9

sunshowers approved these changes Jul 15, 2026

View reviewed changes

clarify PatchedDepPolicy docs

9270b26

davepacheco mentioned this pull request Jul 15, 2026

should tools/update_lldp.sh update lldpd_client, lldpd_protocol crates? #10831

Open

davepacheco added 3 commits July 15, 2026 15:37

add env var

9760484

clean up comments

29363a0

small fixes

c6314e6

davepacheco enabled auto-merge (squash) July 15, 2026 22:53

davepacheco mentioned this pull request Jul 16, 2026

do we need an update_mgs helper script? #10834

Closed

davepacheco merged commit 3f80aca into main Jul 16, 2026
20 checks passed

davepacheco deleted the dap/ls-apis-slightly-less-brittle branch July 16, 2026 11:13

	with this commit, then it may be suspicious to have multiple version \
	with this commit, then it may be suspicious to have multiple versions \

		lldpd_client = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "lldpd-client" }
		lldp_protocol = { git = "https://github.com/oxidecomputer/lldp", rev = "54b266174d4de9628bca9c97b0db176e16f12154", package = "protocol" }

		// This implementation is cheesy, but it works okay for now and fails
		// safely.

Uh oh!

Conversation

davepacheco commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Other notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunshowers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunshowers May 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepacheco commented Jul 9, 2026

Uh oh!

davepacheco commented Jul 15, 2026

Uh oh!

sunshowers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

davepacheco commented Apr 3, 2026 •

edited

Loading

sunshowers May 12, 2026 •

edited

Loading