Bug 2116087 - Review Request: rust-strength_reduce - Faster integer division and modulus operations
Summary: Review Request: rust-strength_reduce - Faster integer division and modulus op...
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:
Blocks: 2116088 2116092
TreeView+ depends on / blocked
 
Reported: 2022-08-06 22:29 UTC by Orion Poplawski
Modified: 2022-11-03 02:48 UTC (History)
2 users (show)

Fixed In Version: rust-strength_reduce-0.2.3-1.fc38
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2022-11-03 02:48:16 UTC
Type: Bug
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Orion Poplawski 2022-08-06 22:29:22 UTC
Spec URL: https://orion.fedorapeople.org/rust-strength_reduce.spec
SRPM URL: https://orion.fedorapeople.org/rust-strength_reduce-0.2.3-1.fc37.src.rpm
Description:
Faster integer division and modulus operations.

Fedora Account System Username: orion

Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=90548736

Comment 1 Fabio Valentini 2022-08-07 18:54:12 UTC
> Source1:        http://www.apache.org/licenses/LICENSE-2.0
> Source2:        http://opensource.org/licenses/MIT

These are both wrong - they point at HTML pages, not at raw text files.
Additionally, the link for MIT doesn't resolve, because that site is now HTTPS only.

You should instead file a PR with the upstream project to add the license files (not HTML dumps :), and then you can link the files from your PR.
You can look at how must Rust crates (for example, https://github.com/seanmonstar/reqwest) handle this for "inspiration".
For example, the "LICENSE-APACHE" and "LICENSE-MIT" file names are pretty standardized across the whole ecosystem.

> # Update version deps
> # https://github.com/ejmahler/strength_reduce/pull/6
> Patch:          rust-strength_reduce-deps.patch

You might be interested in rust2rpm's "-p" flag, which automates generation of patches like this one.

Please also include a link to the PR that you filed upstream for this.

> %files          devel
> %license LICENSE-2.0 MIT

This should be:

%files          devel
%license %{crate_instdir}/LICENSE-APACHE
%license %{crate_instdir}/LICENSE-MIT

To match what rust2rpm generates if these files are present.

It also looks like you uploaded an SRPM file that was already mangled by rpmautospec:

> %changelog
> * Sat Aug 06 2022 John Doe <packager> 0.2.3-1
> - Uncommitted changes

I recommend to use "rpmbuild -bs" to build SRPM files for package review.
"fedpkg srpm" will do rpmautospec pre-processing, which is not what you want for the package review.

Comment 2 Orion Poplawski 2022-08-07 22:01:46 UTC
(In reply to Fabio Valentini from comment #1)
> > Source1:        http://www.apache.org/licenses/LICENSE-2.0
> > Source2:        http://opensource.org/licenses/MIT
> 
> These are both wrong - they point at HTML pages, not at raw text files.
> Additionally, the link for MIT doesn't resolve, because that site is now
> HTTPS only.
> 
> You should instead file a PR with the upstream project to add the license
> files (not HTML dumps :), and then you can link the files from your PR.
> You can look at how must Rust crates (for example,
> https://github.com/seanmonstar/reqwest) handle this for "inspiration".
> For example, the "LICENSE-APACHE" and "LICENSE-MIT" file names are pretty
> standardized across the whole ecosystem.

Done.

> > # Update version deps
> > # https://github.com/ejmahler/strength_reduce/pull/6
> > Patch:          rust-strength_reduce-deps.patch
> 
> You might be interested in rust2rpm's "-p" flag, which automates generation
> of patches like this one.

Good to know, thanks.

> Please also include a link to the PR that you filed upstream for this.

Already there.

> It also looks like you uploaded an SRPM file that was already mangled by
> rpmautospec:
> 
> > %changelog
> > * Sat Aug 06 2022 John Doe <packager> 0.2.3-1
> > - Uncommitted changes

Mainly just not having set %packager I think.

Comment 3 Fabio Valentini 2022-08-17 10:37:39 UTC
> Mainly just not having set %packager I think.

No, that's a different problem.

Thanks for the other fixes, re-running checks now ...

Comment 4 Fabio Valentini 2022-08-17 10:41:34 UTC
Looks like you copy-pasted MIT license with a copyright statement from a different project / a different author for your license file PR?

Comment 5 Orion Poplawski 2022-08-18 02:52:53 UTC
Hmm, indeed - didn't realize the copyright holder was mentioned in the license files.  I've updated to be similar to one of his other projects.  Hard to say if that is appropriate though.

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

Comment 6 Fabio Valentini 2022-08-18 09:59:38 UTC
In this case, I'd rather like to be on the safe side, and wait for upstream response, before we ship license files that might not match what they want or intend.

Comment 7 Orion Poplawski 2022-10-27 23:42:51 UTC
Unfortunately I'm getting no response from upstream.

Comment 8 Fabio Valentini 2022-10-31 12:56:00 UTC
That's unfortunate. However, as far as I can tell, you included standard license texts for the licenses that were intended by upstream (since the MIT and Apache-2.0 SPDX identifiers are unique), so I'm not going to block on this if upstream is non-responsive.

===

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 and is acceptable for Fedora
- license files are included with %license in %files (manually included from proposed upstream change)
- 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 Gwyn Ciesla 2022-11-01 15:35:28 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-strength_reduce

Comment 10 Orion Poplawski 2022-11-03 02:48:16 UTC
Checked in and built.  Thanks all.


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