https://bugzilla.redhat.com/show_bug.cgi?id=1678233 Jan Pazdziora <jpazdziora@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jpazdziora@xxxxxxxxxx --- Comment #3 from Jan Pazdziora <jpazdziora@xxxxxxxxxx> --- (In reply to Miroslav Suchý from comment #2) > > %define unmangled_version 0.6.5 > > %define unmangled_version 0.6.5 > > Any reason to define it twice? > > > %define name rpm2swidtag > > %define version 0.6.5 > > %define release 1%{dist} > > Any reason to define them when it is not used? BTW every tag will define a > corresponding macro. So Name will define %name. So this has no use because > it is overridden by a tag below. No, these should go -- it's a leftover from the original setup.py-generated spec that I missed. > > Summary: Tools for producing SWID tags from rpm package headers and inspecting the SWID tags > > The summary should be shorter than 80 characters. Can you try to make it a > bit shorter? Sure, will make it into Tools for producing SWID tags for rpm packages and inspecting the SWID tags > > Release: 1%{dist} > > This should be `1%{?dist}` so it produces valid output in case of dist macro > is not defined. Fixed. > > Group: Development/Libraries > > This tag has no use and it should be removed. Removed. > > BuildRequires: fakeroot > > It would be nice if you can separate aside - with a comment - the > buildrequires which are needed only for %check section. OK. The only "real" BuildRequires are python3-setuptools and python3-rpm-macros, it seems. > > %package -n dnf-plugin-swidtags > > I would advise putting "Recommend: dnf" in this sub-package. OK. > > %clean > > rm -rf $RPM_BUILD_ROOT > > This section is not used nowadays and should be removed entirely. OK. > > > --root=$RPM_BUILD_ROOT > > Use macros consistently. Either old style or the new style. I recommend to > use `--root=%{buildroot}` OK. > > # %license LICENSE > > Any reason why LICENSE is not included in tar ball? Especially when you are > upstream as well? Because with the 0.6.5 upstream release, the LICENSE is not packages in the .tar.gz. Fix for that is in https://github.com/swidtags/rpm2swidtag master but I'm waiting for one more thing from the rpm team to make a new release. By that time I plan to uncomment this line. > You are missing > %dir %{_sysconfdir}/%{name} > in %files. Fixed. > Who owns `%{_sysconfdir}/swid/` and `/usr/share/swidq` ? Hint - it should be > either you or some your dependency. # rpm -qf /etc/swid fedora-release-common-30-0.21.noarch Not sure if it's better to add the dependency on just add my ownership on that file (or move the swidq.conf to something like /etc/swidq/ altogether). > > %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf.d > > You probably meant: > > %dir %{_sysconfdir}/%{name}/%{name}.conf.d > %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf.d/* True. > > %{_bindir}/rpm2swidtag > > %{_bindir}/swidq > > It would be really nice if you can write a man page for these two. Would you envision anything more that the relevant parts from https://github.com/swidtags/rpm2swidtag/blob/master/README.md? What is the recommended way of producing man pages from Markdown, for rpms? > You are missing a %changelog section. Will be added. > All python modules should BuildRequire python3-devel OK. That would then be the only non-test BuildRequire. > > %{__python3} setup.py build > > You should use %py3_build macro. Fixed. > > %{__python3} setup.py install --single-version-externally-managed --single-version-externally-managed -O1 --root=$RPM_BUILD_ROOT --record=INSTALLED_FILES > > You should use %py3_install plus some custom option if needed. Fixed. I've now updated https://adelton.fedorapeople.org/rpm2swidtag.spec based on the review, except for the man pages. -- 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 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx
