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

Experimental supports cfg #694

Closed
pinkforest opened this issue Aug 28, 2024 · 10 comments · Fixed by #695
Closed

Experimental supports cfg #694

pinkforest opened this issue Aug 28, 2024 · 10 comments · Fixed by #695

Comments

@pinkforest
Copy link
Contributor

pinkforest commented Aug 28, 2024

Re:

p.s. how do we implement the opt-in given we have declared backend details as implementation detail.

feature will not do as it poses problems of a) relaying across dependency chain b) not having top-level binary in control

Same as #414 we could use cfg for this - I propose:

cfg(curve25519_dalek_unstable = "avx512")

Which will in turn support for unstable AVX512 through nightly opt-in unstables basis.

@tarcieri
Copy link
Contributor

I would suggest using the existing curve25519_dalek_backendcfg rather than adding yet another one

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 28, 2024

Problem is this is more like sub-configuration and it conflates configuration between stable and unstable configuration.

By denoting explicitly that it's unstable, it avoids the confusion of opting into something that is not entirely stable.

e.g. curve25519_dalek_backend is mostly for stable backend configuration and is provided for solely as "override"

Also backend is more backend class and not explicit backend, e.g. "simd" can mean anything in between those stable-only backends.

Additionally providing override to sub-backend gets confusing when it's supposed to be only override and not opt-in cfg.

@tarcieri
Copy link
Contributor

Having two different cfg options for the backend reintroduces the problem that they can both be set in conflicting states, which was the whole reason we moved away from features in the first place.

I think we already have all the cfg options we need and the main thing we need to do at this point is not automatically enable nightly-only features.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 28, 2024

The cfg conflict happens even today, it is perfectly possible to set two cfg values stuffed into same key -

However we guard and test that in build script just like we would guard the opt-in which is different to override.

Adding it under _backend will complicate the gating and will pollute unstable configuration where it has = "simd"

Logically speaking if we have curve25519_dalek_backend = "simd" and then we have "avx512" it will be confusing?

When somebody writes --cfg curve25519_dalek_backend="simd" --cfg curve25519_dalek_backend="avx512"

It gets confusing right? because the above would work .. and confusingly people might confuse it as stable backend

We could say --cfg curve25519_dalek_backend="unstable_avx512" perhaps but still confusing (less though) IMO

@tarcieri
Copy link
Contributor

Your example would be caught by the compiler, as opposed to requiring logic at every backend selection site.

I very strongly believe we should not (re)introduce backend selection knobs that duplicate/conflict with each other.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 28, 2024

The current backend knobs are overrides and it does not guarantee selection though.

We can settle with backend="unstable_avx512" and at least the value denotes the override into unstable sub-backend.

btw compiler doesn't mind duplicate --cfg values and it's something I had to address earlier

@tarcieri
Copy link
Contributor

Regardless, the point is not to have logic in backend selection code all over the place that has to handle potentially conflicting user inputs.

That would be a major regression over where we are now.

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 28, 2024

Wanna comment on having the unstable_ in the value to denote it's unstable-ness? That would allow it in one cfg.

@tarcieri
Copy link
Contributor

That seems better than two potentially conflicting attributes

@pinkforest
Copy link
Contributor Author

pinkforest commented Aug 28, 2024

Cool great let's do that

EDIT: Another could have been curve25519_dalek_backend = "simd_unstable_avx512" but it's longer w/o value

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 a pull request may close this issue.

2 participants