Closed justsmth closed 2 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 92.48%. Comparing base (
c358484
) to head (144c689
). Report is 64 commits behind head on main.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Have you considered using a Cargo feature instead of an environment variable to make this work? That seems like a solution with better ergonomics for most downstream Rust applications.
Have you considered using a Cargo feature instead of an environment variable to make this work? That seems like a solution with better ergonomics for most downstream Rust applications.
We considered using a feature for this. The concern is that we want the decision of whether it's acceptable to use the pre-built binaries to be made by the end consumer within their build environment. Because features are only "additive" and features can be enabled by any crate that's part of a build, it seems like the wrong mechanism.
As a feature, the consumer would essentially be required to "delegate" the decision of whether to use NASM pre-built binaries to any/every crate in their dependency tree. One possible compromise might be to expose a feature that specifies what to default to when AWS_LC_SYS_PREBUILT_NASM
is not set in the environment -- but this also seems to violate the nature/purpose of "features" as the environment will be "subtracting" a "feature" that a dependent crate indicated that it needed.
We considered both "environment variables" and "features" as mechanisms for enabling this. Is there another option that would also enable intermediate crates (i.e., those part of the dependency tree between us and the end consumer) to influence what the default choice is?
We considered both "environment variables" and "features" as mechanisms for enabling this. Is there another option that would also enable intermediate crates (i.e., those part of the dependency tree between us and the end consumer) to influence what the default choice is?
No, I don't think so. Your rationale makes sense to me, unfortunately Cargo doesn't support anything like this right now (see https://internals.rust-lang.org/t/pre-rfc-mutually-excusive-global-features/19618 for some discussion -- and if you think there is appetite for sponsoring work on this at AWS, I'd be interested in discussing that!).
As a downstream application builder I would still prefer being able to control this via features, maybe just with documentation guidance that libraries should not touch this and potentially keeping a way to override it via the environment variable?
@djc -- Thanks for your feedback!
We are still discussing this issue internally. We may follow up in a subsequent PR to adjust the build environment conditions that determine whether the pre-built NASM objects are allowed to be used.
Issues:
Addresses #364
Description of changes:
Call-outs:
AWS_LC_SYS_PREBUILT_NASM=1
- Prebuilt artifacts used only if NASM was not found.AWS_LC_SYS_PREBUILT_NASM=0
- NASM is required.Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.