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

fix(s2n-quic-xdp): correct tokio feature dependency #1808

Merged
merged 6 commits into from
Jun 13, 2023

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Jun 12, 2023

Description of changes:

We are currently unable to publish to crates.io because s2n-quic-xdp can't be published. This is because s2n-quic-xdp depends on the net feature, but we forgot to specify that in the Cargo.toml.

Previously the s2n-quic-xdp crate was using the default feature resolver, which unified features across normal and dev dependencies, so the build pulled in full tokio features because tokio-full is a dev dependency.

Publishing crates uses a different feature resolver that doesn't pull in dev-dependencies, so the build failed because our build had a dependency on the net feature of tokio that wasn't fully modeled in the Cargo.toml

This PR

  • adds explicity dependency on the tokio net feature.
  • adds the s2n-quic-xdp crate to the package build check
  • switches the tools workspace to use the non-unifying feature resolver (so this failure mode is now covered by CI)

Edit: The section previously talked about how it would be difficult to write a check for this with cargo publish --dry-run but the feature-resolver = 2 should allow us to correctly test for this failure in CI.

Testing:

This was verified with the following command

cargo package --manifest-path tools/xdp/s2n-quic-xdp/Cargo.toml

This failed before the change, and now succeeds.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft
Copy link
Contributor

camshaft commented Jun 12, 2023

For that workflow we should probably be doing a cargo publish --dry-run to avoid any workspace-specific configuration. The trick is, like you mention, is when we bump the version and it can't resolve our deps.

@jmayclin
Copy link
Contributor Author

Here are some issues with context

rust-lang/cargo#1796

@jmayclin jmayclin marked this pull request as ready for review June 13, 2023 00:00
camshaft
camshaft previously approved these changes Jun 13, 2023
@jmayclin jmayclin requested a review from camshaft June 13, 2023 03:08
@jmayclin jmayclin merged commit b281ceb into aws:main Jun 13, 2023
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.

2 participants