SRPM:https://fedorapeople.org/~limb/review/rust-temp-dir/rust-temp-dir-0.1.16-1.fc44.src.rpm SPEC:https://fedorapeople.org/~limb/review/rust-temp-dir/rust-temp-dir.spec Description: Simple temporary directory with cleanup.
There seems to be some problem with the following file. SRPM URL: https://fedorapeople.org/~limb/review/rust-temp-dir/rust-temp-dir-0.1.16-1.fc44.src.rpm Fetching it results in a 404 Not Found error. Please make sure the URL is correct and publicly available. --- 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.
[fedora-review-service-build]
Copr build: https://copr.fedorainfracloud.org/coprs/build/10173201 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2434109-rust-temp-dir/fedora-rawhide-x86_64/10173201-rust-temp-dir/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.
The package is good to go, according to my review, except that meanwhile a newer version of the crate has been published. I propose that the newer version be re-reviewed. Here's my filled-out review template for the proposed rust-temp-dir-0.1.16-1.fc44.src.rpm. 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", "Apache License 2.0", "*No copyright* Apache License 2.0". 5 files have unknown license. Detailed output of licensecheck in /var/lib/copr-rpmbuild/results/rust-temp- dir/licensecheck.txt [-]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries or specifies bundled libraries with Provides: bundled(<libname>) if unbundling is not possible. [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 installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. [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 0 bytes in 0 files. [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). [-]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rust- temp-dir-devel , rust-temp-dir+default-devel [?]: Package functions as described. [!]: 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. [-]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define unless justified. Note: %define requiring justification: %define autorelease(e:s:pb:n) %{?-p:0.}%{lua: [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. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages.
In rust-flexi_logger, bug 2434118, you can easily patch the temp-dir dev-dependency from 0.1 to 0.2, https://github.com/emabee/flexi_logger/pull/204. That would allow you to package the latest release here.
Thanks! SRPM:https://fedorapeople.org/~limb/review/rust-temp-dir/rust-temp-dir-0.2.0-1.fc45.src.rpm SPEC:https://fedorapeople.org/~limb/review/rust-temp-dir/rust-temp-dir.spec
Created attachment 2131988 [details] The .spec file difference from Copr build 10173201 to 10189710
Copr build: https://copr.fedorainfracloud.org/coprs/build/10189710 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2434109-rust-temp-dir/fedora-rawhide-x86_64/10189710-rust-temp-dir/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.
Troels, are you available to re-review Gwyn’s updated submission? Thanks!
It seems to me that the following package needs to exist as a package first, before we can continue here?: rust-cond_sync - See https://bugzilla.redhat.com/show_bug.cgi?id=2434112 (Or maybe I'm getting the procedure wrong.) For 2434112 I hope to be able to review later this week. If that's too slow, others should feel free to take over.
No, that’s just fine. I don’t think there’s a great hurry here. (Gwyn, pleas correct me if I’m wrong.) I think this package, rust-temp-dir, may be reviewed any time. For rust-cond_sync, bug 2434112, there isn’t yet a submission with the necessary license texts, so it’s still blocked on that – either on upstream action, or Gwyn deciding to patch in probably-correct license text per https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text – and then rust-flexi_logger, bug 2434118, is blocked on both rust-temp-dir and rust-cond_sync. I think I was just making sure you were still tracking this! Thanks for checking in.
This package is currently awaiting the existence of rust-cond_sync.
Sorry, it seems I've misunderstood something: rust-temp-dir does not depend on rust-cond_sync. So version 0.2.0 of rust-temp-dir should be reviewed. Meanwhile, the review document at https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2434109-rust-temp-dir/fedora-rawhide-x86_64/10189710-rust-temp-dir/fedora-review/review.txt has gone. I'll try triggering generation of a new review template for this.
Copr build: https://copr.fedorainfracloud.org/coprs/build/10256267 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2434109-rust-temp-dir/fedora-rawhide-x86_64/10256267-rust-temp-dir/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.
Package Review ============== The review has no bad findings. Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== 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", "Apache License 2.0", "*No copyright* Apache License 2.0". 5 files have unknown license. Detailed output of licensecheck in /var/lib/copr-rpmbuild/results/rust-temp- dir/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 or specifies bundled libraries with Provides: bundled(<libname>) if unbundling is not possible. [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. [x]: 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 installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: No rpmlint messages. [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 0 bytes in 0 files. [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]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rust- temp-dir-devel , rust-temp-dir+default-devel [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. [-]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [-]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define unless justified. Note: %define requiring justification: %define autorelease(e:s:pb:n) %{?-p:0.}%{lua: -> This seems normal with Rust packages. [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.
Thank you! So is this approved then?
Next step is that someone with packager privileges (which I don't have) changes the "fedora-review" flag to "+".
I generally agree with the review by Troels, above. (In reply to Troels Arvin from comment #16) > [x]: Spec use %global instead of %define unless justified. > Note: %define requiring justification: %define autorelease(e:s:pb:n) > %{?-p:0.}%{lua: > -> This seems normal with Rust packages. It’s normal, and it’s due to expansion of rpmautospec macros. This doesn’t happen in source RPMs built with rpmbuild -bs, but it does happen in those built with fedpkg srpm. The advice to prefer %global is dubious and/or outdated, anyway; see https://pagure.io/packaging-committee/issue/1449. I confirmed that the package builds on all architectures: https://koji.fedoraproject.org/koji/taskinfo?taskID=144462512 I confirmed that the license metadata appears correct. However, I did find one issue: the file LICENSE looks like this: Copyright 2021 Leonhard LLC Licensed under the Apache License, Version 2.0 (the "License"); you may not use these files except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. Unfortunately, section 4(a) of the Apache-2.0 license says, “You must give any other recipients of the Work or Derivative Works a copy of this License.” Therefore, the complete license text needs to be included in the package. You should point this out upstream at https://gitlab.com/leonhard-llc/ops and ask them to add the license text, or open a PR to do so. Until upstream makes a new release with license text included, you’ll have to patch in the standard Apache-2.0 text yourself; see https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text.