Spec URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad.spec SRPM URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad-2024.02.16-1.fc42.src.rpm Description: A set of client and server components which implement a crash-reporting system Fedora Account System Username: errornointernet
Copr build: https://copr.fedorainfracloud.org/coprs/build/9157188 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2372509-breakpad/fedora-rawhide-x86_64/09157188-breakpad/fedora-review/review.txt Found issues: - Package has .a files: breakpad-devel. Does not provide -static: breakpad-devel. Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/#packaging-static-libraries Please know that there can be false-positives. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
New spec URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad.spec New SRPM URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad-2024.02.16-1.fc42.src.rpm
Created attachment 2093811 [details] The .spec file difference from Copr build 9157188 to 9157397
Copr build: https://copr.fedorainfracloud.org/coprs/build/9157397 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2372509-breakpad/fedora-rawhide-x86_64/09157397-breakpad/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Taking this review.
Initial spec review notes: > Source0: %{url}/+archive/v%{version}.tar.gz Please make this a named tarball like so: "%{url}/+archive/v%{version}.tar.gz#/%{name}-%{version}.tar.gz" > Source1: https://chromium.googlesource.com/linux-syscall-support/+archive/v2024.02.01.tar.gz Please make this a named tarball like so: "https://chromium.googlesource.com/linux-syscall-support/+archive/v2024.02.01.tar.gz/#linux-syscall-support-2024.02.01.tar.gz" You may want to make a macro for the version so you can centrally control it. My suggestion is to set "%{lss_version}" at the top of the spec and use it in here. > tar xf %{SOURCE0} > mkdir -p src/third_party/lss > tar xf %{SOURCE1} -C src/third_party/lss Please use one of the setup macros for Source0, as it also does some background setup stuff for the build environment. Here's my suggestion: %autosetup -C tar xvf %{SOURCE1} -C src/third_party/lss (tar will create the directories for you) > %build > export CXXFLAGS="$CXXFLAGS -Wno-error=array-bounds -Wno-maybe-uninitialized" > %configure > %make_build Configure step should be split into the %conf stage unless you plan to support EPEL9 or older. This is what it would look like: %conf export CXXFLAGS="$CXXFLAGS -Wno-error=array-bounds -Wno-maybe-uninitialized" %configure %build %make_build > %ifarch x86_64 %{ix86} Please use "%{x86_64}" for x86_64, as it covers all subarches.
(In reply to Neal Gompa from comment #6) > > %autosetup -C > tar xvf %{SOURCE1} -C src/third_party/lss > > (tar will create the directories for you) > Blech, out of habit I used "xvf" instead of "xf". Please use "xf".
Thanks for the review! All done, except for: > %autosetup -C > tar xvf %{SOURCE1} -C src/third_party/lss > (tar will create the directories for you) Doesn't seem to be the case, I've tried playing around with tar options but adding a mkdir -p seems to be the only way (?). New .spec URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad-2024.02.16-1.fc43.src.rpm
Created attachment 2121112 [details] The .spec file difference from Copr build 9157397 to 9974300
Copr build: https://copr.fedorainfracloud.org/coprs/build/9974300 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2372509-breakpad/fedora-rawhide-x86_64/09974300-breakpad/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
A couple more spec things: > export CXXFLAGS="$CXXFLAGS -Wno-error=array-bounds -Wno-maybe-uninitialized" You might want to switch out "$CXXFLAGS" for "%{build_cxxflags}" as that's the actual macro containing the build flags in question. Also, doesn't the devel subpackage need the static subpackage to be installed to work? Or is it independently functional without it? I figure that since there's no shared library, you probably want to add to the devel subpackage "Requires: %{name}-static = %{version}-%{release}".
(In reply to Ryan from comment #8) > Thanks for the review! > > All done, except for: > > > %autosetup -C > > tar xvf %{SOURCE1} -C src/third_party/lss > > > (tar will create the directories for you) > > Doesn't seem to be the case, I've tried playing around with tar options but > adding a mkdir -p seems to be the only way (?). > How odd, well, it doesn't hurt anything, so it's fine.
> I figure that since there's no shared library, you probably want to add to the devel subpackage "Requires: %{name}-static = %{version}-%{release}". Yeah, true... I think this also explains why I had to add breakpad-static in the original quickshell.spec. Thanks, applied (same URLs) New .spec URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad-2024.02.16-1.fc43.src.rpm
Created attachment 2121241 [details] The .spec file difference from Copr build 9974300 to 9976920
Copr build: https://copr.fedorainfracloud.org/coprs/build/9976920 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2372509-breakpad/fedora-rawhide-x86_64/09976920-breakpad/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Last two things: * The license file needs to be included all subpackages (due to the nature of this package not having true interdeps) * The Requires: in the devel package needs "%{name}-static%{?_isa}" (which was my mistake for not suggesting that originally)
Done, New .spec URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad.spec New .srpm URL: https://errornointernet.fedorapeople.org/review-requests/breakpad/breakpad-2024.02.16-1.fc43.src.rpm
Created attachment 2121242 [details] The .spec file difference from Copr build 9976920 to 9976956
Copr build: https://copr.fedorainfracloud.org/coprs/build/9976956 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2372509-breakpad/fedora-rawhide-x86_64/09976956-breakpad/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
This looks great to me now, so... PACKAGE APPROVED.
Thank you!
The Pagure repository was created at https://src.fedoraproject.org/rpms/breakpad