Spec URL: https://raw.githubusercontent.com/damien-lemoal/fedora-packages/master/dm-zoned-tools/dm-zoned-tools.spec SRPM URL: https://raw.githubusercontent.com/damien-lemoal/fedora-packages/master/dm-zoned-tools/dm-zoned-tools-2.1.1-1.fc34.src.rpm Description: The dm-zoned-tools package provides the dmzadm utility which can be used to format, check and repair zoned block devices used with Linux dm-zoned device mapper target driver. Fedora Account System Username: damien-lemoal Fedora enables support for zoned block devices, aka SMR hard-disks (CONFIG_BLK_DEV_ZONED) since Fedora 26. The dm-zoned device mapper target is provided as a pre-compiled kernel module since Fedora 27. The dm-zoned-tools package completes the support for dm-zoned targets by providing to users the user land tool necessary for formatting, checking and repairing dm-zoned target devices. This package successfully builds with koji: https://koji.fedoraproject.org/koji/taskinfo?taskID=68758655 A temporary repository was setup on Fedora Copr: https://copr.fedorainfracloud.org/coprs/damien-lemoal/zonedstorage/package/dm-zoned-tools/ I am the original author of the kernel dm-zoned device mapper target. See kernel commit 3b1a94c88b79 ("dm zoned: drive-managed zoned block device target"). I am also the maintainer of the dm-zoned-tools project, hosted on GitHub at https://github.com/westerndigitalcorporation/dm-zoned-tools. This is my first package submission to Fedora and I need a sponsor.
We'd normally prefer you did not add a license to the spec file itself. For details see https://fedoraproject.org/wiki/Licensing:Main#License_of_Fedora_SPEC_Files You need to drop the BuildRoot line entirely. It'd be nice if the BuildRequires line at least had spaces between the names, but ideally you'd list the BuildRequires one per line, eg: BuildRequires: device-mapper-devel BuildRequires: kmod-devel etc In particular this makes it easier to comment out or conditionalize particular BRs if necessary later. > This is my first package submission to Fedora and I need a sponsor. I don't think I will be able to be a sponsor, but see also this recent discussion: https://pagure.io/packager-sponsors/issue/470
@rjones Thank you for the review. I will correct the spec file as you suggest. One thing I do not understand is your comment about "Doc Type". Right now I have "%doc README.md CONTRIBUTING" in the spec file. Is this incorrect ? Could you clarify your point please ?
The Doc comment is from Bugzilla system. There are advanced fields: Doc Type: If docs needed, set a value Doc Text: Doc Text is used for: This field is the basis of the errata or release note for this bug. It can also be used for change logs.
David, Got it. Thanks.
Yeah ignore the Doc Type field, it's just something that Bugzilla decided to alter randomly.
Taking this review.
I will also sponsor Damien once I've approved this review. :)
Thanks Neal !
Hi Neal, I updated the spec file and source RPM. Koji scratch build pass (https://koji.fedoraproject.org/koji/taskinfo?taskID=68831679). One thing I did not change is the License entry in the spec file. If I remove it, rpmlint throws errors and rpmbuild fails. So I am not super clear on what's to be done here. One thing that is not clear at all is if this entry specifies the license for the spec file itself or for the rpm content ? I have read https://fedoraproject.org/wiki/Licensing:Main#License_of_Fedora_SPEC_Files, but I do not find that text enlightening :) Note that I am fine with the MIT license, but I really do not understand to what it applies\ (spec or whole RPM). Please let me know what the right thing to do is here. Thanks !
The License: entry must be there. That tells under what license RPM content is. What you should remove is: # SPDX-License-Identifier: GPL-3.0-or-later # # Copyright (c) 2020 Western Digital Corporation or its affiliates. Btw, why not replace make install with %make_install ?
Oops. I thought I had done that too. Done now. Re-running koji scratch build. Will update once finished. Thanks !
Looking at the build log it seem fine, maybe just one nitpick. checking command to parse /usr/bin/nm -B output from gcc object... ./configure: line 6775: -e 's/^T .* \(.*\)$/extern int \1();/p' -e 's/^[ABCDGIRSTW][ABCDGIRSTW]* .* \(.*\)$/extern char \1;/p': No such file or directory ok You probably can ignore it, but something failed here in generated configure script. It should have printed "ok" instead of a line with "No such file or directory".
Weird... I do not see this with local builds. I may be missing one dependency ? Will check the build log in more details.
Running ./configure directly does not generate the error "checking command to parse /usr/bin/nm -B output from gcc object...". But I do get it if configure is run through rpmbuild. Googling around, it seems to be due to an incompatibility between binutils & gcc. I am on latest f34.
I updated the spec file to use %make_install
The spec file (a part of the Fedora project), the sources, and the binary files/RPM(s) produced by rpmbuild each have different licenses. Usually we don't add a license notice to spec files and they are implicitly licensed under MIT, although see the link I posted before about exceptions to this. The binaries are what License: refers to, and you need to specify that. It is copied into the headers of the binary RPMs and can be viewed using rpm -qip foo.rpm The source tarball often has the same or wider license as the binaries, and that is defined by whatever license files are included in the tarball and/or headers of source files. This does not need to be specified in the spec file, although in almost every case since it's the same as the binaries, it'll be what License says too.
Richard, I finally understood my confusion: you were talking about the license in the spec file header :) I removed it and updated the repo where I put the files. Thank you for the clarification.
Hi Neal, Any comment on this ? Anything else I need to change ? Thanks !
> Source0: https://github.com/westerndigitalcorporation/%{name}/archive/refs/tags/v%{version}.tar.gz You should just simplify Source0 to "%{url}/archive/v%{version}/%{name}-%{version}.tar.gz" Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags > %ldconfig_scriptlets Drop this, we don't need it in any stable Fedora release, and it's only needed for EPEL7, which this isn't targeted for.
Neal, I fixed the spec file as you suggested. Thanks for the review. Question: I saw that you changed the target version from f34 to rawhide. I am OK with that, but since the dm-zoned module is already in f34 (and f33), can this new package also go into these releases ? Or do new packages only go into the next Fedora version ?
Yes, once this is approved you can request branches for f34 and f33 and build the package for those releases as well.
@dcavalca Thank you for the clarification.
fedora-review has noticed an issue: - If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Note: No gcc, gcc-c++ or clang found in BuildRequires See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
I added "BuildRequires: gcc".
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= - Sources used to build the package match the upstream source, as provided in the spec URL. Note: Upstream MD5sum check error, diff is in /home/ngompa/1964787-dm- zoned-tools/diff.txt See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ ===== 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]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. 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: "Unknown or generated", "[generated file]", "GNU General Public License v3.0 or later", "FSF Unlimited License (with Retention) [generated file]", "FSF Unlimited License [generated file]", "GNU General Public License v2.0 or later [generated file]", "Expat License [generated file]", "GNU General Public License v2.0 or later", "FSF Unlimited License (with Retention) GNU General Public License, Version 2", "FSF Unlimited License (with Retention)", "BSD 2-clause "Simplified" License". 17 files have unknown license. Detailed output of licensecheck in /home/ngompa/1964787-dm-zoned- tools/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. [-]: 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 20480 bytes in 2 files. [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]: 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]: 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]: 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. [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]: 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: [!]: Package should not use obsolete m4 macros Note: Some obsoleted macros found, see the attachment. See: https://fedorahosted.org/FedoraReview/wiki/AutoTools [!]: Spec file according to URL is the same as in SRPM. Note: Bad spec filename: /home/ngompa/1964787-dm-zoned-tools/srpm- unpacked/dm-zoned-tools.spec See: (this test has no URL) [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: dm-zoned-tools-2.1.1-1.fc35.x86_64.rpm dm-zoned-tools-debuginfo-2.1.1-1.fc35.x86_64.rpm dm-zoned-tools-debugsource-2.1.1-1.fc35.x86_64.rpm dm-zoned-tools-2.1.1-1.fc35.src.rpm dm-zoned-tools.src: W: spelling-error %description -l en_US dmzadm -> DMZ 4 packages and 0 specfiles checked; 0 errors, 1 warnings. Rpmlint (debuginfo) ------------------- Checking: dm-zoned-tools-debuginfo-2.1.1-1.fc35.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Source checksums ---------------- https://github.com/westerndigitalcorporation/dm-zoned-tools/archive/v2.1.1/dm-zoned-tools-2.1.1.tar.gz : CHECKSUM(SHA256) this package : 369775e9246a868fee2f856c681d965149d012e8a8bfdf61769751945392c953 CHECKSUM(SHA256) upstream package : f1fffa4ca525308aece0b44cf6c555d504d02d16f1fb305f3e05231d1cbdd12a diff -r also reports differences Requires -------- dm-zoned-tools (rpmlib, GLIBC filtered): libblkid.so.1()(64bit) libblkid.so.1(BLKID_2.15)(64bit) libblkid.so.1(BLKID_2.17)(64bit) libc.so.6()(64bit) libdevmapper.so.1.02()(64bit) libdevmapper.so.1.02(Base)(64bit) libkmod.so.2()(64bit) libkmod.so.2(LIBKMOD_5)(64bit) libuuid.so.1()(64bit) libuuid.so.1(UUID_1.0)(64bit) rtld(GNU_HASH) dm-zoned-tools-debuginfo (rpmlib, GLIBC filtered): dm-zoned-tools-debugsource (rpmlib, GLIBC filtered): Provides -------- dm-zoned-tools: dm-zoned-tools dm-zoned-tools(x86-64) dm-zoned-tools-debuginfo: debuginfo(build-id) dm-zoned-tools-debuginfo dm-zoned-tools-debuginfo(x86-64) dm-zoned-tools-debugsource: dm-zoned-tools-debugsource dm-zoned-tools-debugsource(x86-64) AutoTools: Obsoleted m4s found ------------------------------ AC_PROG_LIBTOOL found in: dm-zoned-tools-2.1.1/configure.ac:25 Generated by fedora-review 0.7.6 (b083f91) last change: 2020-11-10 Command line :/usr/bin/fedora-review -b 1964787 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Generic, Shell-api Disabled plugins: fonts, Java, Python, PHP, SugarActivity, R, Haskell, Perl, Ocaml Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Issues: ======= - Sources used to build the package match the upstream source, as provided in the spec URL. Note: Upstream MD5sum check error, diff is in /home/ngompa/1964787-dm- zoned-tools/diff.txt See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Looks like the Makefile.am and the spec file in the tarball fetched from the spec file and what's in the SRPM are not the same, leading to this issue.
(In reply to Neal Gompa from comment #26) > Issues: > ======= > - Sources used to build the package match the upstream source, as provided > in the spec URL. > Note: Upstream MD5sum check error, diff is in /home/ngompa/1964787-dm- > zoned-tools/diff.txt > See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > > > Looks like the Makefile.am and the spec file in the tarball fetched from the > spec file and what's in the SRPM are not the same, leading to this issue. I had not pushed the latest changes to the spec file and makefile.am to github. Done now. I also removed the call to the deprecated AC_PROG_LIBTOOL from configure.ac (LT_INIT was called anyway before it, so it really was useless).
As I see that the issues will be resolved in the next version of the dm-zoned-tools, I think this is fine to go in now. PACKAGE APPROVED.
I've now sponsored you into the packager group. Congratulations on becoming a Fedora packager! :)
(In reply to Neal Gompa from comment #29) > I've now sponsored you into the packager group. Congratulations on becoming > a Fedora packager! :) Neal, This is great ! Thanks ! Regarding the next steps, is the below link correct as a description of what I need to do next ? https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Yep!
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/dm-zoned-tools