Bug 2372509

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