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

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

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

On Wed, Oct 18, 2023 at 02:46:57PM -0400, Maxim Cournoyer wrote:
> Hi Efraim,
> 
> Efraim Flashner <efraim@flashner.co.il> writes:
> 
> [...]
> 
> >> 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).
> 
> I understand that maintaining the large fleet of cargo crates packaged
> in Guix is a lot of work, but I think we can try some things before
> #:tests? #f; I gathered some idea below.
> 
> > 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.
> 
> OK.
> 
> > 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.
> 
> I believe that's the more useful comparison for our discussion; I gather
> that even -dev crates are built and have their test suite run when
> submitted, but since source packages are not rebuilt (they are a static
> archive, right?) then there's no checking that the test suite continues
> working in time.

The source packages are effectively repackaged tarballs. They've made
sure there's no issues with the DFSG, maybe applied some patches, built
and tested the crates, and then repackaged the tarballs.

> >
> > 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.
> 
> OK.  Asking in ##rust on libera chat (unofficial channel), I got as suggestion
> to call 'carge test' with the '--cap-lints=allow' option documented
> here [0], which should effectively disable just the lint checks, which
> is better than disabling the full test suite.
> 
> [0]  https://doc.rust-lang.org/rustc/lints/levels.html

I checked the cargo-build-system and we do actually use
--cap-lints=allow for building and for testing.

> >> 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.
> 
> That means tests are disabled only when cross-compiling, right?  Do we
> support cross-compiling rust crates?

For those packages, yes. For all the others they would've been disabled
for native and for cross builds.

I'm currently working on cross-building rust packages but have been
running into some problems with llvm or with rustc/cargo itself. I hope
to have it working relatively soon.

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