-
-
Notifications
You must be signed in to change notification settings - Fork 306
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
Avoid using feature flags and env variable to set the same parameter pt.1 emulation_mode #2512
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
7237033
to
e6a829b
Compare
Using only the feature flag simplifies things a bit and allow the usage of optional dependencies
e6a829b
to
2e353c1
Compare
Ubuntu-clippy CI fails because it is run with |
--all-features should enabled a clippy feature which makes it work magically. Maybe just build usermode always? |
1c34d86
to
b57bf87
Compare
@domenukk I'm not sure I fully understood your message 😄 |
6b50825
to
df6e6e4
Compare
df6e6e4
to
0e3c7e4
Compare
da4761f
to
b2b1552
Compare
running clippy --all-features (IMHO) should just work(tm) |
Like, why make this so much harder than it needs to be? |
The thing is that in So far the build script decided which feature to actually use and which silently ignore when running with I thought this change would simplify and clean things a bit (no hidden defaults, fallback env vars, just plain feature), but if it looks more convoluted to you I'll just drop this PR no problem :) |
Features should be additive (see: https://doc.rust-lang.org/nightly/cargo/reference/features.html#feature-unification ) so I think we should either
I'm not sure what other crates do that have mutual exclusive dependencies, like crypto libs with backends etc? |
Right below the link you've posted there is also: https://doc.rust-lang.org/nightly/cargo/reference/features.html#mutually-exclusive-features This should be avoided if possible and yes features should be additive, this is why I was saying
About
It is more or less what is done now, IMHO this should fail at compile time (also according to the above link)
As you say feature are meant to be additive, here again we would also remove/replace stuff |
The link you posted literally states:
In this case it's easy - we default to usermode and x86_64, that's what we've done so far |
BTW I don't care either way, @rmalmain and/or @andreafioraldi should say how they want it |
if there is a conflict i would fail with an error, as we already do for x86+big endian for instance |
Fine by me - but maybe if we still have the |
we can also adapt clippy flags in the clippy script is the CI variable is on. |
This PR follows discussion #2505
TLDR: Using only the feature flag simplifies things a bit and allows the usage of optional dependencies