> Name: rpkg

Name of the package conflicts with already existing package.
https://admin.fedoraproject.org/pkgdb/package/rpms/rpkg/
May I suggest naming it rpkg-client as the upstream project names?


> BuildRequires: python-rpkg
> Requires: python-rpkg

There is no such package `python-rpkg`. Maybe you meant `python2-rpkg`?


> %if 0%{?fedora} >= 26
> ...
> %else
> BuildRequires: pyrpkg
> Requires: pyrpkg

python2-rpkg-1.49-3 which is even for F25 obsoletes pyrpkg < 1.49-2


> %description
> DistGit command-line client.

Can you please change the description text so it is not just a duplicate of summary line?
I would also prefer more descriptive text. Using description from the first comment in this BZ would be probably sufficient.


> Rpmlint
> -------
> Checking: rpkg-0.2-1.fc27.noarch.rpm
>           rpkg-0.2-1.fc27.src.rpm
> rpkg.noarch: W: non-conffile-in-etc /etc/bash_completion.d/rpkg.bash

Can you please mark the file as %config?
%config(noreplace) %{_sysconfdir}/bash_completion.d/rpkg.bash

> rpkg.noarch: W: non-conffile-in-etc /etc/bash_completion.d/rpkg.bash

This should not be a config file, and this should not be in /etc. It should be installed to `/usr/share/bash-completion/completions/`

> May I suggest naming it rpkg-client as the upstream project names?

Good point, unfortunately rename is needed.  What about the name
dist-git-client?

Hello, here are changes according to review:

Spec URL: http://clime.cz/rpkg-client.spec
SRPM URL: http://clime.cz/rpkg-client-0.1-1.git.4.88886a6.fc25.src.rpm

> What about the name dist-git-client?

Personally I have no problem with both rpkg-client and dist-git-client. May clime use what he likes best (even some other name)


> Source0: rpkg-client-git-4.88886a6.tar.gz

It is not docummented, how can we obtain this archive. In the first version of your spec file there was a

# Source is created by:
# git clone https://pagure.io/rpkg-client.git
# cd rpkg-client
# tito build --tgz

if it is still valid, can you please put it back?


> %package -n rpkg

This will create a subpackage called 'rpkg' (not 'rpkg-client-rpkg') which I believe is still considered as name conflict. Can anyone confirm or deny that, please?

Also with

> %files -n rpkg

and no %files section for the whole package means, that the rpkg-client RPM is not even built.

I am sorry, I don't understand the whole rpkg subpackge idea, can you please clarify it for me?
As I see it, the purpose of this package is to ship a single executable python script `%{_bindir}/rpkg` which imports `pyrpkg` and therefore it requires python2-rpkg. That's basically it. Why to use subpackages at all? Can't we have just a simple rpkg-client package which has the (Build)Requires and %files itself?

I see that you want to stick to generating a RPM named 'rpkg' for some reason, but I don't understand why. Why is it better than RPM named 'rpkg-client'?

(In reply to Jakub Kadlčík from comment #5)
> Personally I have no problem with both rpkg-client and dist-git-client.
> May clime use what he likes best (even some other name)

The dist-git-client naming would mean that we are more coupled with 'dist-git'
project; that naming would be also better if we wanted to e.g. rename
'/bin/rpkg' into e.g. '/bin/dist-git' and do some distro-agnostic development
in this project.

OTOH, if we want to just provide '/bin/rpkg' for compatibility with previous
'rpkg', any 'rpkg-*' name is probably better.

> > %package -n rpkg
> 
> This will create a subpackage called 'rpkg' (not 'rpkg-client-rpkg') which I
> believe is still considered as name conflict. Can anyone confirm or deny
> that, please?

That's not a conflict IMO.  The 'rpkg' binary rpm doesn't exist in (rather
new) Fedora, as that's intentional decision done by release-engineering.

> > %files -n rpkg
> 
> and no %files section for the whole package means, that the rpkg-client RPM
> is not even built.
> 
> I am sorry, I don't understand the whole rpkg subpackge idea, can you please
> clarify it for me?

That's because rel-eng guys don't want to maintain this ~100 lines script
anymore in /usr/bin (they provide it as %doc file only).  Theirs point is
that this script was never meant to be working;  and the fact that we have
found an use-case for it doesn't count :).  Rhbz#1452202.

> As I see it, the purpose of this package is to ship a single executable
> python script `%{_bindir}/rpkg` which imports `pyrpkg` and therefore it
> requires python2-rpkg. That's basically it. Why to use subpackages at all?
> Can't we have just a simple rpkg-client package which has the
> (Build)Requires and %files itself?

There's 'rpkg' package in Fedora (dist-git project, even if it doesn't
generate 'rpkg' RPM anymore).  So to provide /bin/rpkg we need to have
different dist-git name, nevertheless I think we can still build 'rpkg'
package.

> I see that you want to stick to generating a RPM named 'rpkg' for some
> reason, but I don't understand why. Why is it better than RPM named
> 'rpkg-client'?

There are packages that depend on binary package 'rpkg' (which was removed
from fedora).  So the point is to provide 'rpkg' again without changing other
packages (e.g. to avoid requiring 'rpkg-client' on Fedora >= 26, and 'rpkg'
on RHEL and older fedoras).

> There are packages that depend on binary package 'rpkg' (which was removed
from fedora).  So the point is to provide 'rpkg' again without changing other
packages (e.g. to avoid requiring 'rpkg-client' on Fedora >= 26, and 'rpkg'
on RHEL and older fedoras).

Aha, now I see the point of subpackage called 'rpkg'

> It is not docummented, how can we obtain this archive. In the first version
> of your spec file there was a
> 
> # Source is created by:
> # git clone https://pagure.io/rpkg-client.git
> # cd rpkg-client
> # tito build --tgz
> 
> if it is still valid, can you please put it back?

Fixed in upstream (https://pagure.io/rpkg-client/c/c51d55f76f8c3c544e4f1eed2adee97ad93df7cb?branch=master). Sorry, dropped that by accident. Would you prefer to provide new srpm and spec at public share after this fix?

> Would you prefer to provide new srpm and spec at public share after this fix?

Yes please.

But otherwise LGTM. Does anyone else see any issues or can proceed to the next steps of the review process?

> Would you prefer to provide new srpm and spec at public share after this fix?

> Yes please.

Spec URL: http://clime.cz/rpkg-client.spec
SRPM URL: http://clime.cz/rpkg-client-0.2-1.git.2.1b40619.fc25.src.rpm

I am sorry clime, I was about to approve this request, but then I noticed one more thing.


> [?]: Package does not own files or directories owned by other packages.
>      Note: Dirs in package are owned also by: /usr/share/bash-
>      completion(kmod, dnf, rpmdevtools, python-pip, licensecheck, bash-
>      completion, rpmlint, yum, python3-pip, fedpkg, glib2, git-core),
>      /etc/rpkg(rpkg, fedpkg), /usr/share/bash-completion/completions(kmod,
>      dnf, rpmdevtools, python-pip, licensecheck, bash-completion, rpmlint,
>      yum, python3-pip, fedpkg, glib2, git-core)


We can get rid of this issue by not claiming listed directories as files of the package. I think that we can remove following lines from the %files sectiton

%dir %{_sysconfdir}/rpkg
%dir %{_datadir}/bash-completion
%dir %{_datadir}/bash-completion/completions

Done.

Spec URL: http://clime.cz/rpkg-client.spec
SRPM URL: http://clime.cz/rpkg-client-0.5-1.fc25.src.rpm

I needed to move the configuration file out of /etc/rpkg and place it directly in /etc/ (that is /etc/rpkg.conf) because /etc/rpkg is already owned by fedpkg.

Well, ok.
But unfortunately I can't build the package now. Please see the following error.

+ install -d /builddir/build/BUILDROOT/rpkg-client-0.5-1.fc27.x86_64/usr/share/bash-completion/completions
+ cp -a rpkg.conf /builddir/build/BUILDROOT/rpkg-client-0.5-1.fc27.x86_64/etc/
cp: cannot create regular file '/builddir/build/BUILDROOT/rpkg-client-0.5-1.fc27.x86_64/etc/': Not a directory
error: Bad exit status from /var/tmp/rpm-tmp.KCipS9 (%install)

Successful COPR build: https://copr.fedorainfracloud.org/coprs/clime/rpkg-client/build/576706/

You can find the up-to-date srpm and .spec in the linked COPR directory.

*directories (that is fedora-rawhide-x86_64 e.g.)

Spec URL: https://copr-be.cloud.fedoraproject.org/results/clime/rpkg-client/fedora-rawhide-x86_64/00576706-rpkg-client/rpkg-client.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/clime/rpkg-client/fedora-rawhide-x86_64/00576706-rpkg-client/rpkg-client-0.6-1.fc27.src.rpm

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed



===== MUST items =====

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rpkg-client

rpkg-client-0.6-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-52327d89db

rpkg-client-0.6-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-6796b3ccfe

rpkg-client-0.6-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-52327d89db

rpkg-client-0.6-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-e812e8ac0b

rpkg-client-0.6-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-f281a9ac6d

%install
rm -rf $RPM_BUILD_ROOT

is no longer needed (for a long time).

I would suggest using the newer %py2_build/%py2_install macros

You're missing a BR on python-setuptools

Is this going to be submitted to EPEL7?

rpkg-client-0.6-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

(In reply to Orion Poplawski from comment #24)
> %install
> rm -rf $RPM_BUILD_ROOT
> 
> is no longer needed (for a long time).
> 
> I would suggest using the newer %py2_build/%py2_install macros
> 
> You're missing a BR on python-setuptools
> 
> Is this going to be submitted to EPEL7?

This should be fixed now in 0.8 https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-570f1396db.

rpkg-client-0.8-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2017-570f1396db

rpkg-client-0.8-1.fc26 has been pushed to the Fedora 26 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-fb518765d0

rpkg-client-0.8-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2017-035046c4e8

rpkg-client-0.8-1.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

rpkg-client-0.8-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.

rpkg-client-0.8-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.