Bug 1983175 - Review Request: google-crc32c - CRC32C implementation with support for CPU-specific acceleration instructions
Summary: Review Request: google-crc32c - CRC32C implementation with support for CPU-sp...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1983176 1983181
TreeView+ depends on / blocked
 
Reported: 2021-07-16 18:06 UTC by Major Hayden 🤠
Modified: 2021-08-23 17:29 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2021-08-23 17:29:56 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Major Hayden 🤠 2021-07-16 18:06:42 UTC
Spec URL: https://download.copr.fedorainfracloud.org/results/mhayden/python-google-cloud/fedora-rawhide-x86_64/02331964-google-crc32c/google-crc32c.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/mhayden/python-google-cloud/fedora-rawhide-x86_64/02331964-google-crc32c/google-crc32c-1.1.1-1.fc35.src.rpm
Description: CRC32C implementation with support for CPU-specific acceleration instructions
Fedora Account System Username: mhayden

COPR fedora-review: https://download.copr.fedorainfracloud.org/results/mhayden/python-google-cloud/fedora-rawhide-x86_64/02331964-google-crc32c/fedora-review

This is used along with python-google-crc32c for Google Cloud's storage client (and resumable uploads).

Comment 1 Lukáš Zaoral 2021-07-30 10:33:32 UTC
Hi Major,
note, that I'm not a member of the packager group yet and this is one of the reviews I have to write to become one. Therefore, some of my suggestions or remarks may not be perfect. Thank you for understanding and patience until some official member responds to your review request (and reviews my review).

The spec file looks great! I have only a few remarks:

* I see that you explicitly disable bundled unit tests. You should try to enable them or provide a reason for not doing so. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites.

* Could you provide a Koji build to demonstrate that all architectures can be built successfully? (Your COPR build is x86_64 only.)

* rpmlint issues following warning: library-not-linked-against-libc /usr/lib64/libcrc32c.so.1.1.0

* Just a nitpick: It may be useful to replace all occurrences of google-crc32c by %{name}.

Comment 2 Major Hayden 🤠 2021-07-30 13:39:46 UTC
(In reply to Lukáš Zaoral from comment #1)
> Hi Major,
> note, that I'm not a member of the packager group yet and this is one of the
> reviews I have to write to become one. Therefore, some of my suggestions or
> remarks may not be perfect. Thank you for understanding and patience until
> some official member responds to your review request (and reviews my review).

Everyone starts somewhere! 🎉 Thanks for having a look.

> The spec file looks great! I have only a few remarks:
> 
> * I see that you explicitly disable bundled unit tests. You should try to
> enable them or provide a reason for not doing so. See
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_test_suites.

The Arch Linux folks disabled them, too, and it's because the repo brings in extra requirements via submodules which end up being installed in the final package. Even if you package 'googletest', the cmake stuff will need to be heavily patched to use it. I'll add some commentary in there about that.

> * Could you provide a Koji build to demonstrate that all architectures can
> be built successfully? (Your COPR build is x86_64 only.)

Here's a new COPR build from this morning with everything included:

  https://copr.fedorainfracloud.org/coprs/mhayden/python-google-cloud/build/2353456/

I sent a scratch build through Koji, too:

  https://koji.fedoraproject.org/koji/taskinfo?taskID=72972061

> * rpmlint issues following warning: library-not-linked-against-libc
> /usr/lib64/libcrc32c.so.1.1.0

I'm not entirely sure how to correct this or how big of an issue it might be.

> * Just a nitpick: It may be useful to replace all occurrences of
> google-crc32c by %{name}.

Good point. I'll go back through and use that more often.

Comment 3 Major Hayden 🤠 2021-07-30 14:25:17 UTC
(In reply to Lukáš Zaoral from comment #1)
> * rpmlint issues following warning: library-not-linked-against-libc
> /usr/lib64/libcrc32c.so.1.1.0

For what it's worth, this warning doesn't seem to be a huge problem from some other reviews I've read. Here's an example: https://bugzilla.redhat.com/show_bug.cgi?id=1968612

Comment 5 Neal Gompa 2021-08-21 11:54:08 UTC
Review notes:

- Packaging follows naming guidelines
- Package licensing is indicated correctly
- Packaging builds and installs
- No serious issues from rpmlint

PACKAGE APPROVED.

Comment 6 Gwyn Ciesla 2021-08-23 13:48:14 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/google-crc32c

Comment 7 Fedora Update System 2021-08-23 17:29:56 UTC
FEDORA-2021-44fc830e78 has been pushed to the Fedora 36 stable repository.
If problem still persists, please make note of it in this bug report.


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