Re: [RFC]: Skipping rust crate tests by default

[Efraim Flashner <efraim@flashner.co.il>]

[-- Attachment #1: Type: text/plain, Size: 7458 bytes --]

On Thu, Oct 05, 2023 at 03:52:46PM +0300, Efraim Flashner wrote:
> Currently for for rust crates we build the crates, run the tests, and
> then in %output we only have the license files and a repackaged version
> of the source.
> 
> The build system goes:
> unpack source
> unpack crates
> patch shebangs
> patch checksums of the crates
> 'build
> 'package
> 'check
> 'install
> 
> 'install is clear, it does whatever the install command is.
> 
> 'package repacks the source crate, after we've done any changes to it in
> the snippet and later if we've gone and patched paths to binaries or
> libraries. In theory this is useful with using these crates in a
> GUIX_ENVIRONMENT
> 
> 'check runs the test suite, which fairly often seems to need some
> massaging to skip the odd test which fails or to try to skip the doc
> tests, which fail far too often.
> 
> 'build sounds like it just builds the package. The first thing it does
> it makes sure that all the necessary crates are included in the build
> environment.
> 
> IMO the 'build phase is the most important one, it's the one that lets
> us know if all the cargo-inputs and cargo-development-inputs are
> correct. We don't care if rust-rand-0.6 or rust-nb-connect-1 builds, we
> only care that it has the correct inputs so that when we pull it in for
> an actual binary or library everything builds correctly.
> 
> I propose changing the cargo-build-system to have '#:tests? #f' by
> default and then enable them for packages which have a "clear output".
> It will keep the benefits of knowing we have the correct inputs without
> worrying about test errors we don't care about. If it fails to build
> during its own 'build phase that's actually worth looking into. It will
> also cut down the amount of time the CI spends building unneeded rust
> crates, and lets us see which ones are actually broken.

To add some more context from other places and to try to flesh out more:

On Tue, Oct 17, 2023 at 11:41:11AM -0400, Maxim Cournoyer wrote:
> Hi Efraim,
> 
> Efraim Flashner <efraim@flashner.co.il> writes:
> 
> > IMO rust-team branch is ready to merge. We've updated rust to 1.70,
> > librsvg to 2.56.4 and many new and updated packages. We've added a phase
> > to the cargo-build-system to fail if it detects pre-built files and
> > we've set the cargo-build-system to skip the test phase by default,
> > allowing us to make sure that the packages have the correct inputs. With
> > these changes I've gotten 100% of the packages built using the
> > cargo-build-system to build successfully.
> 
> This sounds good except I don't understand how disabling the tests by
> default help to "make sure that the packages have the correct inputs" ?

When the tests are disabled, if a package shows red on the CI it means
that either:
A) there was a bundled shared/static library in the sources which need
to be removed
B) The inputs weren't correct and need to be fixed.
What we're skipping is C) the test suite failed.

When we get to the 'build phase of the cargo-build-system, cargo first
checks that it has all of the crates listed in the Cargo.toml file, and
that all of those crates have all of their (cargo-input) dependencies,
and so on. If any of them are missing then the build will fail. This is
also why we need to set #:skip-build? #t when we don't include the
desired cargo-development-inputs.

The change is mainly a quality of life improvement; it decreases the
time that guix people and CI spend building these crates, and it makes
it easier to see which, if any, rust packages need to be checked for
brokenness (with the assumption that a broken or bit-rotted test suite
isn't a problem).

My premise is that the test suite of crates doesn't necessarily pass
when built and run with a newer rust and that we shouldn't be concerned
about it.

> You've explained the rationale here:
> <https://lists.gnu.org/archive/html/guix-devel/2023-10/msg00182.html>,
> saying we sometimes use a newer Rust than the package tests are
> expecting; how does it work in the Rust world?  Don't they always build
> even older versions against the most recent compiler?  What about the
> test suites then?  Are these not typically run by users/distributions?

In general, since rust expects all of the crates to be in source form,
the tests are only run by developers when the crate is being developed.
If someone comes along and uses that crate as a dependency for their
project then they don't run the tests. If they continue using that crate
(at that version) for several years then the developer using that older
crate as a dependency still only compiles the part of the crate they
need for their project and they only run the tests for their project,
not for the crates which they've used as dependencies.

As far as distributions, I can talk for Debian that they only provide
the crates as -dev packages, that is, as the source crates. They make
sure that they compile (and probably that they pass the test suite) at
the time that they are packaged, but no one is distributing pre-compiled
crates as packages to be used as inputs for further packages.


For an example of a failing doc-test, from the rust-nalgebra-0.21 crate:


   Doc-tests nalgebra
error: unnecessary parentheses around index expression
  --> /tmp/guix-build-rust-nalgebra-0.21.1.drv-0/nalgebra-0.21.1/src/linalg/convolution.rs:45:47
   |
45 |                 conv[i] += self[u_i] * kernel[(i - u_i)];
   |                                               ^       ^
   |
note: the lint level is defined here
  --> /tmp/guix-build-rust-nalgebra-0.21.1.drv-0/nalgebra-0.21.1/src/lib.rs:78:9
   |
78 | #![deny(unused_parens)]
   |         ^^^^^^^^^^^^^
help: remove these parentheses
   |
45 -                 conv[i] += self[u_i] * kernel[(i - u_i)];
45 +                 conv[i] += self[u_i] * kernel[i - u_i];
   |

error: unnecessary parentheses around index expression
  --> /tmp/guix-build-rust-nalgebra-0.21.1.drv-0/nalgebra-0.21.1/src/linalg/convolution.rs:49:53
   |
49 |                         conv[i] += self[u] * kernel[(i - u)];
   |                                                     ^     ^
   |
help: remove these parentheses
   |
49 -                         conv[i] += self[u] * kernel[(i - u)];
49 +                         conv[i] += self[u] * kernel[i - u];
   |

error: aborting due to 2 previous errors

error: doctest failed, to rerun pass `--doc`


crates.io lists this version as being released more than 3 years ago and
targeting the 2018 edition of rust. When built with our current
rust-1.68.2 the doc test passes but not with 1.70.0.  The current
upstream version of nalgebra is 0.32.3, so it's unlikely that they'd
release a new version with the doc tests fixed, but I haven't contacted
them about it.

> For one thing the 'guix lint' command would need to be told that
> cargo-build-system has #:tests? set to #f by default to not warn without
> reasons that '#:tests? #t' is unnecessary.

Instead of #:tests? #t I used (not (%current-target-system)), but I
hadn't tested it with guix lint to see if that would need to be changed.

-- 
Efraim Flashner   <efraim@flashner.co.il>   רנשלפ םירפא
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]