https://bugzilla.redhat.com/show_bug.cgi?id=2178600
--- Comment #2 from Neal Gompa <[email protected]> --- (In reply to Dominik 'Rathann' Mierzejewski from comment #1) > Drive-by comments: > > MUST: License tag needs to be updated to SPDX. > Fixed. > Optional stuff: > > The spec layout is a bit unusual in that subpackage sections include %files > lists. I'd rearrange to follow the usual layout (all %files at the end > before %changelog). > > It's sufficient to conditionalize only the %files section of the hevc > subpackage. RPM will not try to produce binary RPM with a missing %files > section. > > I'd also drop the separator comments (# > ----------------------------------------------------------------------) as I > don't see any value in them. > I prefer this style when I have conditionalized subpackages as it makes it much clearer what's going on. > I'd change -f to -v in the rm command below: > find %{buildroot} -name '*.la' -or -name '*.a' | xargs rm -f > for better visibility of what (if anything) is getting deleted here and > failure if there's nothing to delete (because the command is redundant and > should be removed then). > Good suggestion, fixed. > Other than that, LGTM. I've updated the package and spec in place for this. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2178600 _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/[email protected] Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
