Bug 1983175

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}.

(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.

(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

Spec URL: https://download.copr.fedorainfracloud.org/results/mhayden/python-google-cloud/fedora-rawhide-x86_64/02531054-google-crc32c/google-crc32c.spec
SRPM URL: https://download.copr.fedorainfracloud.org/results/mhayden/python-google-cloud/fedora-rawhide-x86_64/02531054-google-crc32c/google-crc32c-1.1.1-1.fc36.src.rpm

Review notes:

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

PACKAGE APPROVED.

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/google-crc32c

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.