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

Allow specifying the target to run the test with #136

Open
MOZGIII opened this issue Nov 15, 2021 · 5 comments
Open

Allow specifying the target to run the test with #136

MOZGIII opened this issue Nov 15, 2021 · 5 comments
Labels
C-enhancement Category: A new feature or an improvement for an existing one help wanted Call for participation: Help is requested to fix this issue

Comments

@MOZGIII
Copy link

MOZGIII commented Nov 15, 2021

Some context first: we are building a blockchain on substrate, and have a monorepo with a bunch of crates. Some crates are intended to be built in both std and no_std modes, but the issue is that when building in no_std mode, wasm target is assumed. Crates have an std flag, which enables std mode in dependencies, and when it's absent - the dependent crates also disable std mode (and switch to assuming wasm).
The problem is we can't test no_std mode with cargo hack.

It would be great if we could use a presence of a certain feature in the features set under test to use a different compile target (in our case - wasm32-unknown-unknown).

An example.

Let us have a crate with features std = ["dep1/std", "dep2/std"], feat1 = ["std", "codec"], feat2 = ["some-non-std-feature"].

We'd like to test in the following way:

  • default-features = false
    features: [std]
    target: native (since std is explicitly enabled)
  • default-features = false
    features: [feat1]
    target: native (since std is implicitly enabled via feat1)
  • default-features = false
    features: [feat2]
    target: WASM (since std is effectively not enabled)
  • default-features = false
    features: []
    target: WASM (since std is effectively not enabled)

We could specify all this manually somehow if we had just one or two crates, but we have way too much of them, and the logic to determine the target is so simple, yet tricky, so I thought it would be a good fit for cargo-hack.
I'm sure if cargo-hack offers this functionality other projects will pick it up as well.

@taiki-e
Copy link
Owner

taiki-e commented Nov 15, 2021

Do you mean you need an option to process the following two lines in one line?

cargo hack check --each-feature
cargo hack check --each-feature --skip std --target wasm32-unknown-unknown

@taiki-e taiki-e added the C-question Category: A question label Nov 15, 2021
@MOZGIII
Copy link
Author

MOZGIII commented Nov 15, 2021

Those two lines are not going to cut it. What they don't handle is:

  1. We don't need all crates to be tested under wasm32-unknown-unknown - some of them simply don't support no_std. yet, some of them work in both std and no_std mode. When we work at no_std mode, we always work in wasm. I.e. the second line breaks here. We can work around this by having a list of all the crates that need to be tested in wasm target - but it's hard to maintain.
  2. When we test in the std mode, the tests still choose feat2, which is only supposed to work in no_std mode; we have to list all such features in the command, which works but is messy and hard to maintain: cargo hack check --feature-powerset --no-dev-deps --exclude-no-default-features --skip runtime-benchmarks --skip try-runtime ....

@MOZGIII
Copy link
Author

MOZGIII commented Nov 15, 2021

Overall, I think baking in the logic to derive target from a feature is kind of overly specific, but I've figured that each crate can have some instructions on how it should be tested by cargo-hack in it's Cargo.toml metadata - which can be a more general solution to the same issue.
What if we add some extra information there?

@taiki-e
Copy link
Owner

taiki-e commented Nov 20, 2021

Thanks for the explanation. If I understand correctly, what this feature request is asking for is something like:

  • A way to specify options such as --exclude and --skip per-crate (optionally per-target).

    The package.metadata in Cargo.toml seems to be a reasonable choice, since it can get as json via cargo-metadata.

    Something like the following:

    # Default options if no per-target option is specified.
    [package.metadata.cargo-hack]
    skip = ["feat1"]
    exclude = false # If true, same as `--exclude crate_name`
    
    # Options when built for specific target (wasm32-unknown-unknown, in this case)
    [package.metadata.cargo-hack.target.wasm32-unknown-unknown]
    skip = ["std"]

And the following is the feature that I initially thought this feature request to be asking for.

  • A way to handle multiple targets in one command.

    For this, cargo already has two unsettling features.

    Implementing an ability that emulates multitarget is okay, but this ability does not seem to be a blocker to meet this feature request.

@taiki-e taiki-e added C-enhancement Category: A new feature or an improvement for an existing one and removed C-question Category: A question labels Nov 20, 2021
@MOZGIII
Copy link
Author

MOZGIII commented Nov 20, 2021

Thanks for the explanation. If I understand correctly, what this feature request is asking for is something like:

Yep. something like this.

I tried to implement it manually with some scripting, but it ended up non-trivial with bash and cargo metadata.

One thing to note is a difficulty with how substrate projects manage std/no_std switching. We only have a flag for std, which is also the default, but can be turned off with --no-default-features; one problem I encountered was that no_std environment only works in wasm target - i.e. there are some compile-time assertions on pointer sizes etc, so it's not possible to test/check no_std with native target.

This means we need to check some featured only with wasm target, some only with non-wasm; and some with both. The presence of the std feature in the selected features is a solid indicator of whether wasm or non-wasm target should be used.

Can we actually detect whether the std feature is active with a particular feature set to pick the target? This will greatly reduce the amount of manually entered info that we'd have to keep in the Cargo.toml metadata.

Some extra context of our case

We have, by in large, three kinds of crates:

  • builds with std and some non-wasm target (the usual rust);
  • builds both without std for wasm target, and with std for non-wasm target (usually, the non-wasm variant builds second, and embeds the wasm variant, wrapping it with the host/wasm glue code); these typically can also run tests in std mode.

What would be nice is to run the checks on the individual crates in the no_std mode, as we've been hit by a lack of said tests a couple of time already.

@taiki-e taiki-e added the help wanted Call for participation: Help is requested to fix this issue label Dec 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A new feature or an improvement for an existing one help wanted Call for participation: Help is requested to fix this issue
Projects
None yet
Development

No branches or pull requests

2 participants