https://bugzilla.redhat.com/show_bug.cgi?id=2160996
--- Comment #8 from Luya Tshimbalanga <luya_tfz(a)thefinalzone.net> ---
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/fedor...
SRPM:
https://download.copr.fedorainfracloud.org/results/luya/blender-egl/fedor...
--
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