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.

Sign up for GitHub

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

Jump to bottom

Extend CI configuration #563

Merged
merged 1 commit into from
Jul 27, 2018
Merged

Extend CI configuration #563

merged 1 commit into from
Jul 27, 2018

Conversation

pitdicker
Copy link
Contributor

The past releases had a couple bugs slip by that could have been caught by CI, so I'm trying to extend it a bit.

Please ignore for now.

@pitdicker pitdicker force-pushed the travis branch 2 times, most recently from 77677f8 to 083cdfb Compare July 20, 2018 20:06
@pitdicker pitdicker closed this Jul 20, 2018
@pitdicker pitdicker reopened this Jul 20, 2018
@pitdicker pitdicker closed this Jul 20, 2018
@pitdicker pitdicker reopened this Jul 21, 2018
@pitdicker pitdicker force-pushed the travis branch 12 times, most recently from f720c3c to 1841db9 Compare July 21, 2018 18:39
@pitdicker
Copy link
Contributor Author

pitdicker commented Jul 21, 2018 •
edited
Loading

Notable changes here:

  • All configurations not have a DESCRIPTION environment variable.
  • We now have only one macOS builder. Before we had a second one wich cross-compiled to iOS. Cross-compiling to iOS didn't require Trust, Cross etc., and takes little time. We already couldn't run tests for it (very hard to set up, because you can't 'just' run a binary on iOS). So I moved it in with the macOS builder.
  • cargo-deadlinks and cargo-web print the installed version.
  • We now have one builder that runs tests for Emscripten, stdweb and wasm-bindgen. At least, that is the idea. I ran into issues trying to get cargo-web test Emscripten on chrome and Node.js, so Emscripten now only builds. Also the result of wasm-bindgen will require manual incantations and doesn't run the test suite, which I thought not worth the effort to somehow test.
  • We have a new cross-platform builder. It only compiles, but doesn't run tests. This will make sure we at least keep compiling on Solaris, CloudABI, FreeBSD, Fuchsia, NetBSD and Redox.
  • The FreeBSD builder that uses Trust wasn't doing any testing, so it was needlessly complicated.
  • I didn't see much use for the thumbv6m-none-eabi builder, we already test no_std support plenty?
  • Enabled running tests on ARMv7 Android using Trust.
  • Testing alloc in cargo test --features serde1,log,nightly,alloc was useless, because it is enabled by default.
  • Added a lengthy (and probably hard to follow) rationale 😄.

This is the most comprehensive set I could come up with, unless we start to make our own Qemu images of all supported OSes and start running tests on these. I.e. do a lot of work to set it up and maintain.

The number of Travis builders is reduced from 11 to 8, all usually done in < 4 minutes.

The 8 Travis builders:

  • pinned stable Rust release
  • stable Rust release, macOS, iOS (cross-compile only)
  • beta Rust release
  • nightly features, benchmarks, documentation
  • WASM via emscripten, stdweb and wasm-bindgen
  • cross-platform builder (doesn't run tests)
  • Linux (MIPS, big-endian)
  • Android (ARMv7)

@pitdicker pitdicker force-pushed the travis branch 2 times, most recently from 9a6139c to 132ca44 Compare July 23, 2018 06:17
dhardy
dhardy reviewed Jul 23, 2018
View reviewed changes
Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Few comments. I'll finish reviewing later. But thanks for doing this; it was very much needed!

.travis.yml Outdated
#
# OPERATING SYSTEMS
# Goal: test on many operating systems, to verify the OsRng code, which is
# mostly architecture-independend.
Copy link
Member

Choose a reason for hiding this comment

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

dependent

.travis.yml Outdated
# One builder cross-compiles for many of the remaining OSes, which ensures we
# keep building, but doesn't run tests.
# OSes supported by Rand but which we can't cross-compile because there
# is no pre-build standard library available: Dragonfly BSD, Haiku, OpenBSD.
Copy link
Member

Choose a reason for hiding this comment

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

pre-built

.travis.yml Outdated
# is no pre-build standard library available: Dragonfly BSD, Haiku, OpenBSD.
#
# CRATE FEATURES, TESTS, AND SUB-CRATES
# Goal: Run unit tests, doctests, examples, and benchmarks (converted to tests)
Copy link
Member

Choose a reason for hiding this comment

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

and test benchmarks

.travis.yml Outdated
# combinations of features.
# Tests run on rand:
# - test no_std support, but only the unit tests:
# `cargo test --lib --no-default-features`
Copy link
Member

Choose a reason for hiding this comment

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

Do we want --lib or --tests?

Copy link
Collaborator

@vks vks Jul 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

On master we have this comment:

        # TODO: use --tests instead of --lib on more recent compiler

So I guess it depends on the minimal Rust version we are supporting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They do exactly the same (because we are only a library), but --lib is available in Rust 1.22.0, --tests isn't.

@pitdicker
Copy link
Contributor Author

Thank you, maybe I should use the spell checker in gedit once in a while...

@vks
Copy link
Collaborator

vks commented Jul 23, 2018

Should we allow failures on nightly? I think those failures are currently a bit of a distraction...

@pitdicker
Copy link
Contributor Author

We are just unlucky at the moment. The alloc crate, wasm-bindgen and simd are all working towards stabilization, and shouldn't keep making breaking changes as often the the last few weeks.

The nightly builders also do extra work, like generating the (most complete) documentation and testing the benchmarks. I wouldn't want to see them easily ignored (we already ignore them anyway when the fix is somewhat outside our control).

@vks
Copy link
Collaborator

vks commented Jul 24, 2018

I think we should have a test that enables all features. Currently it is easy to add a new feature and forget to make sure the tests actually run.

@vks vks mentioned this pull request Jul 24, 2018
Merged
dhardy
dhardy approved these changes Jul 26, 2018
View reviewed changes
.travis.yml
# - test no_std support, but only the unit tests:
# `cargo test --lib --no-default-features`
# - run unit tests and doctests with all features which are available on stable:
# `cargo test --features=serde1,log`
Copy link
Member

Choose a reason for hiding this comment

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

i128_support is also available on recent stable compilers (though see comments in #566; this may get enabled by default)

.travis.yml Outdated
# `cargo test --examples`
# Additional tests on nightly:
# - run unit tests and doctests with all features which are available on nightly:
# `cargo test --features=serde1,log,nightly`
Copy link
Member

Choose a reason for hiding this comment

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

As @vks says, may make sense just to use --all-features here

.travis.yml Outdated
- cargo test --package rand_core --no-default-features
- cargo test --features serde1,log
- cargo test --package rand_isaac --features=serde1
# - cargo test --package rand_xorshift --features=serde1
Copy link
Member

Choose a reason for hiding this comment

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

This can be enabled now.

@dhardy dhardy added the D-changes Do: changes requested label Jul 27, 2018
@pitdicker
Copy link
Contributor Author

Rebased and updated.

@pitdicker pitdicker merged commit d8bea3a into rust-random:master Jul 27, 2018
@pitdicker pitdicker deleted the travis branch July 27, 2018 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vks vks vks left review comments

@dhardy dhardy dhardy approved these changes

Assignees
No one assigned
Labels
D-changes Do: changes requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pitdicker @vks @dhardy