https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Bug ID: 1460917 Summary: Review Request: rpkg - Command-line client tool to DistGit Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: clime@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: http://clime.cz/rpkg.spec SRPM URL: http://clime.cz/rpkg-0.2-1.fc25.src.rpm Description: This is a command-line client to DistGit host machine (see `man rpkg` after installing). You can maintain your packages stored in DistGit with this tool, for example run `rpkg copr-build username/project` to execute COPR build of your package. Fedora Account System Username: clime
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Jakub Kadlčík jkadlcik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jkadlcik@redhat.com
--- Comment #1 from Jakub Kadlčík jkadlcik@redhat.com ---
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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Jakub Kadlčík jkadlcik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |jkadlcik@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #2 from Neal Gompa ngompa13@gmail.com ---
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/`
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #3 from Pavel Raiskup praiskup@redhat.com ---
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?
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #4 from clime@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #5 from Jakub Kadlčík jkadlcik@redhat.com ---
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'?
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Pavel Raiskup praiskup@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugzilla.redhat.com | |/show_bug.cgi?id=1452202
--- Comment #6 from Pavel Raiskup praiskup@redhat.com --- (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).
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #7 from Jakub Kadlčík jkadlcik@redhat.com ---
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'
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #8 from clime@redhat.com ---
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?bra...). Sorry, dropped that by accident. Would you prefer to provide new srpm and spec at public share after this fix?
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #9 from Jakub Kadlčík jkadlcik@redhat.com ---
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?
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #10 from clime@redhat.com ---
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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #11 from Jakub Kadlčík jkadlcik@redhat.com --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #12 from clime@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #13 from Jakub Kadlčík jkadlcik@redhat.com --- 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)
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #14 from clime@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #15 from clime@redhat.com --- *directories (that is fedora-rawhide-x86_64 e.g.)
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #16 from Jakub Kadlčík jkadlcik@redhat.com --- Spec URL: https://copr-be.cloud.fedoraproject.org/results/clime/rpkg-client/fedora-raw... SRPM URL: https://copr-be.cloud.fedoraproject.org/results/clime/rpkg-client/fedora-raw...
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #17 from Jakub Kadlčík jkadlcik@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Jakub Kadlčík jkadlcik@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |fedora-review+
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #18 from Gwyn Ciesla limburgher@gmail.com --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/rpkg-client
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |MODIFIED
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #19 from Fedora Update System updates@fedoraproject.org --- rpkg-client-0.6-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2017-52327d89db
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #20 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #21 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #22 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #23 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Orion Poplawski orion@cora.nwra.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |orion@cora.nwra.com
--- Comment #24 from Orion Poplawski orion@cora.nwra.com --- %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?
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed| |2017-07-26 18:52:17
--- Comment #25 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #26 from clime@redhat.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|CLOSED |ON_QA Resolution|ERRATA |--- Keywords| |Reopened
--- Comment #27 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #28 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #29 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |ERRATA Last Closed|2017-07-26 18:52:17 |2017-08-06 18:25:28
--- Comment #30 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #31 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1460917
--- Comment #32 from Fedora Update System updates@fedoraproject.org --- 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.
package-review@lists.fedoraproject.org