Disable default features for aws-lc-rs
#286
Conversation
daxpedda
force-pushed
the
aws-lc-rs-default-features
branch
from
cbd4c00
to
c0e83b0
Compare
| ring = ["crypto", "dep:ring"] | ||
| fips = ["aws_lc_rs", "aws-lc-rs?/fips"] |
There was a problem hiding this comment.
I removed the ? because it didn't make sense, or was it there for a reason?
There was a problem hiding this comment.
I think you're right that in this context it doesn't make sense and should be removed. My understanding of the ? is that it (docs):
"will only enable the given feature if something else enables the optional dependency."
But in this case, the fips feature is activating aws_lc_rs right alongside aws-lc-rs/fips.
@djc Does that match your understanding? The usage came along with #234 and wasn't discussed explicitly as far as I can tell.
There was a problem hiding this comment.
IIRC there was a bug in older Cargo versions that needed the ? to be there. If it works without it on the project's MSRV, I'm fine with this.
There was a problem hiding this comment.
I tried it locally with Rust v1.66 and it compiles for me.
There was a problem hiding this comment.
"aws_lc_rs", "aws-lc-rs?/fips" is the new way of writing "aws-lc-rs/fips" and makes it explicit that the enabling the aws-lc-rs feature is vital for the fips feature.
There was a problem hiding this comment.
In this case I don't think it makes sense to preserve the ?, since we unconditionally enable the optional dependency anyway when the fips feature is enabled.
There was a problem hiding this comment.
In this case I don't think it makes sense to preserve the ?
I agree. Since @est31 didn't request any changes and approved the PR I think it's fair to merge with the removal?
|
Seems surprising that support for -sys is optional in aws-lc-rs? |
AFAIU the choice is between |
Rcgen doesn't require
aws-lc-rs's default features, so disabling them removes some dependencies.