Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use --cfg curve25519_dalek_backend to select backend #455

Merged
merged 6 commits into from
Dec 9, 2022

Conversation

tarcieri
Copy link
Contributor

@tarcieri tarcieri commented Dec 7, 2022

Crate features are intended to be additive, whereas only 1-of-N possible backends can be selected.

Features can also be activated by transitive dependencies, which leads to a problem of different dependences selecting conflicting backends. Using --cfg instead moves all backend selection control to the toplevel executable.

This commit switches to the following RUSTFLAGS to enable backends:

  • --cfg curve25519_dalek_backend="fiat": uses fiat-crypto backend
  • --cfg curve25519_dalek_backend="simd": uses nightly-only SIMD backend

src/backend/mod.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri force-pushed the cfg-based-backend-selection branch 3 times, most recently from f9de8b0 to a4270c4 Compare December 9, 2022 03:42
Crate features are intended to be additive, whereas only 1-of-N possible
backends can be selected.

Features can also be activated by transitive dependencies, which leads
to a problem of different dependences selecting conflicting backends.
Using `--cfg` instead moves all backend selection control to the
toplevel executable.

This commit switches to the following RUSTFLAGS to enable backends:

- `--cfg curve25519_dalek_backend="fiat"`: uses `fiat-crypto`
- `--cfg curve25519_dalek_backend="simd"`: uses nightly-only SIMD
@tarcieri tarcieri changed the title [WIP] Use --cfg attributes to select backends Use --cfg curve25519_dalek_backend to select backend Dec 9, 2022
@tarcieri tarcieri marked this pull request as ready for review December 9, 2022 03:51
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 9, 2022

This is code complete.

It still needs corresponding documentation changes.

Copy link
Contributor

@rozbb rozbb left a comment

Choose a reason for hiding this comment

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

I think this is good to go. The new backend selection docs can be in a new PR, once #453 lands.

Copy link
Contributor

@pinkforest pinkforest left a comment

Choose a reason for hiding this comment

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

Looks great :)

* Restructure README and CHANGELOG
* Explain semver policy
* Specify feature flags and backends more explicitly
* Remove nightly from the CI bc that didn't belong there
* Add @pinkforest to thankyou list

Co-authored by @pinkforest
@jrose-signal
Copy link
Contributor

I'm sorry I didn't look at this sooner; why is bits a separate axis?

@tarcieri tarcieri deleted the cfg-based-backend-selection branch December 9, 2022 18:06
@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 9, 2022

@jrose-signal because both the (default) serial backend and fiat backend both have 32-bit and 64-bit implementations, so it's something of a matrix with two dimensions

@jrose-signal
Copy link
Contributor

Yes, but future backends may not come in such pairs, and the bits selection doesn't necessarily correspond to your target architecture (as we've discovered), and it's diverging further from the 3.x branch.

@pinkforest
Copy link
Contributor

My initial PR was embedding it with backend and it became horrondeous to maintain not to mention merge conflicts around where we needed to put cfg() gates between as there is cross-use.

@pinkforest
Copy link
Contributor

pinkforest commented Dec 9, 2022

If future backend doesn't need the selection then people don't need to set those bits - this is just an override not a default.

Default is selected automatically and then there is a further lotto over that:

I'm documenting it here:

@tarcieri
Copy link
Contributor Author

tarcieri commented Dec 9, 2022

Yeah, doing anything complex with cfg is something of a nightmare.

@jrose-signal I suggest you review #465. It should hopefully eliminate the need for manual curve25519_dalek_bits configuration for your use cases

@pinkforest
Copy link
Contributor

pinkforest commented Dec 9, 2022

We also have an issue around that would put the backends into separate crates:

I also thought why not re-export "either" via pub (e.g. both u32 and u64 would be known uX) but this would be fairly involved as every export would have to be re-exported and it would be error prone compared checking that imported one is from e.g :u64:

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.

None yet

4 participants