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

Migrate to GitHub Actions #1073

Merged
merged 8 commits into from Dec 14, 2020
Merged

Migrate to GitHub Actions #1073

merged 8 commits into from Dec 14, 2020

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Dec 8, 2020

Closes #1066

Incomplete: 32-bit Linux runner, Big-Endian runner, web (was already broken or removed but script left in utils/ci).

Fingers crossed most of this works...

@dhardy
Copy link
Member Author

dhardy commented Dec 8, 2020

@newpavlov you may be able to help with this?

@dhardy
Copy link
Member Author

dhardy commented Dec 8, 2020

Looks like we can use this for MUSL: https://github.com/NSCoder/rust-musl-action
Is this build-only or does it support running, and if so does one need to specify the target?

And for MIPS (or a BE target)... maybe use https://github.com/rust-embedded/cross
Any tips on how?

@dhardy
Copy link
Member Author

dhardy commented Dec 8, 2020

I don't remember a good reason to test MUSL. Maybe to test portability of math results. MIPS is more important in that we need to test on a Big Endian target. So we don't necessarily need exactly the above targets.

@vks
Copy link
Collaborator

vks commented Dec 9, 2020

Looks good!

The true or false in the job name corresponds to minimal versions?

rand_chacha/README.md Outdated Show resolved Hide resolved
rand_core/README.md Outdated Show resolved Hide resolved
rand_distr/README.md Outdated Show resolved Hide resolved
rand_hc/README.md Outdated Show resolved Hide resolved
rand_pcg/README.md Outdated Show resolved Hide resolved
@vks
Copy link
Collaborator

vks commented Dec 9, 2020

I don't remember a good reason to test MUSL. Maybe to test portability of math results. MIPS is more important in that we need to test on a Big Endian target. So we don't necessarily need exactly the above targets.

The MUSL target was introduced by @newpavlov for our 32-bit tests in 71d222c. I'm not sure why MUSL was chosen in particular.

I agree that a Big Endian platform is important for testing.

@newpavlov
Copy link
Member

Looks good overall! I don't have experience of using VM (which IIUC will be needed for BE testing) with GitHub Actions, so I will not be able to help with that.

Shouldn't we remove the Travis config completely or do you plan to keep it for now as a list of not ported tests? Also it may be useful to split tests of each crate into separate files. This way number of tests can be much smaller if only one crate gets affected. In RustCrypto we test each crate separately (see the paths field), though in rand's case I guess it will be less beneficial due to the crates interdependencies. Either way, it can be done later in a separate PR.

I'm not sure why MUSL was chosen in particular.

I don't remember exactly, but I probably had a problem with dynamically linked libc if program was compiled for i686. With GA it can be solved by adding the gcc-multilib dependency.

@dhardy
Copy link
Member Author

dhardy commented Dec 10, 2020

The true or false in the job name corresponds to minimal versions?

Yes.

Shouldn't we remove the Travis config completely or do you plan to keep it for now as a list of not ported tests?

I was assuming we'd be able to remove it completely in this PR, and probably utils/ci too. I may just create an issue for the missing tests and leave them out here (but still remove the old config).

There is a paths field? Doing this would actually increase the number of runners though, and since the rand tests need to be run for everything it seems to make little sense... with the exception of rand_distr. But then rand_distr tests in theory need to be run on any other changes and avoiding rand tests for rand_distr changes is not such a big deal I think (assuming we still want to test all targets for rand_distr which I think we do). So I think the savings are just not worth the complexity (esp. since running the tests and the extra compilation over rand is pretty quick relative to everything else).

@dhardy
Copy link
Member Author

dhardy commented Dec 10, 2020

Any idea why only two variants of the test matrix get run?

@dhardy
Copy link
Member Author

dhardy commented Dec 10, 2020

thread 'cauchy::test::value_stability' panicked at 'expected: -5.446415 = -5.446413', rand_distr/src/cauchy.rs:163:13

I call this a success for testing! Not entirely sure why it didn't fail before, but we didn't previously test i686-unknown-linux-gnu (only MUSL variant).

@newpavlov
Copy link
Member

newpavlov commented Dec 10, 2020

Shouldn't the matrix look like this?

matrix:
  - os: [ubuntu-latest, windows-latest, macos-latest]
  - toolchain: [stable, beta, nightly]
  - include:
    - os: windows-latest
      toolchain: nightly
      target: i686-unknown-linux-gnu
      deps: sudo apt install gcc-multilib

IIUC the include field only adds jobs which match one of combinations in the original matrix (UPD: never mind, adding non-matching job is literally the next example in the docs). Also I think you probably can use a default target for most jobs.

@dhardy
Copy link
Member Author

dhardy commented Dec 10, 2020

Er... now the test matrix is empty??

@newpavlov newpavlov mentioned this pull request Dec 10, 2020
@newpavlov
Copy link
Member

I can't say at a glance why it works like this, but the simplified config in #1075 works as expected. Try to gradually extend it?

@dhardy
Copy link
Member Author

dhardy commented Dec 10, 2020

According to the docs, the include option can add values to an existing combination as well as including new combinations: the mode seems to depend only on whether existing key-values are used. Previously these examples did not select the minimal key, except for one specific case. Removing this key and using include sections to set all values seems to be the clearest solution.

@dhardy
Copy link
Member Author

dhardy commented Dec 13, 2020

Hopefully the last changeset finally fixes the tests. @vks @newpavlov you might wish to review the switch to approximate testing in value stability.

README.md Outdated
@@ -104,8 +103,8 @@ greater, and 0.4 and 0.3 (since approx. June 2017) require Rustc version 1.15 or
greater. Subsets of the Rand code may work with older Rust versions, but this is
not supported.

Travis CI always has a build with a pinned version of Rustc matching the oldest
supported Rust release. The current policy is that this can be updated in any
Continuous Integration (CI) will always test the oldest supported Rustc version
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe replace "oldest" with "minimum" for clarity?

@vks
Copy link
Collaborator

vks commented Dec 14, 2020

I would prefer to define a macro for approximate testing:

/// Assert that two numbers are almost equal to each other.
///
/// On panic, this macro will print the values of the expressions with their
/// debug representations.
#[macro_export]
macro_rules! assert_almost_eq {
    ($a:expr, $b:expr, $prec:expr) => (
        let diff = ($a - $b).abs();
        if diff > $prec {
            panic!(format!(
                "assertion failed: `abs(left - right) = {:.1e} < {:e}`, \
                 (left: `{}`, right: `{}`)",
                diff, $prec, $a, $b));
        }
    );
}

This gives a better error message than assert((a - b).abs() < prec) and may make the ApproxEq trait unnecessary.

toolchain: nightly
minimal: true
variant: minimal
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe "minimal_versions" is clearer?

@dhardy
Copy link
Member Author

dhardy commented Dec 14, 2020

The ApproxEq trait is there because I got fed up adding tolerances to each usage. We need appropriate values for f32, f64 and u64. Thus, we need a trait. Also, my approach should already have good enough error messages?

@@ -345,7 +345,8 @@ mod tests {
assert_almost_eq!(lnorm.norm.std_dev, 1.0, 2e-16);

let lnorm = LogNormal::from_mean_cv(e.powf(1.5), (e - 1.0).sqrt()).unwrap();
assert_eq!((lnorm.norm.mean, lnorm.norm.std_dev), (1.0, 1.0));
assert!((lnorm.norm.mean - 1.0).abs() < 1e-15);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one does not give a nice error message.

@@ -160,7 +160,7 @@ mod test {
let expected = [15.023088, -5.446413, 3.7092876, 3.112482];
for (a, b) in buf.iter().zip(expected.iter()) {
let (a, b) = (*a, *b);
assert!((a - b).abs() < 1e-6, "expected: {} = {}", a, b);
assert!((a - b).abs() < 1e-5, "expected: {} = {}", a, b);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This also gives a worse error message: it does not tell the difference/error.

@vks
Copy link
Collaborator

vks commented Dec 14, 2020

Also, my approach should already have good enough error messages?

Your approach requires a lot of repetition and it is not very consistent. As far as I see it does not give the absolute error, which makes it harder to adjust the error threshold if a test is failing.

@dhardy
Copy link
Member Author

dhardy commented Dec 14, 2020

Alright, I'll add your macro but keep the trait too.

@dhardy
Copy link
Member Author

dhardy commented Dec 14, 2020

Turns out no new macro definitions are required... thus we obviously should have used this before!

Copy link
Collaborator

@vks vks left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now!

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

Successfully merging this pull request may close these issues.

CI: https://rust-random.github.io/rand is outdated
3 participants