Spec URL: https://github.com/minfrin/retry/blob/master/retry.spec.in SRPM URL: https://copr.fedorainfracloud.org/coprs/minfrin/retry/ Description: The retry command line tool repeats a given command until the command is successful, backing off with a configurable delay between each attempt. Fedora Account System Username: minfrin
Hello Graham, thank you for adding this package. > Spec URL: https://github.com/minfrin/retry/blob/master/retry.spec.in > SRPM URL: https://copr.fedorainfracloud.org/coprs/minfrin/retry/ Please specify the actual download URLs. It will help fedora-review tool to automatically download them, and make the review easier. In the case of the SRPM file, you can go to the project you linked, open the detail of your last build and scroll down to the "Results" section. Once you open fedora-rawhide-x86_64, there is a directory listing where you can find the actual download URL. There is also a spec file next to it, if you want to link spec file from Copr as well. In general, there is no problem with linking a spec file from GitHub but please use the "Raw" link. Also, you are linking retry.spec.in which still needs to be processed somehow to generate the actual retry.spec. If you don't want to have a generated spec in the repo, my recommendation is to simply use the spec file from Copr. > Group: Applications/System Please remove the Group line, it is deprecated. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections > Source: Sources should be indexed from zero, so please change Source: to Source0: Also, according to https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags I would recommend the following URL https://github.com/minfrin/retry/archive/%{name}-%{version}/%{name}-%{version}.tar.gz or even %{URL}/archive/%{name}-%{version}/%{name}-%{version}.tar.gz if you swap URL: and Source0: lines, so the URL is defined first. > BuildRequires: gcc, autoconf, automake, libtool While this is correct, I would recommend BuildRequires: gcc BuildRequires: autoconf BuildRequires: automake BuildRequires: libtool Once you start adding and removing dependencies, it will be much easier to read the diffs. But it's up to your preference. > %description Retry has such a good help page, it would be a pity to not use it. What about using the whole DESCRIPTION text from `retry --help`?
> %define __libtoolize /bin/true Is the line necessary? I tried to build the package without it, and it worked.
I made some suggestions, all of those are minor things that can be easily fixed. Overall, I like the spec file and the package as well. Good job, Graham.
Spec URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/fedora-rawhide-x86_64/03005256-retry/retry.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/fedora-rawhide-x86_64/03005256-retry/retry-1.0.4-1.fc36.src.rpm Description: The retry command line tool repeats a given command until the command is successful, backing off with a configurable delay between each attempt. Fedora Account System Username: minfrin
Thanks for the review, I've made the changes requested and sent updated URLs. Would it be possible to take a look?
The spec file looks really good now. However, we still need to fix a couple of things that fedora-review tool discovered. The checksum for the .tar.gz that you baked into your .src.rpm has a different checksum than this file https://github.com/minfrin/retry/archive/retry-1.0.4/retry-1.0.4.tar.gz CHECKSUM(SHA256) this package : 98abe614ef43fb31c99c7e941cddf3df0527361dc81be395066831eb6e1e7515 CHECKSUM(SHA256) upstream package : e7ab48923fd91aa2c5397cb2db4907a486176a582efe892a2113c9b91655f1e8 In fact, they are totally different tarballs. If you extract the SRPM file using this command `rpm2cpio ./retry-1.0.4-1.fc36.src.rpm | cpio -i` and explore the tarball, you can see that there are different files than in the upstream tarball. Rpmlint also discovered this retry.x86_64: W: incoherent-version-in-changelog 1.0.0-1 ['1.0.4-1.fc36', '1.0.4-1'] Basically, your 1.0.4 version is not mentioned in the changelog. Also, when you make updates to a specfile, don't forget to bump the release number and add a new changelog entry :-)
Oops, forgot to tell you. The easiest way to reproduce those errors by yourself is by going to your project settings in Copr and enabling the "Run fedora-review tool for packages in this project" option. Then you will see a "review.txt" link in your build results. For more information, please see https://frostyx.cz/posts/running-fedora-review-after-copr-build https://docs.pagure.org/copr.copr/user_documentation.html#automatic-run-of-fedora-review-tool
Got to the bottom of the weirdness. The releases are created by autotools using make dist, while the github archive URL is just a tarball of the repo. The archive's name doesn't match the tag name, so the archive URL doesn't work for standard builds. Will upload new URLs with the source URL corrected.
Spec URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/fedora-rawhide-s390x/03017943-retry/retry.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/fedora-rawhide-s390x/03017943-retry/retry-1.0.4-1.fc36.src.rpm Description: The retry command line tool repeats a given command until the command is successful, backing off with a configurable delay between each attempt. Fedora Account System Username: minfrin
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages [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. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "*No copyright* Apache License 2.0", "FSF All Permissive License", "[generated file]", "FSF Unlimited License (with Retention) GNU General Public License, Version 2 [generated file]", "FSF Unlimited License [generated file]", "Apache License 2.0", "GNU General Public License v2.0 or later [generated file]", "GNU General Public License v3.0 or later", "MIT License [generated file]", "GNU General Public License v2.0 or later". 8 files have unknown license. Detailed output of licensecheck in /var/lib/copr-rpmbuild/results/retry/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [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. [-]: 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. [x]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 3 files. [x]: Package complies to the Packaging Guidelines [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]: 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]: 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 must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [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]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [-]: 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. [?]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [?]: Package should compile and build into binary rpms on all supported architectures. [!]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [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]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Package should not use obsolete m4 macros Rpmlint ------- Checking: retry-1.0.4-1.fc36.x86_64.rpm retry-debuginfo-1.0.4-1.fc36.x86_64.rpm retry-debugsource-1.0.4-1.fc36.x86_64.rpm retry-1.0.4-1.fc36.src.rpm retry.x86_64: W: spelling-error %description -l en_US stdin -> stein, stain, stdio retry.x86_64: W: spelling-error %description -l en_US stdout -> stout, std out, std-out retry.x86_64: W: spelling-error %description -l en_US stderr -> std err, std-err, stander retry.x86_64: E: zero-length /usr/share/doc/retry/README retry.src: W: spelling-error %description -l en_US stdin -> stein, stain, stdio retry.src: W: spelling-error %description -l en_US stdout -> stout, std out, std-out retry.src: W: spelling-error %description -l en_US stderr -> std err, std-err, stander 4 packages and 0 specfiles checked; 1 errors, 6 warnings. Rpmlint (debuginfo) ------------------- Checking: retry-debuginfo-1.0.4-1.fc36.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- Cannot parse rpmlint output: Source checksums ---------------- https://github.com/minfrin/retry/releases/download/retry-1.0.4/retry-1.0.4.tar.gz : CHECKSUM(SHA256) this package : 9d5dff85ed7faddf0d39aa4cd8cc5e6920cefef44d3babf407360e94d58bcc09 CHECKSUM(SHA256) upstream package : 9d5dff85ed7faddf0d39aa4cd8cc5e6920cefef44d3babf407360e94d58bcc09 Requires -------- retry (rpmlib, GLIBC filtered): libc.so.6()(64bit) rtld(GNU_HASH) retry-debuginfo (rpmlib, GLIBC filtered): retry-debugsource (rpmlib, GLIBC filtered): Provides -------- retry: retry retry(x86-64) retry-debuginfo: debuginfo(build-id) retry-debuginfo retry-debuginfo(x86-64) retry-debugsource: retry-debugsource retry-debugsource(x86-64)
> Got to the bottom of the weirdness. I am glad you figured it out. The package looks good, it is +1 from me, but since this seems to be your first Fedora package, we need a Fedora packager sponsor to give this an actual +1. I am dropping myself from the assignee so they notice it requires their attention. You can check out this page https://docs.pagure.org/fedora-sponsors/ and try to ask someone to take a look. It should be easy since we already did the work :-) (Don't worry, this step won't be necessary for any of your next packages)
Thanks for your detailed help.
Jakub, you (or any Fedora packager) can still approve the package. Graham just won’t be able to import it until he has found a sponsor. It’s normal to have one or more packages approved before finding a sponsor—see https://docs.fedoraproject.org/en-US/package-maintainers/How_to_Get_Sponsored_into_the_Packager_Group/#submitting_quality_new_packages.
Ah, thank you Ben, I didn't know that. In that case, I am giving +1
Hi Graham, I have sponsored you. I lurk on the httpd dev list and am comfortable with your abilities. Please feel free to contact me directly if you have any questions about Fedora processes and I'll do my best to help out. Just a few minor things on this package: 1. In the %prep section, after running "%setup -q" the current directory is automatically set to %{_builddir}/%{name}-%{version} so you could simplify "rm -rf %{_builddir}/%{name}-%{version}/debian" to just "rm -rf debian". In fact if the files in there do not affect the result of the build, you don't even need to do that - the only files that will will end up in the final package are those in the %files list. 2. It's normal to use gcc rather than clang unless upstream does not support gcc or there's some other good reason not to use gcc (see https://docs.fedoraproject.org/en-US/packaging-guidelines/#compiler). 3. I'd have a blank line before %build in the spec file to clearly separate the %prep, %build, %install, %check (not present here but may be in other packages) and %files sections, which I think helps readability. Jakub, you should probably assign this ticket back to yourself since you did the actual review. Cheers, Paul.
I have a new version of retry that's been released, a feature release. I have polished out the spec file issues above, hopefully I haven't missed any. https://github.com/minfrin/retry/releases/tag/retry-1.0.5
Package was never imported. The ticket status is being reset, since creating the repository will require a fresh approval. Let us know if you're still interested in this package.
Quick ping on this one, is it possible to look at this again?
I am really sorry Graham, this somehow slipped through the cracks. I am giving +1 because we already did the review and the package was good. I can see that Paul already sponsored you and added you to the packager group, so you should be able to import and build the package now.
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.
Spec URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/rhel-10-ppc64le/09163194-retry/retry.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/rhel-10-ppc64le/09163194-retry/retry-1.0.6-1.el10.src.rpm Description: The retry command line tool repeats a given command until the command is successful, backing off with a configurable delay between each attempt. Fedora Account System Username: minfrin
Spec URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/fedora-rawhide-s390x/09163210-retry/retry.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/minfrin/retry/fedora-rawhide-s390x/09163210-retry/retry-1.0.6-1.fc43.src.rpm Description: The retry command line tool repeats a given command until the command is successful, backing off with a configurable delay between each attempt. Fedora Account System Username: minfrin
New release of retry v1.0.6, this ticket has been updated. Something I've found confusing - I am a member of packagers group, but cannot create any kind of repo at https://src.fedoraproject.org/user/minfrin/projects for a package. Is this expected?
(In reply to Graham Leggett from comment #24) > New release of retry v1.0.6, this ticket has been updated. > > Something I've found confusing - I am a member of packagers group, but > cannot create any kind of repo at > https://src.fedoraproject.org/user/minfrin/projects for a package. Is this > expected? Package repos are created via tickets at https://pagure.io/releng/fedora-scm-requests/issues, which you should create via fedpkg request-repo. This requires either an approved package review in Bugzilla, or the --exception option and someone to manually confirm that one of the exceptions in https://docs.fedoraproject.org/en-US/fesco/Package_review_policy/#what applies. You can fork existing Fedora packages into your own namespace, like https://src.fedoraproject.org/fork/minfrin/rpms/<whatever>, but you aren’t expected to be able to create arbitrary repos that aren’t connected to existing Fedora packages.
Thanks for confirming, once the v1.0.6 is reviewed I will try the import.