fix(bindings): Specify correct minimum versions #5028
Conversation
goatgoose
force-pushed
the
minimal-versions
branch
2 times, most recently
from
9225d39 to
b2ccb43
Compare
goatgoose
force-pushed
the
minimal-versions
branch
from
b2ccb43 to
761106f
Compare
| @@ -36,8 +36,8 @@ stacktrace = [] | |||
| # unstable-foo = [] | |||
|
|
|||
| [dependencies] | |||
| aws-lc-rs = { version = "1" } | |||
| libc = "0.2" | |||
| aws-lc-rs = { version = "1.6.4" } | |||
There was a problem hiding this comment.
aws-lc-rs 1.6.4 is needed because the DEP_AWS_LC environment variables were not exported before then, and they're needed in order to build s2n-tls-sys.
| aws-lc-rs = { version = "1" } | ||
| libc = "0.2" | ||
| aws-lc-rs = { version = "1.6.4" } | ||
| libc = "0.2.121" |
There was a problem hiding this comment.
libc 0.2.121 is needed because aws-lc-sys versions before 0.24.0 required libc 0.2.121. aws-lc-sys 0.24.0 removed the libc dependency, so libc 0.2.62 should be acceptable for aws-lc-sys >= 0.24.0. However, I'm not aware of a way to specify a different minimum version depending on the version of another dependency in a build.
We could alternatively specify a higher minimum version for aws-lc-rs, so that aws-lc-sys doesn't specify any libc requirement.
| @@ -2,6 +2,7 @@ | |||
| name = "bench" | |||
| version = "0.1.0" | |||
| edition = "2021" | |||
| publish = false | |||
There was a problem hiding this comment.
publish was set to false in bench so that the --ignore-private flag would skip minimum version checks for it.
| hyper = { version = "1" } | ||
| hyper-util = { version = "0.1", features = ["client-legacy", "tokio", "http1", "http2"] } | ||
| hyper = { version = "1.4" } | ||
| hyper-util = { version = "0.1.4", features = ["client-legacy", "tokio", "http1", "http2"] } |
There was a problem hiding this comment.
hyper-util 0.1.4 is needed because we rely on Connection being implemented for TokioIo, which was implemented in 0.1.4.
| # A minimum hyper version of 1.3 is required by hyper-util 0.1.4. | ||
| hyper = { version = "1.3" } |
There was a problem hiding this comment.
|
I added a bit more detail to the inline comments, let me know if they can be improved more! |
Description of changes:
There are currently dependency versions in the s2n-tls rust bindings that are specified as acceptable but are impossible to actually use. For example, we specify "1" as an acceptable aws-lc-rs version, but versions before 1.6.4 result in a build failure. This PR updates these dependency versions to the correct minimum versions
This issue was discovered when adding s2n-tls as a dependency of smithy-rs. smithy-rs has a test which tries to build their crates with the minimum versions of all dependencies, which breaks for s2n-tls. I added a similar test to our CI.
Call-outs:
In our test, I added the
--directflag, which uses the direct-minimal-versions variant to only use minimum versions of direct dependencies rather than all transitive dependencies. This is because I'm not sure what we would do about a transitive dependency that failed to properly specify a minimum version, so it didn't seem right to block CI on this. The Rust documentation also doesn't recommend the transitive variant for this reason.However, smithy-rs doesn't specify the --direct flag in their test, which is how this issue was observed when adding s2n-tls as a dependency. Should we be checking minimal versions in all transitive dependencies too?
Testing:
Tested via the new minimal-versions job in CI.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.