Spec URL: https://dl.dropboxusercontent.com/u/15321270/bsd-mailx.spec SRPM URL: https://dl.dropboxusercontent.com/u/15321270/bsd-mailx-8.1.2-1.el6.src.rpm Description: bsd-mailx package is intended to be used in epel6 repository as an alternative to the mailx package available in the RHEL-6 and derived distributions Fedora Account System Username: pschiffe
I've updated .spec file to be compliant with the RHEL-6 rpmlint (added buildroot tag, defattr, %clean section). Updated .spec and srpm are on the same URL as in description.
Hi Peter, (In reply to Peter Schiffer from comment #1) > I've updated .spec file to be compliant with the RHEL-6 rpmlint (added > buildroot tag, defattr, %clean section). Updated .spec and srpm are on the > same URL as in description. Are you going to ship for EL5? Otherwise you don't need the below. %defattr(-,root,root,-) rpm 4.4 or later do not need defattr: http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions %install rm -rf $RPM_BUILD_ROOT https://fedoraproject.org/wiki/EPEL:Packaging#Prepping_BuildRoot_For_.25install Thanks Douglas
Hi Douglas, this is only for EL6. I've added those because rpmlint in EL6 was complaining about it. Should I remove them? peter
(In reply to Peter Schiffer from comment #3) > Hi Douglas, > > this is only for EL6. I've added those because rpmlint in EL6 was > complaining about it. Should I remove them? > > peter Yes. I will double check the rpmlint output in the review.
Done, .spec file and srpm are updated.
Hi Peter, Please break Source1 into multiple patch lines, it helps to debug things like below: Rpmlint ------- Checking: bsd-mailx-8.1.2-1.fc18.x86_64.rpm bsd-mailx.x86_64: E: missing-call-to-setgroups /usr/bin/bsd-mailx 1 packages and 0 specfiles checked; 1 errors, 0 warnings. Looks like this have been introduced by the patches applied. Can you please double check this error? Thanks Douglas
Hi Douglas, ah yes, I know about this error and I've already checked it. In this case, this shouldn't be a problem because the gid is not changed, the bsd-mailx binary doesn't have suid and sgid flags. This error was introduced by 03-Base-fixes-2.patch. I don't think that it's realy worthy expanding the for loop applying upstream patches for better debugging things like this (which is probably only this one error, considering the bsd-mailx is not actively developed any more). peter
Hi Peter, (In reply to Peter Schiffer from comment #7) > Hi Douglas, > > ah yes, I know about this error and I've already checked it. In this case, > this shouldn't be a problem because the gid is not changed, the bsd-mailx > binary doesn't have suid and sgid flags. > > This error was introduced by 03-Base-fixes-2.patch. I don't think that it's > realy worthy expanding the for loop applying upstream patches for better > debugging things like this (which is probably only this one error, > considering the bsd-mailx is not actively developed any more). > I am sorry, I disagree. The spec should be always simple and clear, without that I am not able to approve it, but you can always request other people to review it. BTW, since bsd-mailx is not acively developed any more, you should be aware of: http://fedoraproject.org/wiki/Orphaned_package_that_need_new_maintainers#Claiming_Ownership_of_a_Deprecated_Package "If you really want to maintain a deprecated package, you need to be aware that if upstream is dead, fixing release critical bugs, etc **becomes your responsibility.***" Please let me know if you have any additional question. Thanks Douglas
Douglas, listing 24 non-redhat patches doesn't make .spec file clear and simple, in my opinion. Upstream is Debian, which is not dead. Not actively developed means no new features are added any more, but critical bugs and security issues are fixed by them, so there is no problem. I meant that I don't expect the upstream source code to be updated very often. Anyway, thank you for your part of the review. peter
(In reply to Peter Schiffer from comment #9) > Douglas, > > listing 24 non-redhat patches doesn't make .spec file clear and simple, in > my opinion. > > Upstream is Debian, I agree with Douglas - We should not inherit packages from secondary sources but from primary sources. According to http://ftp-master.metadata.debian.org/changelogs/main/b/bsd-mailx/unstable_changelog upstream is OpenBSD. Anyway, http://ftp.de.debian.org/ is the German mirror of the Debian package repositories. The "generic Url" would be ftp://ftp.debian.org. However, the actual Debian sources are in Debian's git: git://anonscm.debian.org/users/robert/bsd-mailx.git (c.f. http://anonscm.debian.org/gitweb/?p=users/robert/bsd-mailx.git) Also, Debian's git is telling their package received its last change 2011-11-20. To me, this doesn't sound more maintained than the OpenBSD upstream.
(In reply to Ralf Corsepius from comment #10) > > According to > http://ftp-master.metadata.debian.org/changelogs/main/b/bsd-mailx/ > unstable_changelog upstream is OpenBSD. Yes, bsd-mailx is from OpenBSD, but without Debian patches, it's unusable on Linux. > Anyway, http://ftp.de.debian.org/ is the German mirror of the Debian package > repositories. The "generic Url" would be ftp://ftp.debian.org. I agree with removing the ".de" from the SourceX tags. > However, the actual Debian sources are in Debian's git: > git://anonscm.debian.org/users/robert/bsd-mailx.git > (c.f. http://anonscm.debian.org/gitweb/?p=users/robert/bsd-mailx.git) I think, I'll change the URL from http://packages.debian.org/sid/bsd-mailx to http://packages.debian.org/source/sid/bsd-mailx which contains links for both, the tarball and the Debian's git. What do you think? peter
I think you should point to OpenBSD sources, not use the Debian tarball and to adopt the Debian patches individually. This is what most other packagers do. They adopt "Debian patches", in cases they are sensible, because in longer terms patches tend to diverge and you will end up with "patching patches". Besides this, I can not avoid seriously asking whether this package is worth packaging at all, because both OpenBSD upstream and Debian seem to be "dormant"/ "on hiatus".
This package is intended only for EPEL6, because we need it as an alternative to the mailx package. Mailx package in RHEL-5 is based on this, bsd-mailx, but in RHEL-6 it's based on heirloom mailx, which is not 100% percent backwards compatible with bsd-mailx. Also there might be a problem with OpenBSD sources, because so far I found only CVS urls for them and no http url, but I can try to look again...
In my humble opinion, how the patches are fetched/applied is a matter of personal preference. As there is to currently policy or MUST guideline to forbid it, reviewers using this as justification to block/decline a review is inapprorpriate.
I've updated .spec file and srpm package. I've kept Debian as upstream. OpenBSD has only CVS method of getting their sources, without any web pages dedicated to their packages, or possibility to download tarballs of packages. (That's the reason why Debian is importing sources from OpenBSD CVS to their git I think). But I've "adopted" Debian patches.. So, the current state of bsd-mailx package: Upstream is Debian, with package web page, git with clean (unmodified) upstream source files and possibility to download tarball. I've dropped the second Debian specific source archive, instead, all Debian patches are adopted as ours.
(In reply to Peter Schiffer from comment #15) > I've updated .spec file and srpm package. > > I've kept Debian as upstream. OpenBSD has only CVS method of getting their > sources, without any web pages dedicated to their packages, or possibility > to download tarballs of packages. (That's the reason why Debian is importing > sources from OpenBSD CVS to their git I think). No. Debian imports all packages into their VCS, independently of a package's master format (They usually unpackage the tarball).
(In reply to Ralf Corsepius from comment #16) > No. Debian imports all packages into their VCS, independently of a package's > master format (They usually unpackage the tarball). Not 100% true. I've checked man-db, zlib, stgit (randomly) and none of them is in their VCS. But you have the point, that they have imported many other packages no matter the master format is. http://packages.debian.org/source/sid/man-db http://packages.debian.org/source/sid/zlib http://packages.debian.org/source/sid/stgit Anyway, I would still rather use the Debian as an upstream..
I've just updated the .spec file and srpm, now with OpenBSD as upstream.
Hi @Peter I haven't reviewed the package thoroughly, but I have a comments for you - The package has buildroot, for el6 it is not necessary, see http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag - Every time that you make changes to the spec, you should bump the release number - There are a weird requires in the retrieved requires from rpmbuild Requires (from fedora-review) -------- bsd-mailx (rpmlib, GLIBC filtered): /bin/sh /usr/sbin/alternatives config(bsd-mailx) Please see: http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering libbsd.so.0()(64bit) libbsd.so.0(LIBBSD_0.0)(64bit) libbsd.so.0(LIBBSD_0.2)(64bit) libc.so.6()(64bit) rtld(GNU_HASH) - A question, is bsd-mailx a fork of mailx.? I quote As a general rule, Fedora packages must NOT contain any usage of the Conflicts: field https://fedoraproject.org/wiki/Packaging:Conflicts Cheers Eduardo
Hi Eduardo. (In reply to Eduardo Echeverria from comment #19) > Hi @Peter > > I haven't reviewed the package thoroughly, but I have a comments for you > > - The package has buildroot, for el6 it is not necessary, see > http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag Fixed. > - Every time that you make changes to the spec, you should bump the release > number OK. > - There are a weird requires in the retrieved requires from rpmbuild > > Requires (from fedora-review) > -------- > bsd-mailx (rpmlib, GLIBC filtered): > /bin/sh > /usr/sbin/alternatives > > config(bsd-mailx) Please see: > http://fedoraproject.org/wiki/Packaging:AutoProvidesAndRequiresFiltering This is there because of the %config macro in the %files section, and it's in both, requires and provides. With this rpm feature it's possible to separate config files to the stand-alone rpm packages (or something like that). IMHO there's no need to filter this.. > > - A question, is bsd-mailx a fork of mailx.? Other way around. mailx (specifically heirloom-mailx is fork of bsd-mailx). We need bsd-mailx in EPEL-6 for compatibility reasons. > I quote > As a general rule, Fedora packages must NOT contain any usage of the > Conflicts: field > https://fedoraproject.org/wiki/Packaging:Conflicts This is necessary because of the alternatives. I've added alternatives support to the mailx-12.4-7 in RHEL-6, so both, mailx and bsd-mailx can be installed. Older mailx and bsd-mailx won't play nice. Also, your quote is missing second part: As a general rule, Fedora packages must NOT contain any usage of the Conflicts: field. ... However, there are some cases in which using the Conflicts: field is appropriate and acceptable. https://dl.dropboxusercontent.com/u/15321270/bsd-mailx.spec https://dl.dropboxusercontent.com/u/15321270/bsd-mailx-8.1.2-2.el6.src.rpm Thank you for your comments! peter
Hi Peter, We are almost there, can you please handle: %clean rm -rf $RPM_BUILD_ROOT It's for EPEL5. http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean Thanks Douglas
Fixed. https://dl.dropboxusercontent.com/u/15321270/bsd-mailx.spec https://dl.dropboxusercontent.com/u/15321270/bsd-mailx-8.1.2-3.el6.src.rpm peter
Hi, Thanks all for helping on this package. Peter, thanks for handling all comments for improvement. There is a final comment that you can do it in parallel of packaging (see below). 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]: 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]: %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 requires other packages for directories it uses. [-]: Package uses nothing in %doc for runtime. [x]: Package is not known to require ExcludeArch. [x]: Package complies to the Packaging Guidelines [-]: 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 %doc. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "BSD (3 clause)". Detailed output of licensecheck in /home/fedora/971103 -bsd-mailx/licensecheck.txt [x]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [-]: Package does not generate any conflict. It conflicts with mailx < 12.4-7. see Bugzilla comment#20 [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]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [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]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [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]: Each %files section contains %defattr if rpm < 4.4 [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [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 do not use a name that already exist [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. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local [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: No rpmlint messages. ===== 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. [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 [x]: 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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [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]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Uses parallel make. [x]: SourceX is a working URL. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [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: bsd-mailx-8.1.2-3.fc18.x86_64.rpm 1 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint bsd-mailx 1 packages and 0 specfiles checked; 0 errors, 0 warnings. # echo 'rpmlint-done:' Requires -------- bsd-mailx (rpmlib, GLIBC filtered): /bin/sh /usr/sbin/alternatives config(bsd-mailx) (bz comment#20) libbsd.so.0()(64bit) libbsd.so.0(LIBBSD_0.0)(64bit) libbsd.so.0(LIBBSD_0.2)(64bit) libc.so.6()(64bit) rtld(GNU_HASH) Provides -------- bsd-mailx: bsd-mailx bsd-mailx(x86-64) config(bsd-mailx) Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review -b 971103 Comments =========== If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. Could you please handle that? The above comment can be fixed in parallel of packaging. Final Status: APPROVED
Added license file. Thanks for the review and all comments! https://dl.dropboxusercontent.com/u/15321270/bsd-mailx.spec https://dl.dropboxusercontent.com/u/15321270/bsd-mailx-8.1.2-4.el6.src.rpm peter
New Package SCM Request ======================= Package Name: bsd-mailx Short Description: Simple mail user agent Owners: pschiffe Branches: el6 no-fedora-devel-pls InitialCC: This package is intended only for EPEL-6 branch, not for Fedora. Thanks, peter
devel is created automatically, retire it using the EOL procedure in the wiki.
Oh, OK. Thanks for the info.
bsd-mailx-8.1.2-4.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/bsd-mailx-8.1.2-4.el6
bsd-mailx-8.1.2-4.el6 has been pushed to the Fedora EPEL 6 testing repository.
bsd-mailx-8.1.2-4.el6 has been pushed to the Fedora EPEL 6 stable repository.