Bug 971103 - Review Request: bsd-mailx - Simple mail user agent
Review Request: bsd-mailx - Simple mail user agent
Status: CLOSED ERRATA
Product: Fedora EPEL
Classification: Fedora
Component: Package Review (Show other bugs)
el6
All Linux
medium Severity medium
: ---
: ---
Assigned To: Douglas Schilling Landgraf
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-06-05 13:03 EDT by Peter Schiffer
Modified: 2013-08-01 16:38 EDT (History)
9 users (show)

See Also:
Fixed In Version: bsd-mailx-8.1.2-4.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-01 16:38:18 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
dougsland: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Peter Schiffer 2013-06-05 13:03:28 EDT
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
Comment 1 Peter Schiffer 2013-06-05 14:12:08 EDT
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.
Comment 2 Douglas Schilling Landgraf 2013-06-06 21:34:50 EDT
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
Comment 3 Peter Schiffer 2013-06-07 07:55:06 EDT
Hi Douglas,

this is only for EL6. I've added those because rpmlint in EL6 was complaining about it. Should I remove them?

peter
Comment 4 Douglas Schilling Landgraf 2013-06-07 08:46:49 EDT
(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.
Comment 5 Peter Schiffer 2013-06-07 09:20:29 EDT
Done, .spec file and srpm are updated.
Comment 6 Douglas Schilling Landgraf 2013-06-07 21:53:35 EDT
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
Comment 7 Peter Schiffer 2013-06-11 18:55:56 EDT
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
Comment 8 Douglas Schilling Landgraf 2013-06-11 19:42:54 EDT
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
Comment 9 Peter Schiffer 2013-06-12 06:29:36 EDT
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
Comment 10 Ralf Corsepius 2013-06-12 08:13:47 EDT
(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.
Comment 11 Peter Schiffer 2013-06-12 09:45:55 EDT
(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
Comment 12 Ralf Corsepius 2013-06-12 10:33:05 EDT
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".
Comment 13 Peter Schiffer 2013-06-12 11:50:47 EDT
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...
Comment 14 Rex Dieter 2013-06-13 12:08:24 EDT
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.
Comment 15 Peter Schiffer 2013-06-18 14:27:54 EDT
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.
Comment 16 Ralf Corsepius 2013-06-19 11:02:42 EDT
(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).
Comment 17 Peter Schiffer 2013-06-19 11:33:40 EDT
(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..
Comment 18 Peter Schiffer 2013-06-20 04:42:20 EDT
I've just updated the .spec file and srpm, now with OpenBSD as upstream.
Comment 19 Eduardo Echeverria 2013-06-21 00:03:05 EDT
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
Comment 20 Peter Schiffer 2013-06-21 06:40:28 EDT
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
Comment 21 Douglas Schilling Landgraf 2013-06-21 14:01:06 EDT
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
Comment 23 Douglas Schilling Landgraf 2013-06-24 12:23:00 EDT
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
Comment 24 Peter Schiffer 2013-06-25 07:22:43 EDT
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
Comment 25 Peter Schiffer 2013-06-25 07:35:30 EDT
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
Comment 26 Gwyn Ciesla 2013-06-25 07:42:33 EDT
devel is created automatically, retire it using the EOL procedure in the wiki.
Comment 27 Peter Schiffer 2013-06-25 10:13:37 EDT
Oh, OK. Thanks for the info.
Comment 28 Fedora Update System 2013-06-25 10:14:19 EDT
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
Comment 29 Fedora Update System 2013-06-25 20:42:07 EDT
bsd-mailx-8.1.2-4.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 30 Fedora Update System 2013-08-01 16:38:18 EDT
bsd-mailx-8.1.2-4.el6 has been pushed to the Fedora EPEL 6 stable repository.

Note You need to log in before you can comment on or make changes to this bug.