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

Provide means to test external_asm code path #337

Closed
jarkkojs opened this issue Jan 26, 2022 · 6 comments
Closed

Provide means to test external_asm code path #337

jarkkojs opened this issue Jan 26, 2022 · 6 comments

Comments

@jarkkojs
Copy link
Contributor

In order to exercise external asm code path, I needed to do this:

find -type f -print0 | xargs -0 sed -i 's/inline_asm/xxx_inline_asm/'
find -type f -print0 | xargs -0 sed -i 's/not(xxx_inline_asm)/external_asm/'
git checkout build.rs
git checkout src/lib.rs
cargo build --features external_asm
cargo test --features external_asm
git reset --hard HEAD

Since it is requirement for code paths to fully work, we need a mechanism for this.

Perhaps a in-project configuration flag, e.g. x86_64_inline_asm,of which value would be set by rustc specific config algs inline_asm would provide a feasible solution for this?

@josephlr
Copy link
Contributor

Maybe I'm just confused at the context, but why can't you just do:

cargo build --no-default-features --features external_asm,instructions

That's what we currently do in the CI.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Jan 26, 2022

So with that issue being that you'd need to white every possible feature other feature than the one that you want blacklist. It simply does not work in the long run. It's an anti-pattern.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Jan 26, 2022

Maybe I'm just confused at the context, but why can't you just do:

cargo build --no-default-features --features external_asm,instructions

That's what we currently do in the CI.

Let me also add that I did not know this and will use it in the next time but for my common sense, and e.g. scalability of the project that just sounds grazy, even if it does work with a limited amount of flags.

I would even consider using m4 or cpp for preprocessing rather doing that type of a pattern.

@jarkkojs
Copy link
Contributor Author

jarkkojs commented Jan 26, 2022

OK maybe this is not a such a big issue in the context of this crate. Probably the amount of cfg flag will never grow. Thank you for the support. I used the following commands:

cargo build --no-default-features --features external_asm,instructions
cargo test --no-default-features --features external_asm,instructions -- --test-threads=1

I appreciate the support in any case!

@Freax13
Copy link
Member

Freax13 commented Jan 26, 2022

OK maybe this is not a such a big issue in the context of this crate. Probably the amount of cfg flag will never grow.

Cargo features are meant to be purely additive, you don't need to add all the features activated by default to run the tests. Besides that the thing that was actually causing problems was that two features are mutually exclusive, which isn't very common, but is useful sometimes. However that's about as complicated as it gets. For example I've never seen a project use more than two mutually exclusive features or two sets of two mutually exclusive features. For those reasons, in practice this works just fine and scale isn't really an issue.

@phil-opp
Copy link
Member

The good news is that inline asm will be stabilized in Rust 1.59. Then we can remove the external asm completely and don't need the mutually exclusive features anymore.

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

No branches or pull requests

4 participants