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 i686-unknown-linux-gnu and aarch64-unknown-linux-gnu on CI #613

Merged
merged 9 commits into from
Jan 4, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Dec 9, 2020

Tests i686-unknown-linux-gnu and aarch64-unknown-linux-gnu on CI by using cross.
Closes #598

I'm surprised that it looks like we don't have to ignore channel tests.

r? @jeehoonkang

@taiki-e taiki-e force-pushed the i686-linux-gnu branch 2 times, most recently from d35d314 to af6c551 Compare December 9, 2020 14:12
.github/workflows/ci.yml Outdated Show resolved Hide resolved
strategy:
matrix:
crates:
- crossbeam
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can maintain only one set of crates for both x86-64 and the cross-target platforms. Maybe later we will add more crates, and I'd wish we should change the CI script in only one place...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. A single 'crates' set is certainly preferable. (It makes a matrix a little more complicated, but it should be possible.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes a matrix a little more complicated, but it should be possible.

Hmm, the way I thought would work didn't really work. I'll look for other ways.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I addressed this by testing all crates at once in cross tests. (07a1366. The way of adjusting the matrix did not work.)

This should make non-x86-64 platform test time a bit longer than x86-64 platform, at this time. However, eventually, x86-64 platform test will take more time because sanitizers (#591), miri (#578), etc. will be added to x86-64 platform test. So I think this is fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the revised PR. I just want to suggest another (potential) alternative: can we also test x86-64 platform in cross? If so, we can uniformly treat all platforms using cross.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cross doesn't provide images for windows-msvc and darwin, so I don't think using cross on those platforms will no benefit.
However, I'll take a look for a way to merge the test and cross jobs into one job.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@taiki-e taiki-e marked this pull request as draft December 10, 2020 01:31
@taiki-e taiki-e changed the title Test i686-unknown-linux-gnu on CI Test i686-unknown-linux-gnu and aarch64-unknown-linux-gnu on CI Dec 10, 2020
@taiki-e taiki-e marked this pull request as ready for review December 15, 2020 10:58
@taiki-e taiki-e requested a review from jeehoonkang December 15, 2020 10:58
@taiki-e taiki-e added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Dec 31, 2020
@taiki-e taiki-e removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author label Jan 4, 2021
@taiki-e taiki-e force-pushed the i686-linux-gnu branch 3 times, most recently from a1c5b13 to 8796beb Compare January 4, 2021 10:43
@taiki-e taiki-e force-pushed the i686-linux-gnu branch 3 times, most recently from 85c048b to 41657a2 Compare January 4, 2021 11:22
Feature flag can be accidentally enabled by --all-features, so use cfg
instead.
@taiki-e
Copy link
Member Author

taiki-e commented Jan 4, 2021

@jeehoonkang Updated to test all crates at once.

  • Tests that were previously run per crate are run at once using --all flag.
  • Use cross if the target is set, otherwise, use cargo and test with the host target. See Test i686-unknown-linux-gnu and aarch64-unknown-linux-gnu on CI #613 (comment) for context.
    For now, the non-host target only runs tests.
  • Replaced sanitize feature with cfg(crossbeam_sanitize).
    Feature flag can be accidentally enabled by --all-features, so use cfg instead. (This issue actually occurred when I tried to use --all-features instead because --features nightly cannot be used together with --all.)
    sanitize feature is a feature for testing, not a feature intended to be used as a public API, so it should be okay to remove it.
  • Separated check for docs into its own job.
    This is in a different area than the test and I think it makes sense to run it as a separate job.

This was referenced Jan 4, 2021
Copy link
Contributor

@jeehoonkang jeehoonkang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup! I have a few comments.

.github/workflows/ci.yml Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
ci/test.sh Outdated Show resolved Hide resolved
@jeehoonkang
Copy link
Contributor

Thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 4, 2021

Build succeeded:

@bors bors bot merged commit a66fe72 into master Jan 4, 2021
@bors bors bot deleted the i686-linux-gnu branch January 4, 2021 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

We need to test i686 linux-gnu on CI by using cross
2 participants