Spec URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt/fedora-rawhide-x86_64/04346132-neomutt/neomutt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt/fedora-rawhide-x86_64/04346132-neomutt/neomutt-20220429-1.fc37.src.rpm Description: NeoMutt - Email Client NeoMutt is a fork of the venerable Mutt Email Client. It has extra features, such as Notmuch integration and a much-improved codebase Fedora Account System Username: flatcap
Here are the things I've noticed that need to be fixed. =============================================================================== The licensecheck utility found multiple files under different licenses. These all must be reflected in the License field. There also must be a comment explaining the multiple license breakdown. Public Domain: contrib/colorschemes/zenburn.neomuttrc pgpewrap.c docs/mbox.5 MIT: contrib/colorschemes/neonwolf-256.neomuttrc test/acutest.h BSD: autosetup/mutt-gettext.tcl autosetup/mutt-iconv.tcl contrib/hcache-bench/neomutt-hcache-bench.sh mutt/queue.h Unlicense: contrib/keybase/ https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_field =============================================================================== Remove the BugURL line. The Fedora build system (koji) will set this to https://bugz.fedoraproject.org/<packagename>, which redirects to a list of open bugzilla bugs. I can't find any documentation about this, but you can verify it on a Fedora system by running `rpm -q --qf '%{bugurl}\n' <anypackagename>`. =============================================================================== It is recommended to follow the following format for the Source URL. -Source: https://github.com/neomutt/neomutt/archive/%{version}.tar.gz +Source: https://github.com/neomutt/neomutt/archive/%{version}/%{name}-%{version}.tar.gz https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags =============================================================================== Each patch should have a comment about their upstream status, or justification why they are downstream-only. https://docs.fedoraproject.org/en-US/packaging-guidelines/PatchUpstreamStatus/ =============================================================================== No C compiler is listed as a build requirement. The build currently works because the perl build requirement recursively pulls in gcc, but this is not guaranteed to always be the case. Add `BuildRequires: gcc`. https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/#_buildrequires_and_requires https://docs.fedoraproject.org/en-US/packaging-guidelines/#compiler =============================================================================== The conditional _pkgdocdir macro definition can be removed. That's already defined on all maintained versioned of Fedora and RHEL. =============================================================================== The invocations of ./configure, make, and make install should be replaced by the standardized macro equivalents of %configure, %make_build, and %make_install. This will allow the removal of --sysconfdir, _smp_mflags, and DESTDIR, as those are already included in the macros. =============================================================================== The license file should be marked as %license, not %doc. This ensures the license is included even when the transaction flag nodocs is set. -%doc *.md +%license LICENSE.md +%doc AUTHORS.md ChangeLog.md INSTALL.md README.md SECURITY.md https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text =============================================================================== The package places files in /usr/libexec/neomutt/, but it doesn't own that directory. The easiest fix it to just have %files own that directory recursively. It should also be referenced as %{_libexecdir}. -/usr/libexec/neomutt/pgpewrap -/usr/libexec/neomutt/smime_keys +%{_libexecdir}/neomutt/
I had a few other questions/suggestions that may or may not lead to changes. =============================================================================== Source1 is just installled as a %doc file. Could this be added to the upstream GitHub repo and maintained there? That would also avoid the need to manually copy it to the build directory in %prep, as it would already be there from the tarball extraction. =============================================================================== Several conditionals are disabled for EL releases. The comments indicate this is due to packages missing from RHEL, or not working from RHEL. Some of these are incorrect. Additionally, Fedora packages have the benefit of being able to be built in EPEL, where lots of packages that are missing from RHEL are maintained by the community for use on RHEL. This will probably allow many of these conditionals to be re-enabled. If the provided version is too old to work or is otherwise broken, then the affected conditional can be left disabled for that release. idn: libidn-devel in rhel7, rhel8, and epel9 lmdb: lmdb-devel in epel7, rhel8, and rhel9 autocrypt: sqlite-devel in rhel7, rhel8, and rhel9 idn2: libidn2-devel in epel7, rhel8, and rhel9 lua: lua-devel in rhel7, rhel8, and rhel9 lz4: lz4-devel in rhel7, rhel8, and rhel9 notmuch: notmuch-devel in epel8 and epel9 pcre2: pcre2-devel in rhel7, rhel8, and rhel9 zstd: libzstd-devel epel7, rhel8, and rhel9 =============================================================================== Consider replacing instances of $RPM_BUILD_ROOT with %{buildroot}. This is not required. The guidelines only state to pick one and use it consistently, but I prefer the latter. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_using_buildroot_and_optflags_vs_rpm_build_root_and_rpm_opt_flags =============================================================================== Why is the %{_docdir}/neomutt directory being deleted on RHEL during %install? =============================================================================== There are a large number of %doc files. Consider splitting them out to a neomutt-doc subpackage. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
Thanks for the thorough review! I've spent all day trying to understand my .spec file. For six years, I've been hacking in ignorance :-) I'll have some updates and questions, soon.
Sure thing, take your time. If you would like interactive help feel free to ping me on either IRC (carlwgeorge on libera.chat) or Matrix (@carlwgeorge:matrix.org).
Created attachment 1881493 [details] RPM Spec file for NeoMutt Updated .spec file following review
I've covered most of your requests, and learned a lot along the way. I think I now understand how the Centos Stream, RHEL, EPEL fit together. It's all gone OK, except for RHEL7 -- it's just too old. There's GPGME, but it's not new enough for Autocrypt; There's Lua, but it's too old to compile; and our biggest feature, Notmuch, is a non-starter. If we don't even try to support RHEL7, then the spec file is trivial. =============================================================================== Simple stuff, first: > Remove the BugURL line > format for the Source URL > Each patch should have a comment > BuildRequires: gcc > _pkgdocdir macro definition can be removed > The package places files in /usr/libexec/neomutt/ > replacing instances of $RPM_BUILD_ROOT with %{buildroot} Done =============================================================================== > licensecheck utility I've done my best to comment the exceptions from GPLv2+. Suggestions welcome. =============================================================================== > %configure, %make_build, and make_install Done. This _mostly_ works. RHEL7 seems to require me to add `CC=gcc` to %configure (other systems seem to work ok without it) =============================================================================== > license file should be marked as %license, not %doc > -%doc *.md > +%license LICENSE.md > +%doc AUTHORS.md ChangeLog.md INSTALL.md README.md SECURITY.md I did this, but then it complained that LICENSE.md was installed but not packaged. Currently the spec has both %license and %doc for LICENSE.md =============================================================================== > Source1 is just installed as a %doc file Good point. I've upstreamed that and a couple of the patches. However, the spec file won't work until I make a release with the changes. I'll do that soon. =============================================================================== > Several conditionals are disabled for EL releases. > this is due to packages missing from RHEL, or not working from RHEL Yeah, the spec file is very old :-) I've stripped out the unnecessary conditionals, leaving only RHEL7. EPEL now contains everything NeoMutt needs. =============================================================================== > Why is the %{_docdir}/neomutt directory being deleted on RHEL during install? Ah! I discovered the reason (dating back six years). NeoMutt installs the docs into /usr/share/doc/neomutt RHEL7 (and only this) expects its docs in neomutt-VERSION At the end of every build, Mock would complain about all the unpackaged files. I've replaced the `rm -fr` with a `mv` and now it's happier. =============================================================================== > There are a large number of %doc files. Consider splitting them out I gave this a try, but I couldn't figure it out. I was hoping to be able to just move the %doc, etc, into %package doc %files doc ... Unfortunately, this expected files in /usr/share/doc/neomutt-doc/ Is there some simple way to set the install dir to just 'neomutt'? When you next have some time, I'm ready for your next round of reviewing. Many thanks!
> It's all gone OK, except for RHEL7 -- it's just too old. > If we don't even try to support RHEL7, then the spec file is trivial. It's up to you. You can build for as many or as few EPEL branches as you want. If you prefer to keep the spec file cleaner, it's ok to avoid EPEL7. You can always add it later if you change your mind. > I've done my best to comment the exceptions from GPLv2+. > Suggestions welcome. Looks great! > Done. This _mostly_ works. > RHEL7 seems to require me to add `CC=gcc` to %configure > (other systems seem to work ok without it) I imagine it's a problem with the much older version of gcc, or perhaps just the older rpm macros. Probably not worth the hassle of pinning it down when we can just skip building for EPEL7. > I did this, but then it complained that LICENSE.md was installed but not > packaged. Currently the spec has both %license and %doc for LICENSE.md That is strange. The one line should be sufficient to copy the file to the correct location and own the file. Do you have a local ~/.rpmmacros file that is overriding %license? Does it happen when building for Fedora and EPEL, or just EPEL? > I gave this a try, but I couldn't figure it out. That's ok, it's not strictly required. We don't need to block the review on it. In the future if anyone strongly wants it they can send a pull request to implement it. > Is there some simple way to set the install dir to just 'neomutt'? Yes, you can do `%global _docdir_fmt %{name}`. Be aware this may cause issues if multiple subpackages share ownership of any of the doc files (see bug 1625015). I would put AUTHORS.md, ChangeLog.md, README.md, and SECURITY.md only in the main package, and everything else only in neomutt-doc. Also, the INSTALL.md file should be omitted entirely, as it won't be relevant for people that install the rpm.
I took a look at the spec file attachment, and things are looking real good, but two small new issues were introduced. I imagine you added `rpm --showrc` for troubleshooting, but it should not be included in the final spec file. Pro-tip, this command also works in a container. Make sure to install redhat-rpm-config (and epel-rpm-macros for EL releases) for all macros to be available. Your multi-line configure is missing an escape after --full-doc, so the conditional flags after that won't be included. One more thing, please post revisions to the package as new `Spec URL:` and `SRPM URL:` lines (with working URLs), similar to the original comment template. You don't have to repeat the `Description:` or `Fedora Account System Username:` lines. The fedora-review tool doesn't work with attachments, it expects URLs in that format and will pick the last occurrences of those lines in the comments of the given bug.
I see what's happening with LICENSE.md. %license handles copying the file to /usr/share/licenses/neomutt/, but the upstream Makefile is putting another copy in /usr/share/doc/neomutt/. I'm not sure if you'd prefer to adjust the upstream Makefile or just create a patch for this spec file to not install that file (or INSTALL.md) only during the rpm build. https://github.com/neomutt/neomutt/blob/d0f9f8f3da210c085d55e8e33dd64b0996d53957/docs/Makefile.autosetup#L27-L30
Subscribing to it as some of my coworkers want to run neomutt at work As per the dual license files - yeah I normally just delete them at the end of %install rather than messing with Makefile, unless the changes are clean enough that they can be upstreamed. Once this is in could it be branched for EPEL 9 and 8 as well? Thanks!
Spec URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt-test/fedora-36-x86_64/04502025-neomutt/neomutt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt-test/fedora-36-x86_64/04502025-neomutt/neomutt-20220606-1.fc36.src.rpm Sorry for the slow reply. Things have been upstreamed and we're down to two Fedora-specific patches. > > It's all gone OK, except for RHEL7 > it's ok to avoid EPEL7 Thanks. Then I choose not to support RHEL7/EPEL7. NeoMutt offers almost nothing over Mutt on that platform. > I imagine you added `rpm --showrc` for troubleshooting D'oh. Removed > Your multi-line configure is missing an escape Fixed > One more thing, please post revisions to the package as new > `Spec URL:` and `SRPM URL:` lines (with working URLs) Done Thanks
This is looking real good. We do still need to put the finishing touches on the documentation/license handling. I just realized this spec is doing something that is forbidden in the guidelines [0]. > Marking a relative path with %doc in the %files section will cause RPM to copy the referenced file or directory from %_builddir to the proper location for documentation. Files can also be placed in %_pkgdocdir, and the build scripts of the software being packaged may do this automatically when called in %install. However, mixing these methods is problematic and may result in duplicated or conflicting files, so use of %doc with relative paths and installation of files directly into %_pkgdocdir in the same source package is forbidden. The upstream Makefile is installing the files into %{buildroot}%{_pkgdocdir}, then %doc is doing it again. This is also why we run into issues not listing all the files as %doc. What we should do is rm INSTALL.md and LICENSE.md from %{_pkgdocdir} during %install (right after %make_install), then only list %{_pkgdocdir} in %files, removing all %doc lines with relative paths. I tried this and it worked, except it needed one more tweak to resolve an "ambiguous python shebang" error (this can probably be fixed upstream). %{make_install} +rm %{buildroot}%{_pkgdocdir}/INSTALL.md %{buildroot}%{_pkgdocdir}/LICENSE.md +sed -e '1 s/python/python3/' -i %{buildroot}%{_pkgdocdir}/keybase/keybase.py %license LICENSE.md -%doc LICENSE.md -%doc AUTHORS.md ChangeLog.md INSTALL.md README.md SECURITY.md -%doc docs/CODE_OF_CONDUCT.md docs/CONTRIBUTING.md -%doc docs/*.txt -%doc docs/*.html -%doc docs/mime.types -%doc contrib/account-command -%doc contrib/colorschemes -%doc contrib/hcache-bench -%doc contrib/keybase -%doc contrib/logo -%doc contrib/lua -%doc contrib/samples -%doc contrib/vim-keys -%doc contrib/oauth2 +%{_pkgdocdir} [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_documentation
While at a conference last week I was chatting with another Fedora packager who uses neomutt (FAS steve). He offered to co-maintain the package once it's approved. I can help you set up that permission when then time comes. Have you had a chance to fix up the documentation as described in the previous comment?
Damn! Sorry, I missed the notification. I'm on it. Since then, we've been shuffling files around. We've dropped some old contrib cruft and moved all the non-doc files out of /usr/share/doc/neomutt. Contrib will now be installed to the data dir: /usr/share/neomutt
Hi Steve! Thanks for your offer to help maintain the package.
Spec URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt-test/fedora-36-x86_64/04570893-neomutt/neomutt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt-test/fedora-36-x86_64/04570893-neomutt/neomutt-20220628-1.fc36.src.rpm Note: This has been built against a development version of NeoMutt, which has some bugs. > We do still need to put the finishing touches on the documentation/license handling. > %{make_install} > +rm %{buildroot}%{_pkgdocdir}/INSTALL.md %{buildroot}%{_pkgdocdir}/LICENSE.md done > +%{_pkgdocdir} done. much tidier :-) > one more tweak to resolve an "ambiguous python shebang" error > +sed -e '1 s/python/python3/' -i %{buildroot}%{_pkgdocdir}/keybase/keybase.py We decided instead to drop the keybase script -- unmaintained, unused ---------------------- Other changes: The contrib files (all the non-docs) have been moved to /usr/share/neomutt I've added: `%{_datadir}` to the `%files` section to cover them. I've added a patch: neomutt-fedora_colors.patch (Each distro seems to have its own custom colours) The spec *used* to grep the config out of another file (now deceased). License: as keybase.py has gone, so has one of the license types (Unlicense). Thanks!
> I've added: `%{_datadir}` to the `%files` section to cover them. This should be %{_datadir}/neomutt. Packages shouldn't duplicate directory ownership with the filesystem package [0]. -%{_datadir} +%{_datadir}/neomutt > I've added a patch: neomutt-fedora_colors.patch This patch creates a single file, which is later read with cat and written to a file in the buildroot. You can just have that file as an additional source, then put it into place during %install. +# Fedora color scheme +Source1: fedora_colors.rc -# Fedora color scheme -Patch3: neomutt-fedora_colors.patch -%patch3 -p1 -b .fedora_colors -cat contrib/fedora_colors.rc >> %{buildroot}%{_sysconfdir}/neomuttrc +install -D -p -m 0644 %{SOURCE1} %{buildroot}%{_sysconfdir}/neomuttrc One more thing to be aware of, the built package is getting an automatic dependency on /usr/bin/python3 due to the shebang lines of executable files in %{_datadir}/neomutt (account-command/macos-keychain/keychain.py and oauth2/mutt_oauth2.py). If you want to avoid that dependency, chmod those files during %install to make them not executable. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directory_ownership
Spec URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt-test/fedora-36-x86_64/04583618-neomutt/neomutt.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/flatcap/neomutt-test/fedora-36-x86_64/04583618-neomutt/neomutt-20220629-1.fc36.src.rpm > This should be %{_datadir}/neomutt of course. fixed > > I've added a patch: neomutt-fedora_colors.patch > You can just have that file as an additional source done > then put it into place during %install. > +install -D -p -m 0644 %{SOURCE1} %{buildroot}%{_sysconfdir}/neomuttrc I changed this to `cat` because we want to append the /etc/neomuttrc that we installed earlier. > the built package is getting an automatic dependency on /usr/bin/python3 ok, understood. I don't have a problem with that. I did a quick check of a minimal Fedora install and that included python3 (it was a requirement for dnf). Thanks!
Thanks for making all those adjustments. The spec file looks good now. I do need to point out that the changelog entries going forward will need to follow one of the formats specified in the guidelines [0]. Basically just use 20220629-1 instead of NeoMutt-20220629. The historical entries can stay in a non-compliant format. I've sponsored you into the packagers group [1], and this package is APPROVED. [0] https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs [1] https://accounts.fedoraproject.org/user/flatcap/ Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= - Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 3614720 bytes in 20 files. See: https://docs.fedoraproject.org/en-US/packaging- guidelines/#_documentation ===== 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", "GNU General Public License, Version 2", "*No copyright* GNU General Public License, Version 2", "GNU General Public License v2.0 or later", "*No copyright* Public domain", "BSD 2-Clause with views sentence", "GNU General Public License v3.0 or later", "BSD 2-Clause License", "Public domain", "GNU General Public License", "BSD 3-Clause License", "MIT License". 119 files have unknown license. Detailed output of licensecheck in /home/carl/packaging/reviews/neomutt/2086484-neomutt/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [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]: The spec file handles locales properly. [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. [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]: %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 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]: 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]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [!]: Sources can be downloaded from URI in Source: tag Note: Could not download Source0: https://github.com/neomutt/neomutt/archive/20220629/neomutt-20220629.tar.gz See: https://docs.fedoraproject.org/en-US/packaging- guidelines/SourceURL/ [-]: 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]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: SourceX tarball generation or download is documented. Note: Package contains tarball without URL, check comments [-]: 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. [-]: %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]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [-]: Large data in /usr/share should live in a noarch subpackage if package is arched. Note: Arch-ed rpms have a total of 7516160 bytes in /usr/share [x]: Rpmlint is run on debuginfo package(s). Note: There are rpmlint messages (see attachment). [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/neomutt
> The spec file looks good now. Great! > the changelog entries going forward will need to ... > ... use 20220629-1 instead of NeoMutt-20220629 OK, I've updated my template > The historical entries can stay in a non-compliant format. I fixed them too (trivial) > I've sponsored you into the packagers group [1], and this package is APPROVED. Thanks for your help
https://bodhi.fedoraproject.org/updates/FEDORA-2022-0bfee9488f