chore(bindings): release #4388
chore(bindings): release #4388
Conversation
| tokio = { version = "1", features = ["net", "time"] } | ||
|
|
||
| [build-dependencies] | ||
| home = "=0.5.5" # a transitive dependency of aws-lc-sys. newer versions require rust 1.70 | ||
| regex = "=1.9.6" # a transitive dependency of aws-lc-sys. newer versions require rust 1.65, see https://github.com/aws/s2n-tls/issues/4242 |
There was a problem hiding this comment.
This doesn't feel like the right approach to me. Pinning transitive(!) dependencies like this is forcing the costs of lower MSRV on all downstream consumers, rather than letting them deal with it as they see fit (e.g., patching these crates to a lower version, keeping an older crates.io index as-of that version, etc.) -- and for consumers that are happy to use latest Rust, this forces them to use stale or many versions of your dependencies.
There was a problem hiding this comment.
(It obviously doesn't need to block this PR, since it seems like a pre-existing decision, just wanted to leave a comment)
There was a problem hiding this comment.
I started investigating this on other platforms and I dont see an issue when building this on linux. Converting this to draft while I finish investigating
There was a problem hiding this comment.
aws-lc-rs was behaving differently on Mac vs Linux because of bindgen build logic on those platforms. aws-lc has a PR to to pin bindgen deps and test it in CI.
The behavior I was seeing:
Mac:
- new git clone: fails ‘cargo build’
- new git clone: fails ‘cargo publish --dry-run’
Linux:
- new git clone: success ‘cargo build’
- new git clone: success ‘cargo publish --dry-run’
There was a problem hiding this comment.
This doesn't feel like the right approach to me. Pinning transitive(!) dependencies like this is forcing the costs of lower MSRV on all downstream consumers
Its not ideal but in order to guarantee that the project can be built and run with the MSRV we advertise, we need to restrict these. I think that the pain we live with until we can update to a higher MSRV. Hopefully that day is not too far in the future.
There was a problem hiding this comment.
This isn't the right approach. It needs to be done in dev-dependencies. Applications will, unfortunately, need to do the same if they want to build on older rustc versions.
There was a problem hiding this comment.
Spoke offline, seems that even though build-deps is what these dependencies are being used for, we want to pin them at the dev-deps since those dont get passed onto the application.
There was a problem hiding this comment.
updated in c01822e
There was a problem hiding this comment.
This isn't the right approach. It needs to be done in
dev-dependencies. Applications will, unfortunately, need to do the same if they want to build on older rustc versions.
I see, so the goal of this is purely for your own CI to check that you're still satisfying msrv? I think typically folks do that by checking in a minimum version compatible lockfile rather than needing to manually do things like this.
There was a problem hiding this comment.
checking in a minimum version compatible lockfile
What is this method? Do you have a link to a sample proj or link to a tool?
toidiu
force-pushed
the
ak-bindings-release
branch
from
1e14a14 to
c01822e
Compare
| jobserver = "=0.1.26" # newer versions require rust 1.66, see https://github.com/aws/s2n-tls/issues/4241 | ||
| home = "=0.5.5" # newer versions require rust 1.70, see https://github.com/aws/s2n-tls/issues/4395 | ||
| regex = "=1.9.6" # newer versions require rust 1.65, see https://github.com/aws/s2n-tls/issues/4242 |
There was a problem hiding this comment.
You should only need to do this once for the whole workspace. I would just do it in the generate crate.
There was a problem hiding this comment.
Let me test and verify.
generate already has those restrictions and it still fails https://github.com/aws/s2n-tls/blob/main/bindings/rust/generate/Cargo.toml#L13
At the minimum I need the pinning in s2n-tls-sys. I am testing by pulling down a fresh copy of s2n-tls and running the ./generate.sh script on my Mac.
There was a problem hiding this comment.
Tested with a fresh clone of s2n-tls and leaned up in: ffb7edb
Please DONT merge until #4393 has been merged
Description of changes:
The diff going out as part of the release: v1.4.2...v1.4.3
Testing:
I ran
cargo publish --dry-run --allow-dirtyand caught a bunch of build-dependency issues. More on there in the comments: #4388 (comment)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.