[Bug 1678233] Review Request: rpm2swidtag - Tools for producing SWID tags from rpm package headers and inspecting the SWID tags

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite Conditions]     [KDE Users]

  Powered by Linux