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-rustls, s2n-quic-tls): enable building with default features #1673

Merged
merged 4 commits into from
Mar 22, 2023

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Mar 22, 2023

Description of changes:

  • add alloc dependency to rustls and tls crates
  • add default dependency to h3 crate
  • add CI step to check that each crate builds.

Note that the name of this PR is confusing. "default" features is literally referring to "the set of features included when build is invoked directly on the package", and not necessarily cargo's concept of "default-features".

Currently we are unable to release to crates.io because s2n-quic-rustls and s2n-quic-tls do not build with default features. This happened because in a #1633 , components that s2n-quic-rustls depends on were moved behind the alloc feature flag.

We failed to detect this in our CI because the workspace build builds from s2n-quic with default features enabled. This build happens successfully because the alloc feature is included in default.

An issue that seems to describe in a bit more detail is linked here: rust-lang/cargo#8366

Call-outs:

I'm still a bit uncertain on our overall story for no-std usage. It might be good to add an example showing an intended use case.

Why not just dry run publishing/packaging?

Because that relies on artifacts being published to crates.io

For example consider that we just version bumped things so that for crate A which consumes B, we now have
A = 1.0.0 depends on B = 1.0.0.

  • dry run publish B = 1.0.0 succeeds
  • dry run publish A = 1.0.0 fails because B 1.0.0 can't be found on crates (because it wasn't actually published)
    The same problem applies to the cargo package workflow.

Testing:

Tested by invoking the individual package builds. Also adding a step for that to the CI. The list of packages that are will be dynamically updated.

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

@jmayclin jmayclin marked this pull request as ready for review March 22, 2023 01:46
@@ -16,6 +16,7 @@ env:
# Pin the nightly toolchain to prevent breakage.
# This should be occasionally updated.
RUST_NIGHTLY_TOOLCHAIN: nightly-2023-02-20
MSRV: 1.63
Copy link
Contributor

Choose a reason for hiding this comment

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

We seem to already have a msrv that we parse from the rust-toolchain below in env. Do we also need 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.

Good call out, will update.

@toidiu
Copy link
Contributor

toidiu commented Mar 22, 2023

Why not just dry run publishing/packaging?

This also fails on crates that are marked publish = false

@jmayclin
Copy link
Contributor Author

I opened a draft PR with just the workflow changes to make sure that it breaks on our current state, and the task was failed successfully #1676. The new workflow failed on h3, tls, and rustls as expected.

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