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

move dpd-client dependency out of omicron common #5738

Merged
merged 2 commits into from May 13, 2024

Conversation

rcgoodfellow
Copy link
Contributor

This breaks downstreams that import omicron_common as the way dpd-client gets built is specific to the omicron build machinery.

For example, this happens when trying to bump omicron in maghemite.

error: failed to run custom build command for `dpd-client v0.1.0 (https://github.com/oxidecomputer/omicron?branch=main#c472464a)`

Caused by:
  process didn't exit successfully: `/Users/ry/src/maghemite/target/debug/build/dpd-client-8e2b35d52fea36a5/build-script-build` (exit status: 1)
  --- stdout
  cargo:rerun-if-changed=../../package-manifest.toml

  --- stderr
  Error: ../../out/downloads/dpd-3b84ea6516cafb4595a6f2a668df16c1a501b687.json doesn't exist; rerun `tools/ci_download_dendrite_openapi` (after updating `tools/dendrite_openapi_version` if the dendrite commit in package-manifest.toml has changed)

/// Opaque object representing link state. The contents of this object are not
/// yet stable.
#[derive(Clone, Debug, Deserialize, Serialize)]
pub struct SwitchLinkState {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bulk of this PR is moving this type to nexus/types/src/external_api/shared.rs

This breaks downstreams that import omicron_common as the way dpd-client
gets built is specific to the omicron build machinery.
oximeter imports this and oximeter is imported by downstreams
@@ -27,7 +27,6 @@ http.workspace = true
ipnetwork.workspace = true
macaddr.workspace = true
mg-admin-client.workspace = true
dpd-client.workspace = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This causes downstream breakage, as compilation of this crate requires omicron-specific build machinery.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah... I feel like CI should catch this, maybe by building a stub project with omicron-common as its only dependency. Could you file an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

thanks!

@@ -27,7 +27,6 @@ http.workspace = true
ipnetwork.workspace = true
macaddr.workspace = true
mg-admin-client.workspace = true
dpd-client.workspace = true
Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah... I feel like CI should catch this, maybe by building a stub project with omicron-common as its only dependency. Could you file an issue for this?

@rcgoodfellow rcgoodfellow merged commit 23688c8 into main May 13, 2024
21 checks passed
@rcgoodfellow rcgoodfellow deleted the no-dpd-client-in-common branch May 13, 2024 21:31
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.

None yet

2 participants