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): pin jobserver in more places and run cargo package in CI #2009

Merged
merged 11 commits into from
Oct 18, 2023

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented Oct 17, 2023

Description of changes:

When publishing the s2n-quic-tls-default and s2n-quic crates to crates.io, the pinned jobserver dev-dependency in s2n-quic-tls was not respected, and the verification step failed. This change adds additional dev-dependency pins of jobserver to the s2n-quic-tls-default and s2n-quic crates.

I've also modified the crates CI task that builds each crate individually to use the cargo package command, as that more closely replicates what happens during a cargo publish. This CI task now only runs on crates that we publish (as non-published crates use path definitions that are not valid within the cargo package command). In addition, the CI task will use cargo build if the PR is bumping the s2n-quic version number, as cargo package would not be able to find unpublished dependencies in crates.io

Testing:

Tested locally and in new CI action

Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed? -->

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

@WesleyRosenblum WesleyRosenblum marked this pull request as draft October 17, 2023 19:05
@WesleyRosenblum WesleyRosenblum changed the title fix(s2n-quic): pin jobserver in more places fix(s2n-quic): pin jobserver in more places and run cargo package in CI Oct 18, 2023
@WesleyRosenblum WesleyRosenblum marked this pull request as ready for review October 18, 2023 00:53
@@ -414,11 +420,19 @@ jobs:
override: true

- name: build
if: ${{ needs.env.outputs.new-release == 'true' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just run this on all PR's rather than only running it on release PRs? I know that they are similar tasks, but the extra coverage could be nice.

Copy link
Contributor Author

@WesleyRosenblum WesleyRosenblum Oct 18, 2023

Choose a reason for hiding this comment

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

The way I see it is the build that cargo package performs is the same as cargo build but even more strict (as in likely to fail). And given that this step was added to catch issues with publishing, and cargo package is very closely tied to what cargo publish does, it seems like running cargo package would catch anything cargo build does? Would you be fine with having just the single command for now, and we can revisit if we encounter some edge case where cargo build fails but cargo package succeeds?

Copy link
Contributor

Choose a reason for hiding this comment

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

the build that cargo package performs is the same as cargo build but even more strict

I agree with that interpretation, but I think my lesson from this is that I really don't understand the differences 😅

In which case it's nice to have to two different build steps, but anything that slips through should be very visible, so I'm not too worried about it.

quic/s2n-quic-tls-default/Cargo.toml Show resolved Hide resolved
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