Migrate to GitHub Actions #1073

dhardy · 2020-12-08T12:52:48Z

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 · 2020-12-08T12:56:51Z

@newpavlov you may be able to help with this?

dhardy · 2020-12-08T16:55:23Z

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 · 2020-12-08T16:57:17Z

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 · 2020-12-09T22:55:25Z

Looks good!

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

rand_chacha/README.md

rand_core/README.md

rand_distr/README.md

rand_hc/README.md

rand_pcg/README.md

vks · 2020-12-09T23:04:09Z

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 · 2020-12-10T07:22:49Z

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 · 2020-12-10T11:38:32Z

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 · 2020-12-10T12:09:51Z

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

dhardy · 2020-12-10T12:21:26Z

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 · 2020-12-10T12:21:53Z

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 · 2020-12-10T12:33:21Z

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

newpavlov · 2020-12-10T12:52:57Z

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 · 2020-12-10T14:10:20Z

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.

According to the Cargo manifest, this section is not used and the README should be used for build status.

Incomplete: Linux 32-bit, Big-Endian, Web

…ences slightly

dhardy · 2020-12-13T14:17:59Z

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

vks · 2020-12-14T10:05:36Z

README.md

@@ -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


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

vks · 2020-12-14T10:14:02Z

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.

vks · 2020-12-14T10:15:29Z

.github/workflows/test.yml

            toolchain: nightly
-            minimal: true
+            variant: minimal


Maybe "minimal_versions" is clearer?

dhardy · 2020-12-14T11:07:03Z

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?

vks · 2020-12-14T12:14:25Z

rand_distr/src/normal.rs

@@ -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);


This one does not give a nice error message.

vks · 2020-12-14T12:15:22Z

rand_distr/src/cauchy.rs

@@ -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);


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

vks · 2020-12-14T12:17:51Z

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 · 2020-12-14T12:55:49Z

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

dhardy · 2020-12-14T13:45:25Z

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

vks

Thanks, looks good now!

dhardy force-pushed the work2 branch from d913a49 to cc5726a Compare December 8, 2020 15:45