Bug 2116083 - Review Request: rust-primal - Put raw power into prime numbers
Summary: Review Request: rust-primal - Put raw power into prime numbers
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On: 2116065 2116066 2116079 2116081
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-08-06 22:05 UTC by Orion Poplawski
Modified: 2022-11-13 19:16 UTC (History)
2 users (show)

Fixed In Version: rust-primal-0.3.1-3.fc38
Clone Of:
Environment:
Last Closed: 2022-11-10 01:05:42 UTC
Type: Bug
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Orion Poplawski 2022-08-06 22:05:47 UTC
Spec URL: https://orion.fedorapeople.org/rust-primal.spec
SRPM URL: https://orion.fedorapeople.org/rust-primal-0.3.0-1.fc37.src.rpm
Description:
`primal` puts raw power into prime numbers. This crates includes: optimised
prime sieves, checking for primality, enumerating primes, factorising numbers,
and state-of-the-art estimation of upper and lower bounds for π(n) (the number
of primes below n) and p_k (the k-th prime).

Fedora Account System Username: orion

Comment 1 Fabio Valentini 2022-08-06 22:30:34 UTC
It looks like the URLs are no longer valid.
I'm curious what you're packaging the "primal" crates for?

Comment 2 Orion Poplawski 2022-08-07 00:42:34 UTC
URLs should be working now.  It's coming in from deps for clamav, but I suspect all I may really need is primal-check for rustfft (the rest comes in for the test deps in primal-check).

Comment 3 Fabio Valentini 2022-08-07 19:00:29 UTC
(In reply to Orion Poplawski from comment #2)
> URLs should be working now.  It's coming in from deps for clamav, but I
> suspect all I may really need is primal-check for rustfft (the rest comes in
> for the test deps in primal-check).

If that is the case, I would consider not running the test suite.
It looks like some of the crates that this is pulling in are *really really old* - for example, "hamming" was last updated 7 years ago.

Comment 4 Orion Poplawski 2022-08-16 01:17:50 UTC
Upstream now provides the license files.

Spec URL: https://orion.fedorapeople.org/rust-primal.spec
SRPM URL: https://orion.fedorapeople.org/rust-primal-0.3.1-1.fc38.src.rpm

Comment 5 Fabio Valentini 2022-10-20 08:30:48 UTC
It looks like the build for rust-primal-sieve ended up untagged in koji:
https://koji.fedoraproject.org/koji/buildinfo?buildID=2078635

You might want to file a releng / fedora-infra ticket about that ...

Comment 6 Fabio Valentini 2022-10-20 08:34:43 UTC
Other initial comments:

1. Please remove markdown markup from the package's Summary and this bug's title.

For example, the Summary could just be "Put raw power into prime numbers", as repeating the package's name in the summary is considered "bad".

2. Ensure to always regenerate Rust packages' .spec files for all new versions.

Any new version might change a crates optional dependencies or features, and since rust2rpm needs to generate a subpackage for every one of these, it needs to be re-run for all versions to keep optional dependencies / features and RPM subpackages in sync.

(Good thing we have rpmautospec now ... keeping %changelog around while regenerating the spec was a nightmare before that.)

Comment 7 Orion Poplawski 2022-10-21 14:20:37 UTC
I've fixed the summary and description.  Not sure what was up with the hung rust-primal-sieve, but I've built a new version and that seems to be present now.

Comment 8 Fabio Valentini 2022-11-04 16:01:18 UTC
Package looks good to me, with one exception:
Please use "rust2rpm -p" to remove the "criterion" dependency.
It is unused and only inflates the dependency tree and build times.

===

Package was generated with rust2rpm, simplifying the review.

- package builds and installs without errors on rawhide
- test suite is run and all unit tests pass
- latest version of the crate is packaged
- license matches upstream specification (MIT OR Apache-2.0) and is acceptable for Fedora
- license files are included with %license in %files
- package complies with Rust Packaging Guidelines

Package APPROVED.

===

Recommended post-import rust-sig tasks:

- add @rust-sig with "commit" access as package co-maintainer

- set bugzilla assignee overrides to @rust-sig (optional)

- set up package on release-monitoring.org:
  project: $crate
  homepage: https://crates.io/crates/$crate
  backend: crates.io
  version scheme: semantic
  version filter: alpha;beta;rc;pre
  distro: Fedora
  Package: rust-$crate

- track package in koschei for all built branches

Comment 9 Orion Poplawski 2022-11-05 01:18:48 UTC
Spec URL: https://orion.fedorapeople.org/rust-primal.spec
SRPM URL: https://orion.fedorapeople.org/rust-primal-0.3.0-1.fc38.src.rpm

Removed criterion.

Thanks for the review.

Comment 10 Jens Petersen 2022-11-07 10:48:18 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-primal

Comment 11 Orion Poplawski 2022-11-09 05:15:49 UTC
Getting a build failure on i686:

     Running `/usr/bin/rustdoc --edition=2018 --crate-type lib --crate-name primal --test /builddir/build/BUILD/primal-0.3.1/src/lib.rs -L dependency=/builddir/build/BUILD/primal-0.3.1/target/release/deps -L dependency=/builddir/build/BUILD/primal-0.3.1/target/release/deps --extern primal=/builddir/build/BUILD/primal-0.3.1/target/release/deps/libprimal-11197fdc66de5d7d.rlib --extern primal_check=/builddir/build/BUILD/primal-0.3.1/target/release/deps/libprimal_check-e49fff9074ba3a42.rlib --extern primal_estimate=/builddir/build/BUILD/primal-0.3.1/target/release/deps/libprimal_estimate-175af939340fd196.rlib --extern primal_sieve=/builddir/build/BUILD/primal-0.3.1/target/release/deps/libprimal_sieve-79764c267e05f357.rlib --extern primal_slowsieve=/builddir/build/BUILD/primal-0.3.1/target/release/deps/libprimal_slowsieve-e2398735e9434eb3.rlib -C embed-bitcode=no --error-format human`
running 10 tests
test src/lib.rs - (line 101) ... FAILED
test src/lib.rs - (line 179) ... ok
test src/lib.rs - (line 164) ... ok
test src/lib.rs - (line 239) ... ok
test src/lib.rs - (line 191) ... ok
test src/lib.rs - (line 207) ... ok
test src/lib.rs - (line 67) - compile ... ok
test src/lib.rs - (line 136) ... ok
test src/lib.rs - (line 51) ... ok
test src/lib.rs - (line 39) ... ok
failures:
---- src/lib.rs - (line 101) stdout ----
error: literal out of range for `usize`
  --> src/lib.rs:116:17
   |
18 | assert_eq!(sum, 8795091674);
   |                 ^^^^^^^^^^
   |
   = note: `#[deny(overflowing_literals)]` on by default
   = note: the literal `8795091674` does not fit into the type `usize` whose range is `0..=4294967295`
error: aborting due to previous error
Couldn't compile the test.
failures:
    src/lib.rs - (line 101)
test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.43s
error: doctest failed, to rerun pass `--doc`
error: 1 target failed:
    `--doc`


I've filed this upstream: https://github.com/huonw/primal/issues/53

Comment 12 Fabio Valentini 2022-11-09 16:48:59 UTC
I don't understand why the type of that literal is inferred to be "usize". Using an u64 instead should be fine on all architectures, I think.

Still, this looks like an obvious mistake in the test, so you can safely skip it during package builds for now.
You should be able to use this instead of "%cargo_check" to skip doctests from the src/lib.rs file on 32-bit targets:

%ifarch %{ix86} %{arm}
%cargo_check -- -- --skip "src/lib.rs"
%else
%cargo_check
%endif

Comment 13 Orion Poplawski 2022-11-10 01:05:42 UTC
THanks.  Checked in and built.

Comment 14 Fabio Valentini 2022-11-13 19:16:51 UTC
Meh, I just noticed that I typo'ed %cargo_check instead of %cargo_test. Sorry about that.


Note You need to log in before you can comment on or make changes to this bug.