Bug 1678233

I will do the review.

> %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.

> 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?

> Release: 1%{dist}

This should be `1%{?dist}` so it produces valid output in case of dist macro is not defined.

> Group: Development/Libraries

This tag has no use and it should be removed.

> BuildRequires: fakeroot

It would be nice if you can separate aside - with a comment - the buildrequires which are needed only for %check section.

> %package -n dnf-plugin-swidtags

I would advise putting "Recommend: dnf" in this sub-package.

> %clean
> rm -rf $RPM_BUILD_ROOT

This section is not used nowadays and should be removed entirely.

> --root=$RPM_BUILD_ROOT

Use macros consistently. Either old style or the new style. I recommend to use `--root=%{buildroot}`

> # %license LICENSE

Any reason why LICENSE is not included in tar ball? Especially when you are upstream as well?


You are missing 
 %dir %{_sysconfdir}/%{name}
in %files.


Who owns `%{_sysconfdir}/swid/` and `/usr/share/swidq` ? Hint - it should be either you or some your dependency.

> %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf.d

You probably meant:

%dir %{_sysconfdir}/%{name}/%{name}.conf.d
%config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf.d/*

> %{_bindir}/rpm2swidtag
> %{_bindir}/swidq

It would be really nice if you can write a man page for these two.

You are missing a %changelog section.

All python modules should BuildRequire python3-devel

> %{__python3} setup.py build

You should use %py3_build macro.

> %{__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.

(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.

Whoa, you are fast! :

>Would you envision anything more that the relevant parts from https://github.com/swidtags/rpm2swidtag/blob/master/README.md? 

Yes.

> What is the recommended way of producing man pages from Markdown, for rpms?

Personally, I use:

BuildRequires: asciidoc
BuildRequires: libxslt

%build
a2x -d manpage -f manpage foo.8.asciidoc

%install
install -m644 foo.8 %{buildroot}/%{_mandir}/man8/

You can see an example here:
https://github.com/xsuchy/fedora-upgrade
The syntax of AsciiDoc:
https://powerman.name/doc/asciidoc


> 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).

Personally, I would add the dependency. But all other solutions you mentioned are fine too.

The dependency is there.

The man pages will likely take me some time as I'm not quite happy with any of the approaches I tried -- for example, the asciidoc seems to have rather huge set of dependencies. Do you view them as a blocker?

> Do you view them as a blocker?

No. But I will periodically remind it to you. :)

APPROVED

(In reply to Miroslav Suchý from comment #6)
> 
> No. But I will periodically remind it to you. :)

That'd be very kind of you. :-P

Thank you!

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/rpm2swidtag

I've made upstream release 0.7.1 and updated to it, we now ship the LICENSE file.

Built rpm2swidtag-0.7.1-1.fc30 via https://koji.fedoraproject.org/koji/taskinfo?taskID=32890827.