Spec URL: https://sagitter.fedorapeople.org/google-cpu_features/google-cpu_features.spec SRPM URL: https://sagitter.fedorapeople.org/google-cpu_features/google-cpu_features-0.6.0-1.fc33.src.rpm Description: 'cpu_features' a new dependency of next 'COPASI' release. Fedora Account System Username: sagitter
Looks pretty clean at first glance, except for a couple of things related to shared library versioning: Per https://docs.fedoraproject.org/en-US/packaging-guidelines/#_downstream_so_name_versioning, you “must” to try to convince upstream to start doing versioned shared libraries before considering adding versions downstream. (Yeah, I know, it’s Google, good luck—but this upstream seems to have listened to similar kinds of requests in the past, like making versioned releases at all.) Maybe somebody already tried and I just could not find it… Per https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files, you “should” explicitly put the so-version in the file globs rather than using “libfoo.so*”. (This seems less useful when you are versioning downstream, of course.)
Taking review. SPEC looks clean. While one of the headers is under BSD license, it doesn't seem to be used for build. $ licensecheck -r .|grep BSD ./ndk_compat/cpu-features.h: BSD 2-clause "Simplified" License So, license is OK (ASL 2.0) Source matches src.rpm: $ sha512sum cpu_features-0.6.0.tar.gz cpu_features-0.6.0.tar.gz.orig 006a2e05253712cf605ecabccdda63dd9325445f8d145d5e2432c4342332e652f318810997321849be131082db435d88143020fdc85268fba204586cf37eef0d cpu_features-0.6.0.tar.gz 006a2e05253712cf605ecabccdda63dd9325445f8d145d5e2432c4342332e652f318810997321849be131082db435d88143020fdc85268fba204586cf37eef0d cpu_features-0.6.0.tar.gz.orig Please put the SO version explicitly in the file list, like the previous commenter said. rpmlint looks ok. $ rpmlint . google-cpu_features-debugsource.x86_64: W: spelling-error Summary(en_US) cpu -> CPU, cup, cu google-cpu_features-debugsource.x86_64: W: spelling-error %description -l en_US cpu -> CPU, cup, cu google-cpu_features-debuginfo.x86_64: W: spelling-error Summary(en_US) cpu -> CPU, cup, cu google-cpu_features-debuginfo.x86_64: W: spelling-error %description -l en_US cpu -> CPU, cup, cu google-cpu_features.x86_64: W: no-manual-page-for-binary list_cpu_features google-cpu_features-devel.x86_64: W: spelling-error Summary(en_US) cpu -> CPU, cup, cu google-cpu_features-devel.x86_64: W: summary-not-capitalized C google-cpu_features headers and development-related files google-cpu_features-devel.x86_64: W: spelling-error %description -l en_US cpu -> CPU, cup, cu google-cpu_features-devel.x86_64: W: no-documentation 5 packages and 0 specfiles checked; 0 errors, 9 warnings. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_build_failures Please open a ticket with upstream asking for s390x support, too.
Some more nitpicking: I prefer to list BuildRequires one per line and sorted alphabetically. That makes them more readable and makes the future diffs smaller in case you need to change them. You should use BuildRequires: cmake instead of cmake3 if you're not planning an EPEL7 package, especially since you're not using %cmake3_* macros, only %cmake_*. Either way, please be consistent here. Is there a particular reason not to name this simply "cpu_features" without the google- prefix? Also, please add an empty line before %description devel. Regarding %build section, -DBUILD_SHARED_LIBS:BOOL=ON and -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON are already supplied by %cmake macro, no need to duplicate. CMakeLists.txt has this: if(BUILD_SHARED_LIBS AND UNIX) set(BUILD_PIC ON) endif() so it looks like -DBUILD_PIC:BOOL=ON is redundant as well. Similar to BuildRequires, it's better to put each argument to %cmake on a separate line, like this: %cmake \ -DBUILD_TESTING:BOOL=ON \ -DCMAKE_BUILD_TYPE:STRING=Release \ -DCMAKE_SKIP_INSTALL_RPATH:BOOL=YES \ -DCPUFEATURES_VERSION_MAJOR:STRING=0 \ -DCPUFEATURES_VERSION:STRING=0.6 \ %cmake_build I'd also add CONTRIBUTING.md as %doc to -devel package.
Ping?
Thank you for the review Dominik. I'll update the package soon.
> Please open a ticket with upstream asking for s390x support, too. Supported architectures are explicitly listed by Upstream: https://github.com/google/cpu_features#support > Is there a particular reason not to name this simply "cpu_features" without the google- prefix? There are other google-* package (like google-bechmark) in Fedora, and they come from the same Google Open Source repositories; it's a correct way to name this package in my opinion.
Spec URL: https://sagitter.fedorapeople.org/google-cpu_features/google-cpu_features.spec SRPM URL: https://sagitter.fedorapeople.org/google-cpu_features/google-cpu_features-0.6.0-2.fc33.src.rpm
(In reply to Antonio T. sagitter from comment #6) > > Please open a ticket with upstream asking for s390x support, too. > > Supported architectures are explicitly listed by Upstream: > https://github.com/google/cpu_features#support That doesn't mean you can't open an issue asking about it. From what I can tell, nobody has done that so far. > > Is there a particular reason not to name this simply "cpu_features" without the google- prefix? > > There are other google-* package (like google-bechmark) in Fedora, and they > come from the same Google Open Source repositories; it's a correct way to > name this package in my opinion. Alright.
(In reply to Antonio T. sagitter from comment #7) > Spec URL: > https://sagitter.fedorapeople.org/google-cpu_features/google-cpu_features. > spec > SRPM URL: > https://sagitter.fedorapeople.org/google-cpu_features/google-cpu_features-0. > 6.0-2.fc33.src.rpm Looks good now, APPROVED.
Thank you.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/google-cpu_features