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

Test more conditions in GitHub actions #12

Closed
joshlf opened this issue Sep 22, 2022 · 11 comments
Closed

Test more conditions in GitHub actions #12

joshlf opened this issue Sep 22, 2022 · 11 comments
Assignees

Comments

@joshlf
Copy link
Member

joshlf commented Sep 22, 2022

Use GitHub actions to test the following axes:

  • Currently stable, current beta, current nightly, MSRV stable (1.51.0)
  • All of the different targets that affect us (currently, this is only relevant for the simd and simd-nightly features, which emit impls for various architecture-specific types)
  • All of the Cargo features we support
  • cargo check, cargo test, and cargo miri test

Note that, depending on how we implement this, some combinations of the above may not be possible to run. E.g., if we run on an x86_64 machine, it may be possible to run cargo miri test while targeting powerpc (I'm not sure about this), but it definitely won't be possible to run cargo test while targeting powerpc.

As a possible stretch goal, also include a test to make sure that README.md is kept up to date (#18).

@joshlf joshlf changed the title Test more conditions Test more conditions in GitHub actions Sep 22, 2022
@AntoniosBarotsis
Copy link
Contributor

AntoniosBarotsis commented Oct 1, 2022

Hello! I was wondering if this still up for taking and if so whether you could explain the issue in a bit more detail (I'm also new to rust in general so I probably missed some stuff).

From what I understand you want to:

  • Check, build and test your crate + the 3 features, namely alloc, simd and simd-nightly
  • Run all these against some predetermined targets

I am not sure if running these in both stable and nightly (when applicable) changes anything.

Also, have you considered using this for the MSRV?

@joshlf
Copy link
Member Author

joshlf commented Oct 2, 2022

Hi @AntoniosBarotsis , thanks for offering! Yes, it's still open; feel free to assign it to yourself if you feel like taking it on!

There are a few considerations here; I'll break them down by the "axes" I mentioned in the original comment:

Version

We want to make sure to test with different versions of the toolchain for a few reasons:

  • The nightly toolchain is required in order for us to test using unstable Rust features, which are required for the simd-nightly Cargo feature that we expose (we could also add other nightly-only Cargo features in the future)
  • The beta toolchain is just us being a good citizen - the Rust project always needs more people to test code using the current beta
  • The MSRV stable ensures that we successfully compile on our published MSRV
  • The current stable is technically redundant given that we're also going to be testing on MSRV (if tests succeed on MSRV, they should succeed on current stable as well), but I'm a bit paranoid that if we ever removed tests for MSRV in the future and we didn't also have a test for current stable, we might not notice and would end up in a situation where we weren't testing on any stable, which would be bad. It also has the advantage that, if a PR breaks MSRV stable but not current stable, it means that it's relying on a feature that was introduced between MSRV stable and current stable, and suggests that we might want to update our MSRV in the next breaking release.

Targets

Some of our code is CPU architecture-specific. Notably, the simd and simd-nightly Cargo features emit trait implementations for architecture-specific SIMD types. Thus, when compiling for, e.g., x86_64, the powerpc SIMD types aren't tested, and vice-versa. In order to make sure all of the SIMD types (and any other future architecture-specific functionality we add) are tested, we need to compile for every architecture that we emit code for. Currently, the full list of architectures is:

  • x86
  • x86_64
  • wasm32
  • powerpc
  • powerpc64
  • arm
  • aarch64

Cargo features

We want to make sure that every combination of Cargo features we support works (including enabling no features). Currently, those features are alloc, simd, and simd-nightly. Since our features don't really interact (e.g., the behavior of the alloc feature is not changed by enabling the simd feature), it's probably sufficient to just test once with all features disabled and once with all features enabled instead of testing with all 2^3 = 8 possible combinations.

Tests

We want to run a few different tests:

  • cargo check
  • cargo build (basically the same as cargo check, but in some rare cases, cargo check can succeed while cargo build fails)
  • cargo test - Run our tests!
  • cargo miri test - Run our tests under Miri, which is an interpreter that can catch some undefined behavior in unsafe Rust code (and basically all that zerocopy does is unsafe, so this is super important for us)

Invalid combinations

Note that some combinations of these axes aren't valid. E.g., since the simd-nightly feature relies on unstable Rust features, we can't enable it on the stable or beta toolchains. Similarly, if the machine we're running on is a Linux x86_64 machine, we can't actually run (e.g., cargo test) any code which is built for other CPU architectures. Maybe we can still run cargo miri test? I'm not actually sure whether that would work.

The task

What is required for this issue is to figure out how to write a GitHub Actions config file that will run every combination of these axes (except for the combinations that aren't valid), and ideally does it in an idiomatic way (e.g., we probably don't need to literally list every possible combination in the config file). I haven't ever written a GitHub Actions config file myself before, so unfortunately I can't provide much guidance on that front. However, from a bit of Googling, it looks like there is a lot of documentation on them, a lot of blog posts written about them (including specifically about using them for Rust tests, and even more specifically about using them for Rust tests which target other CPU architectures).

Hopefully that detail helps! Feel free to ask any other questions you have 😃

Also, have you considered using this for the MSRV?

I wasn't aware of that; thanks for the heads up! Apparently our MSRV is 1.51.0 😃

@AntoniosBarotsis
Copy link
Contributor

AntoniosBarotsis commented Oct 2, 2022

"The beta toolchain is just us being a good citizen"

That's a nice initiative actually.

I think I understood most of what you said, I'll go over everything tomorrow again as it is really late here.

I was thinking of using Toast which helps a lot when testing the pipeline since you can run it locally but I'll see if that would work with the many config combinations you want (because I want each one of those combinations to be its own task, I think it is possible but I'll have to check tomorrow). I have used this once before, it is trivial to run this from Github Actions but being able to also run it locally is a massive help instead of pushing 10 times until your yml is formatted correctly.

I think that's it for now, I'll probably start asking questions tomorrow when I start setting stuff up :)

Edit: I don't think I can assign myself to the issue but yes I'll be working on this, wondering if you could also add a Hacktoberfest label to the merge request if you're happy with the end result 👀

@joshlf
Copy link
Member Author

joshlf commented Oct 2, 2022

Awesome, that all sounds great!

I don't think I can assign myself to the issue

Done 😃

wondering if you could also add a Hacktoberfest label to the merge request if you're happy with the end result 👀

Sure! Feel free to remind me if I forget once you put up a PR.

@joshlf
Copy link
Member Author

joshlf commented Oct 2, 2022

I dug a bit into how to do cross-compilation; hopefully this'll be helpful for you.

First, you actually need to install a target, not a toolchain. If you install a toolchain, it'll assume you're actually running on that sort of machine (e.g., if you install a powerpc toolchain, it'll be a powerpc binary), and you'll get a lovely error like:

/path/to/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/bin/cargo: /path/to/.rustup/toolchains/nightly-aarch64-unknown-linux-gnu/bin/cargo: cannot execute binary file

Instead, you can install a target using rustup target add and compile for it using Cargo's --target flag:

$ rustup target add aarch64-unknown-linux-gnu
$ cargo check --target=aarch64-unknown-linux-gnu

Note that this seems not to play well with Cargo's toolchain selection mechanism. In particular, let's assume that you previously had your default toolchain set to nightly (which I did when I was trying this out). If you then run cargo +stable check --target=aarch64-unknown-linux-gnu, you'll find that you don't have what you need for the stable toolchain and the given target:

$ cargo +stable check --target=aarch64-unknown-linux-gnu
   Compiling proc-macro2 v1.0.43
   Compiling quote v1.0.21
   Compiling unicode-ident v1.0.4
   Compiling syn v1.0.99
    Checking byteorder v1.4.3
error[E0463]: can't find crate for `core`
  |
  = note: the `aarch64-unknown-linux-gnu` target may not be installed
  = help: consider downloading the target with `rustup target add aarch64-unknown-linux-gnu`

What you need to do is specifically install the target for the toolchain you'll be using:

$ rustup +stable target add aarch64-unknown-linux-gnu

Putting that all together, if you're trying to run a given Cargo command (let's say check for this example) for a particular $TOOLCHAIN (stable, beta, nightly, 1.51.0-x86_64-unknown-linux-gnu (our MSRV), etc) and $TARGET, you'd run:

$ rustup +$TOOLCHAIN target add $TARGET
$ cargo +$TOOLCHAIN check --target=$TARGET

@AntoniosBarotsis
Copy link
Contributor

Ok so, this might be a bit easier than I thought.

I haven't started working on this yet but I just added a workflow for a personal project I am working on and it turns out that using different channels is very easy and different compiler versions should be basically the exact same. Because of this, I will probably not need Toast.

I did not know this but it is possible to make the workflow not fail if specific tasks fail which is great because nightly failures won't cause the entire workflow to be invalidated (but you can still see where and why they failed).

Lastly, I keep forgetting you can add if checks to these workflows so ignoring the "invalid cases" that you mentioned should be fairly straightforward as well.

You can most likely expect the first draft tonight! I'll try to provide a rough explanation of how stuff works when I start pushing since you haven't worked with Github Actions before.

@joshlf
Copy link
Member Author

joshlf commented Oct 2, 2022

Awesome, thanks!

By the way, heads up that we may not actually have any nightly-only code after all - it's possible that the stdsimd feature is already stabilized. If that's the case, maybe just add a fake Cargo feature that enables some unstable feature just to test to make sure that we can ignore certain combinations? E.g., in Cargo.toml:

[features]
fake-nightly = []

...and in lib.rs:

#![cfg_attr(feature = "fake-nightly", try_blocks)]

@AntoniosBarotsis AntoniosBarotsis mentioned this issue Oct 2, 2022
4 tasks
@AntoniosBarotsis
Copy link
Contributor

AntoniosBarotsis commented Oct 2, 2022

Could you outline the exact targets that you want because I'm not sure what the differences between some of these are.

You can check the available targets with rustup target list.

The one I am currently using is x86_64-unknown-linux-gnu.

Also I am currently trying to add miri test and it seems that it is failing somewhere, check these logs for example. Is this expected or should it be passing?

@AntoniosBarotsis
Copy link
Contributor

I think that everything else is actually working fine.

You can see the parameters I use here, the tests will be on all possible combinations of these. Let me know if I missed something.

@joshlf
Copy link
Member Author

joshlf commented Oct 3, 2022

Moving the conversation over to the PR thread.

@joshlf
Copy link
Member Author

joshlf commented Oct 4, 2022

Fixed in fa7e3e8.

@joshlf joshlf closed this as completed Oct 4, 2022
@joshlf joshlf mentioned this issue Aug 20, 2023
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

2 participants