https://bugzilla.redhat.com/show_bug.cgi?id=2160996 --- Comment #8 from Luya Tshimbalanga <luya_tfz@xxxxxxxxxxxxxxxx> --- Thank you for taking the review > - missing license file > Fixed by including LICENSE file > - missing documentation > There seems to be a lot, so a subpackage could be beneficial. As pointed out by Ben, it turned out unnecessary because of precompiled javascripts. > > - Package does not own files or directories owned by other packages. > /usr/share/cmake is owned by this package. Change: > %{_datadir}/cmake/ > to > %{_datadir}/cmake/%{name}/ Fixed. > > - missing %check > Please check if it is feasible to compile and run the tests > %ctest applied on %check line. > - unnecessary %global _hardened_build 1 > > > - %global _disable_ld_no_undefined %nil > Does this do anything? > > - remove commented `debug_package` and `_legacy_common_support` macros if > they're not necessary. > All removed. > ===== Issues ===== > > - The docs/assets/ directory contains precompiled CSS and JavaScript, which > could not be packaged as-is: > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Web_Assets/#_css > > https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/ > #_compilationminification > > This is OK since you are not packaging the HTML documentation, but it would > be best if you remove the directory in %prep. > > (I think this is more than adequate reason NOT to package the > documentation.) Done. > > - Similarly, you should remove the following in %prep: > > - javascript/ contains precompiled JavaScript > - maya/ contains precompiled Windows and MacOS binaries (inside .tar.gz > archives) > Done by removing both javascript and maya directory. > - Add to %files: > > %license LICENSE AUTHORS > Fixed > - Please remove the following, since it is not used and is therefore > confusing: > > # draco git > %global commit0 4cba1acdd718b700bb33945c0258283689d4eac7 > %global shortcommit0 %(c=%{commit0}; echo ${c:0:7}) > %global gver git%{shortcommit0} > > - It would be good to remove these commented-out lines too: > > #%%global debug_package %%{nil} > #define _legacy_common_support 1 > > - This is the default since Fedora 23, so the explicit macro can be removed: > > %global _hardened_build 1 > > %global _disable_ld_no_undefined %nil Above lines removed. > > - The -devel package should have a fully versioned dependency on the base > package. Instead of > > Requires: draco >= %{version}-%{release} > > use > > Requires: draco%{?_isa} = %{version}-%{release} > Fixed. > - The use of the %{name} macro is inconsistent; this is not against the > guidelines, but consider standardizing on either using %{name} or writing > out > “draco”. Personally, I’ve come to favor less use of trivial macros like > this, > but this is mostly a subjective question. > An oversight from by part. I chose keeping the %{name} macro for consistency. > - Change > > %{_datadir}/cmake/ > > to > > %{_datadir}/cmake/%{name}/ > Fixed > - Please check if this is doing anything useful; I suspect it is not, since > Fedora is already setting LTO flags in CFLAGS/CXXFLAGS/LDFLAGS. > > -DCMAKE_INTERPROCEDURAL_OPTIMIZATION=ON It turned out unneeded so that line is removed. > > - Please remove this, since it is already in the expansion of the %cmake > macro (and uses %{_prefix} instead of hard-coded /usr): > > -DCMAKE_INSTALL_PREFIX=/usr \ > > - You have this twice; please remove one of them. > > -DCMAKE_INSTALL_PREFIX=/usr \ > Fixed by removed them. > - Consider including README.md as %doc. Done > > - Man pages are always desired (but not required): > > draco.x86_64: W: no-manual-page-for-binary draco_decoder > draco.x86_64: W: no-manual-page-for-binary draco_decoder-1.5.5 > draco.x86_64: W: no-manual-page-for-binary draco_encoder > draco.x86_64: W: no-manual-page-for-binary draco_encoder-1.5.5 > > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages The linked guideline failed to clarify about the proper use of "help2man" command in the spec file. I commented out these line for the time until a proper solution occurs. (In reply to Ben Beasley from comment #7) > Are you sure this package needs to be ExclusiveArch: x86_64? > > Upstream documentation > (https://github.com/google/draco/blob/master/BUILDING.md) suggests that at > least i686 and aarch64 should be supported, and after removing the > ExclusiveArch line I was able to do a successful scratch build for all > current Fedora primary architectures. The build succeeded on all supported architecture as highlighted on https://copr.fedorainfracloud.org/coprs/luya/blender-egl/build/5235725/ so ExclusiveArch line is removed. Based on the feedback, here is the update: SPEC: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-rawhide-x86_64/05235725-draco/draco.spec SRPM: https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedora-rawhide-x86_64/05235725-draco/draco-devel-1.5.5-1.fc38.x86_64.rpm -- 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=2160996 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx 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/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
