Skip to content

Conversation

@gustafla
Copy link
Contributor

@gustafla gustafla commented Jun 4, 2025

I want to control some of the CPU feature flags and hardening features.

Copy link
Owner

@DoumanAsh DoumanAsh left a comment

Choose a reason for hiding this comment

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

This looks ok, but please make sure these are not default
User should be explicit about using features

@gustafla
Copy link
Contributor Author

gustafla commented Jun 5, 2025

That's a very good concern, but in order to keep the library build as it was before, some of the opus cmake features have to be default in cargo, because they are default in libopus itself and would be enabled implicitly if not given "ON"/"OFF" definitions.

The other option would be to add "no-hardening", etc. features, but that would clash against the idea that cargo features should always be additive.

I think the best would be to add an automatic script a la bindgen that would parse the relevant feature flags and their defaults from libopus's CMakeLists.txt, and output both a Cargo.toml features-section and a patched build.rs.

@DoumanAsh
Copy link
Owner

I didn't have time to check it myself, but what CMake features are enabled by default?
I believe it should be only OPUS_X86_MAY_* flags at most

In general library's features should be never by default and I do not accept any library that has default feature without reason.

Your current PR only allows to enable these CMake flags, and never disable
So having it as default is not correct anyway.
Default CMake options should be normally suitable for target and features should be only to be used when user needs it (most likely scenario is cross compilation)

Do keep in mind that Cargo features are not necessary reflective of CMake itself so you should not try to map it 1 to 1.
Please elaborate further what you want to achieve and I will take a look at it myself

@gustafla
Copy link
Contributor Author

gustafla commented Jun 5, 2025

Allright, thanks for your review. I put the PR together a bit too hastily yesterday. I looked at the CMakeLists.txt and it's feature enables/disables are more complicated than I thought, because it has conditionals for whenther the target CPU is x86, ARM or such and only then some features get enabled.

This FeatureSummary mechanism or something related to it could possibly be exploited in order to generate sensible global Cargo features, but for now I would be satisfied with just a few safety disable features.

@DoumanAsh DoumanAsh changed the title 0.5.6: Add more libopus features as cargo features Expose libopus cmake configuration options through cargo features Jun 5, 2025
@DoumanAsh DoumanAsh merged commit 93e6fe9 into DoumanAsh:master Jun 5, 2025
3 checks passed
@DoumanAsh
Copy link
Owner

I release 0.5.6

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