Spec URL: https://download.copr.fedorainfracloud.org/results/man2dev/wcurl/srpm-builds/08590656/wcurl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/man2dev/wcurl/srpm-builds/08590656/wcurl-2024.12.08-1.src.rpm Description: A simple wrapper around curl to easily download files Fedora Account System Username: man2dev
Copr build: https://copr.fedorainfracloud.org/coprs/build/8590657 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343235-wcurl/fedora-rawhide-x86_64/08590657-wcurl/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
A few quick preliminary findings before I go through the whole fedora-review template. ---- You have (accidentally, I think) put everything into a subpackage called wcurl-wcurl. Try omitting %package %{name} Summary: %{summary} %description %{name} %{_description} and changing %files %{name} to %files so that everything is associated with the base wcurl package. ---- The package doesn’t actually contain /usr/bin/wcurl. Upstream doesn’t have a build system (since this is just a shell-script wrapper around curl), but you still have to install something. Also, man pages need to be installed in a standard location, not as generic documentation; see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages. Try this: %install install -t '%{buildroot}%{_bindir}' -D -p wcurl install -t '%{buildroot}%{_mandir}/man1' -D -m 0644 -p wcurl.1 Remove this from %files: %doc %{name}.1 …and add these: %{_bindir}/wcurl %{_mandir}/man1/wcurl.1* ---- You don’t need these: BuildRequires: bash Requires: bash The shell script actually has #!/bin/sh so it just requires a POSIX shell, not bash in particular. You can rely on that (and, indeed, bash itself) being present in the buildroot, and once you actually install the script properly in /usr/bin, you’ll find that the dependency on /usr/bin/sh is automatically generated based on the shebang line. ---- Arguably, AUTHORS would be better considered as an additional LICENSE file (%license) rather than a generic documentation file (%doc), since it’s referenced from the copyright statement in the LICENSE file text. ---- Please do not include an Epoch tag in a new package! Use these only when absolutely necessary. https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_epoch_tag ---- You don’t have to number the sole Source; you can write "Source:" instead of "Source0:". The exception is if you plan to build this for EPEL8 from the same spec file. ---- The description starts with a lower-case letter, which looks like an accident, and unnecessarily repeats the summary. Consider dropping the %_description macro (since you aren’t going to have any subpackages with repeated descriptions) and just writing this: %description %{summary}. ---- The License needs to be a valid SPDX expression. Write "curl", not "Curl". See https://spdx.org/licenses/curl.html and https://docs.fedoraproject.org/en-US/legal/allowed-licenses/. ---- This is not wrong: %autosetup -p1 -n %{name}-%{version} …but -n %{name}-%{version} is the default, so you can omit it. ---- Upstream includes tests that don’t require network access, and you should be able run them, except that the shunit2 package in Fedora is too old. I filed bug 2343252 and opened https://src.fedoraproject.org/rpms/shunit2/pull-request/1, but for now Still, you can add: # Requires shunit2 2.1.8, but we have 2.1.6. # https://bugzilla.redhat.com/show_bug.cgi?id=2343252 %bcond tests 0 and %if %{with tests} BuildRequires: shunit2 %endif and in %check: %if %{with tests} PATH="${PATH}:%{buildroot}%{_bindir}" ./tests/tests.sh %endif and then (I confirmed by testing) you will be ready to run the tests later, if and when my PR is merged, plus you will have documented why you are not doing so now.
(In reply to Ben Beasley from comment #2) > A few quick preliminary findings before I go through the whole fedora-review > template. > > ---- > > You have (accidentally, I think) put everything into a subpackage called > wcurl-wcurl. > > Try omitting > > %package %{name} > Summary: %{summary} > > %description %{name} %{_description} > > and changing > > %files %{name} > > to > > %files > > so that everything is associated with the base wcurl package. > > ---- > > The package doesn’t actually contain /usr/bin/wcurl. Upstream doesn’t have a > build system (since this is just a shell-script wrapper around curl), but > you still have to install something. > > Also, man pages need to be installed in a standard location, not as generic > documentation; see > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages. > > Try this: > > %install > install -t '%{buildroot}%{_bindir}' -D -p wcurl > install -t '%{buildroot}%{_mandir}/man1' -D -m 0644 -p wcurl.1 > > Remove this from %files: > > %doc %{name}.1 > > …and add these: > > %{_bindir}/wcurl > %{_mandir}/man1/wcurl.1* > > ---- > > You don’t need these: > > BuildRequires: bash > Requires: bash > > The shell script actually has > > #!/bin/sh > > so it just requires a POSIX shell, not bash in particular. You can rely on > that (and, indeed, bash itself) being present in the buildroot, and once you > actually install the script properly in /usr/bin, you’ll find that the > dependency on /usr/bin/sh is automatically generated based on the shebang > line. > > ---- > > Arguably, AUTHORS would be better considered as an additional LICENSE file > (%license) rather than a generic documentation file (%doc), since it’s > referenced from the copyright statement in the LICENSE file text. > > ---- > > Please do not include an Epoch tag in a new package! Use these only when > absolutely necessary. > > https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/ > #_epoch_tag > > ---- > > You don’t have to number the sole Source; you can write "Source:" instead of > "Source0:". The exception is if you plan to build this for EPEL8 from the > same spec file. > > ---- > > The description starts with a lower-case letter, which looks like an > accident, and unnecessarily repeats the summary. Consider dropping the > %_description macro (since you aren’t going to have any subpackages with > repeated descriptions) and just writing this: > > %description > %{summary}. > > ---- > > The License needs to be a valid SPDX expression. Write "curl", not "Curl". > See https://spdx.org/licenses/curl.html and > https://docs.fedoraproject.org/en-US/legal/allowed-licenses/. > > ---- > > This is not wrong: > > %autosetup -p1 -n %{name}-%{version} > > …but -n %{name}-%{version} is the default, so you can omit it. > > ---- > > Upstream includes tests that don’t require network access, and you should be > able run them, except that the shunit2 package in Fedora is too old. > > I filed bug 2343252 and opened > https://src.fedoraproject.org/rpms/shunit2/pull-request/1, but for now > > Still, you can add: > > # Requires shunit2 2.1.8, but we have 2.1.6. > # https://bugzilla.redhat.com/show_bug.cgi?id=2343252 > %bcond tests 0 > > and > > %if %{with tests} > BuildRequires: shunit2 > %endif > > and in %check: > > %if %{with tests} > PATH="${PATH}:%{buildroot}%{_bindir}" ./tests/tests.sh > %endif > > and then (I confirmed by testing) you will be ready to run the tests later, > if and when my PR is merged, plus you will have documented why you are not > doing so now. Thank you so much for your helpful guidance. I apologize for the oversights; all the mentioned fixes have been applied. Spec URL: https://download.copr.fedorainfracloud.org/results/man2dev/wcurl/srpm-builds/08591366/wcurl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/man2dev/wcurl/srpm-builds/08591366/wcurl-2024.12.08-1.src.rpm
Created attachment 2074701 [details] The .spec file difference from Copr build 8590657 to 8591374
Copr build: https://copr.fedorainfracloud.org/coprs/build/8591374 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2343235-wcurl/fedora-rawhide-x86_64/08591374-wcurl/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
> Thank you so much for your helpful guidance. I apologize for the oversights; > all the mentioned fixes have been applied. No need to apologize. Package reviews are a great place for mentoring. I have gotten probably hundreds of packages through review, and I still learn things in package reviews. I still have some suggestions on the latest submission, but I’ve approved the package as-is. Congratulations on your first package in Fedora! The documentation at https://docs.fedoraproject.org/en-US/package-maintainers/Package_Review_Process/#_contributor covers importing and building your package, but if you still have any questions, please feel free to ask them. Full review below: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== Issues ===== - The line %bcond tests 0 is missing. Add that line so that you can do e.g.: fedpkg mockbuild --with tests For more documentation on build conditionals, see: https://rpm-software-management.github.io/rpm/manual/conditionalbuilds.html Since an “undefined” build conditional has the same effect as one that is set false, the package still builds correctly, and this issue doesn’t block approval. - I think it would be better to remove these: BuildRequires: shellcheck BuildRequires: devscripts and these: shellcheck %{buildroot}%{_bindir}/wcurl ./tests/* checkbashisms %{buildroot}%{_bindir}/wcurl ./tests/* While these are tests that you *could* run, they fall under the category of linters, style-checkers, etc., that check code quality rather than functionality. There is no general guideline against running linting tests (and so you are not *required* to change anything here), but the Python guidelines make a pretty good general case against them in https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters. “Linters are “often ‘opinionated’ and their ‘opinions’ change frequently enough that they are nuisance in Fedora, where the linter is not pinned to an exact version.” ===== Notes ===== - This is not wrong: %autosetup -n %{name}-%{version} but it just reiterates the default. This would be simpler and equivalent: %autosetup ===== 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. Note: Checking patched sources after %prep for licenses. Licenses found: "curl License". Detailed output of licensecheck in /home/ben/fedora/review/2343235-wcurl/20250201/2343235-wcurl/licensecheck.txt [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. [-]: 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]: The License field must be a valid SPDX expression. [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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 6936 bytes in 2 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. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. https://koji.fedoraproject.org/koji/taskinfo?taskID=128733728 [-]: %check is present and all tests pass. No tests are run in the current submission, but the package is (nearly) ready to enable them if the shunit2 dependency is updated, which is the best you can reasonably do, and I verified that the tests *would* 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]: 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 all installed packages. Note: No rpmlint messages. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: wcurl-2024.12.08-1.fc42.noarch.rpm wcurl-2024.12.08-1.fc42.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.6.1 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmp5j31nyjv')] checks: 32, packages: 2 wcurl.spec:8: W: mixed-use-of-spaces-and-tabs (spaces: line 1, tab: line 8) 2 packages and 0 specfiles checked; 0 errors, 1 warnings, 7 filtered, 0 badness; has taken 0.1 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.6.1 configuration: /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 3 filtered, 0 badness; has taken 0.0 s Source checksums ---------------- https://github.com/curl/wcurl/archive/v2024.12.08.tar.gz#/wcurl-2024.12.08.tar.gz : CHECKSUM(SHA256) this package : 9c0615b2c5d6b21da79ff559e75452197330d18449085a18e05f4b623b144a94 CHECKSUM(SHA256) upstream package : 9c0615b2c5d6b21da79ff559e75452197330d18449085a18e05f4b623b144a94 Requires -------- wcurl (rpmlib, GLIBC filtered): /usr/bin/sh curl Provides -------- wcurl: wcurl Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -b 2343235 Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api Disabled plugins: fonts, Haskell, PHP, C/C++, Perl, Ocaml, SugarActivity, Java, Python, R Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH
The Pagure repository was created at https://src.fedoraproject.org/rpms/wcurl
> ===== Issues ===== > > - The line > > %bcond tests 0 > > is missing. Add that line so that you can do e.g.: > > fedpkg mockbuild --with tests > > For more documentation on build conditionals, see: > > > https://rpm-software-management.github.io/rpm/manual/conditionalbuilds.html > > Since an “undefined” build conditional has the same effect as one that is > set > false, the package still builds correctly, and this issue doesn’t block > approval. > > - I think it would be better to remove these: > > BuildRequires: shellcheck > BuildRequires: devscripts > > and these: > > shellcheck %{buildroot}%{_bindir}/wcurl ./tests/* > checkbashisms %{buildroot}%{_bindir}/wcurl ./tests/* > > While these are tests that you *could* run, they fall under the category of > linters, style-checkers, etc., that check code quality rather than > functionality. There is no general guideline against running linting tests > (and so you are not *required* to change anything here), but the Python > guidelines make a pretty good general case against them in > https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_linters. > “Linters are “often ‘opinionated’ and their ‘opinions’ change frequently > enough that they are nuisance in Fedora, where the linter is not pinned to > an > exact version.” > > ===== Notes ===== > > - This is not wrong: > > %autosetup -n %{name}-%{version} > > but it just reiterates the default. This would be simpler and equivalent: > > %autosetup > notes have been applied:) # https://src.fedoraproject.org/rpms/wcurl Spec URL: https://download.copr.fedorainfracloud.org/results/man2dev/wcurl/srpm-builds/08598633/wcurl.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/man2dev/wcurl/srpm-builds/08598633/wcurl-2024.12.08-1.src.rpm
FEDORA-2025-ab74ee4eaf (wcurl-2024.12.08-4.fc43) has been submitted as an update to Fedora 43. https://bodhi.fedoraproject.org/updates/FEDORA-2025-ab74ee4eaf
FEDORA-2025-ab74ee4eaf (wcurl-2024.12.08-4.fc43) has been pushed to the Fedora 43 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2025-688bf4d7b6 (wcurl-2024.12.08-4.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2025-688bf4d7b6
FEDORA-2025-688bf4d7b6 (wcurl-2024.12.08-4.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.