Bug 188400 - Review Request: ssmtp
Review Request: ssmtp
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Patrice Dumas
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2006-04-08 21:58 EDT by manuel wolfshant
Modified: 2014-10-29 12:28 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-11-13 17:34:25 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description manuel wolfshant 2006-04-08 21:58:55 EDT
Spec Name or Url: http://wdl.lug.ro/linux/ssmtp/ssmtp.spec
SRPM Name or Url: http://wdl.lug.ro/linux/ssmtp/ssmtp-2.61-1.src.rpm
Description: 
This is sSMTP, a program that replaces sendmail on workstations that should
 send their mail via the departmental mailhub from which they pick up their
 mail (via pop, imap, rsmtp, pop_fetch, NFS... or the like).  This program
 accepts mail and sends it to the mailhub, optionally replacing the domain in
 the From: line with a different one.


Thanks for reviewing
Comment 1 Patrice Dumas 2006-04-09 06:06:11 EDT
Don't you need a sponsor? If you don't I'll assign that bug to myself, otherwise
you should seek a sponsor.

Here are my comments, even though I cannot sponsor you:

* %configopt is useless, just substitute the value

* Shouldn't provide smtpdaemon, as it doesn't accepts mail. You can have a look at 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=165957#c3

* similarly it is arguable whether it should provide MTA or not

* it is useless to provide explicitely files that are allready distributed,
namely %{_sbindir}/ssmtp

* use consistently %{buildroot} or $RPM_BUILD_ROOT

* don't install ssmtp in sbindir but in bindir

* generate_config_alt shouldn't be called that way, but maybe ssmtp_config

* the ssmtp man page shouldn't have an added .ssmtp. And mta-ssmtpconfman and
mta-ssmtpman are useless in the alternatives call.

* the alternatives --auto seems dubious to me.

* %{_sysconfdir}/ssmtp/ should be owned, add in %files
%dir %{_sysconfdir}/ssmtp/

* add the release in the changelog entry

* [ %{buildroot} != "/" ] is useless

* missing 
Requires(post): %{_sbindir}/alternatives
BuildRequires: openssl-devel

* the ssmt.stuff.diff seems to be the debian patch for unstable. It should be
named like the debian patch in that case, and a comment in the spec file could
be usefull. Also I think it is better to base fedora packages on debian unstable
for such cases, it seems it is what you did, but in that case I think the url
should point to the unstable.

* I personally think that System Environment/Daemons isn't cery right for ssmtp
as it isn't a daemon but only a client. For esmtp I used Applications/Internet,
but feel free to chose what you prefer.
Comment 2 manuel wolfshant 2006-04-09 21:05:46 EDT
Thank for the helpful comments, Patrice !
Yes, I will need a sponsor, but I've thought I could ask for one when the
package gets in a neater shape.


I did consider not including smtpdaemon in provides, but this is a tough
decision and I would definitely like to hear a second opinion. The problemn is
that there are packages in Base (such as mdadm) which claim that they require
smtpdaemon, although they make either direct use of /usr/sbin/sendmail or via
the mail command. In my case I was interested exactly by the fact that I do NOT
want/need to have a daemon listening on port 25. Same goes for MTA.

Without the alternatives --auto call, a default mta would not be restored if the
package is removed, this is why I think that the script should be kept.
The reasons I have placed ssmtp in /usr/sbin are that both sendmail and postfix
place the sendmail binary in /usr/sbin and so does the Debian provided ssmtp,
too. Since applications actually look after /usr/sbin/sendmail which is a
symlink (via alternatives mta) to the real binaries, I guess I could move ssmtp
to /usr/bin. However I kind of think that maintaining the placement as chosen by
the authors of the program is a good idea.


New spec and src.rpm are available at http://wdl.lug.ro/linux/ssmtp/ssmtp.spec
and http://wdl.lug.ro/linux/ssmtp/ssmtp-2.61-2.src.rpm , respectively. The old
spec has been renamed to ssmtp.v1.spec, in case anyone whishes to compare the
versions.
Major changes: (hopefully) fixed a bug in the pre/post scriptlets; as suggested,
dropped the double ssmtp in the files' name and the [ %{buildroot} != "/" ]
test; completely removed the generate_config_alt script (I have found a bug in
it and I intend to provide another script, given the time to write it; the
provied ssmtp.conf file only needs 3-4 lines to be edited so I hope it's not a
major burden); changed the group to Applications/Internet; renamed the patch;
added openssl/openssl-devel to requires/buildrequires. Other minor changes are
listed in the Changelog.
Comment 3 Patrice Dumas 2006-04-10 03:47:13 EDT
(In reply to comment #2)
> Thank for the helpful comments, Patrice !
> Yes, I will need a sponsor, but I've thought I could ask for one when the
> package gets in a neater shape.

In my opinion you should start to find one sooner than later... I think a
sponsor not only look at the shape of your package, but also your willing to
accept the fedora extra rules and to learn from others...

> I did consider not including smtpdaemon in provides, but this is a tough
> decision and I would definitely like to hear a second opinion. The problemn is
> that there are packages in Base (such as mdadm) which claim that they require
> smtpdaemon, although they make either direct use of /usr/sbin/sendmail or via
> the mail command. In my case I was interested exactly by the fact that I do NOT
> want/need to have a daemon listening on port 25. Same goes for MTA.

It seems to me a mdadm bug. smtpdaemon should be required in case there is a
need for a daemon listening on port 25. It seems very wrong to me to provide
smtpdaemon when it is untrue, the packages that requires a smtp daemon would fail.

> Without the alternatives --auto call, a default mta would not be restored if the
> package is removed, this is why I think that the script should be kept.

No, after alternatives --remove, if the current alternative was the alternative
removed, it is set to the auto. However if the current alternative wasn't the
auto nor the removed alternative, the alternative is changed to the auto, this
is very bad!

> The reasons I have placed ssmtp in /usr/sbin are that both sendmail and postfix
> place the sendmail binary in /usr/sbin and so does the Debian provided ssmtp,
> too. Since applications actually look after /usr/sbin/sendmail which is a
> symlink (via alternatives mta) to the real binaries, I guess I could move ssmtp
> to /usr/bin. However I kind of think that maintaining the placement as chosen by
> the authors of the program is a good idea.

Ok, I had a wrong loook at Makefile.in. I think it is a mistake, as ssmtp could
be used by users even if it isn't set up as a sendmail replacement. esmtp and
msmtp (which compares more to ssmtp than sendmail and postfix, because they
don't need to be run as root) are installed in bindir. To me it looks like an
upstream error. But feel free to do what you prefer.

> added openssl/openssl-devel to requires/buildrequires. Other minor changes are
> listed in the Changelog.

The Requires openssl isn't needed, it should be picked up automatically by rpm.

In the changelog there are typos on the version.

%dir %{_sysconfdir}/ssmtp/
would be better than 
%{_sysconfdir}/ssmtp/
because the latter means the directory and what it contains, but what it
contains is allready listed separately.
Comment 4 manuel wolfshant 2006-04-11 05:31:19 EDT
4th release available at http://wdl.lug.ro/linux/ssmtp/ssmtp.spec
http://wdl.lug.ro/linux/ssmtp/ssmtp-2.61-4.src.rpm

Changelog:
release 3:
- removed Requires: openssl
- removed Provides: smtpdaemon
- cleaning of %files
- correct typos in version numbers in changelog
- disabled "alternatives --auto mta" in  %postun, pending more tests

release 4:
- added a small hack to make it compile on RHEL 3


Since this is my first package and its shape gets cleaner, I am looking for a
sponsor.
Comment 5 Patrice Dumas 2006-04-11 05:49:35 EDT
> release 4:
> - added a small hack to make it compile on RHEL 3

This is quite ugly. Maybe you could use what is on 
http://fedoraproject.org/wiki/DistTag

Moreover the CPPFLAGS setting should be before %configure, enabling not to set
it explicitely for make.
Comment 6 Enrico Scholz 2006-04-11 07:53:19 EDT
* the ugly CPP flag check can be probably expressed simply as

  | BuildRequires: krb5-devel  # (perhaps implicated by openssl-devel in RHEL3)
  | ...
  | test -e /usr/include/krb5.h || CPPFLAGS='-I/usr/include/kerberos'


* the 'Provides: smtpdaemon' is right; afais, it is used by other
  packages to express a requirement on a sendmail compatible CLI
  tool. Interpreting it as something which listens on a TCP port does
  not make sense because the daemon might be running on another host
  which can not be controlled by the local package-manager

* sometimes ago, I packaged ssmtpd myself. In this package, the
  %install section was shorter:

  | %install 
  | rm -rf $RPM_BUILD_ROOT 
  | %makeinstall etcdir='$(sysconfdir)' GEN_CONFIG=/bin/true mandir="$RPM_BUILD_ROOT%_mandir/man8" 
  | install -p -m644 ssmtp.conf $RPM_BUILD_ROOT%_sysconfdir/ssmtp/ssmtp.conf 

  I does not include your alternatives symlinks but is nevertheless a
  little bit shorter.

* not a blocker on Fedora, but using '/sbin/update-alternatives'
  instead of '/sbin/alternatives' is more portable across the various
  Linux distributions.

* I would not Provide: the man-pages; it is error-prone (see your
  'man5/ssmtp.conf.5' entry) and you can not control how 'rpmbuild'
  packages them finally (perhaps bz2-packed or plain instead of
  gzip-packed)

* I think, a real name would help significantly in getting a sponsor...
Comment 7 Paul Howarth 2006-04-11 08:00:54 EDT
For CPPFLAGS, why not just have:

CPPFLAGS=$(pkg-config --cflags-only-I openssl)
Comment 8 manuel wolfshant 2006-04-11 08:49:27 EDT
Enrico: sendmail in Core has the man pages listed in Provides, this I why I have
included them. Since Mandrake already has a ssmtp package and I only care about
RH/FC, I have not modified the alternatives require.

Paul: I love your idea, but this would add an additional Buildrequire for
pkgconfig. So the question is: which way to go: a)simple and fast as suggested
by Enrico (test -f ), b) elegant by using pkgconfig or c) using %{dist} (once I
figure out how to use it, since %if ( 0%{dist} == 0rhel3 ) terminates the build ) ?
Comment 9 Patrice Dumas 2006-04-11 10:51:35 EDT
(In reply to comment #6)

> * the 'Provides: smtpdaemon' is right; afais, it is used by other
>   packages to express a requirement on a sendmail compatible CLI
>   tool. Interpreting it as something which listens on a TCP port does
>   not make sense because the daemon might be running on another host
>   which can not be controlled by the local package-manager

It is not exactly what I concluded after that:

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=66396
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=165957

There seems to be some disension and confusion on that matter. I 
don't have a firm opinion on that, but the fetchmail case seems to 
indicate that smtpdaemon is used to indicate a smtp daemon running
locally on port 25. I agree that it doesn't make that much sense 
because having such a daemon installed doesn't mean that it is 
actually started, nor that it has not been tuned to ignore messages 
for some users, or listen on another port (or not listen at all...). 
If there isn't such package, however, it is certain that there is 
no local smtp daemon...

>   | %install 
>   | rm -rf $RPM_BUILD_ROOT 
>   | %makeinstall etcdir='$(sysconfdir)' GEN_CONFIG=/bin/true
mandir="$RPM_BUILD_ROOT%_mandir/man8" 
>   | install -p -m644 ssmtp.conf $RPM_BUILD_ROOT%_sysconfdir/ssmtp/ssmtp.conf 

This should work. A longuer version with an explicit overwritting 
of used variables on a 
make install
line (instead on %makeinstall) would be even more suitable, in my 
opinion, given that the Makefile.in is done by hand. But 
%makeinstall and specific overriding seems fine to me too.
Something along (haven't tested)

make install bindir=%{buildroot}%{_sbindir} \
  etcdir=%{buildroot}%{_sysconfdir} GEN_CONFIG=/bin/true
     mandir="%{buildroot}%_mandir/man8"

>   I does not include your alternatives symlinks but is nevertheless a
>   little bit shorter.

There are also some more manpages, and the following lines are still usefull, do
it must be longer anyway :-/:

mkdir  %{buildroot}%{_bindir}/
install -D -m 644 debian/mailq.8 %{buildroot}%{_mandir}/man1/mailq.ssmtp.1
install -m 644 debian/newaliases.8 %{buildroot}%{_mandir}/man1/newaliases.ssmtp.1
install -D -m 644 ssmtp.conf.5 %{buildroot}%{_mandir}/man5/ssmtp.conf.5
ln -s %{_sbindir}/ssmtp %{buildroot}%{_sbindir}/sendmail.ssmtp
ln -s %{_sbindir}/ssmtp %{buildroot}%{_bindir}/newaliases.ssmtp
ln -s %{_sbindir}/ssmtp %{buildroot}%{_bindir}/mailq.ssmtp

Comment 10 Paul Howarth 2006-04-11 10:55:16 EDT
(In reply to comment #8)
> Paul: I love your idea, but this would add an additional Buildrequire for
> pkgconfig.

So it would. I thought openssl-devel might have a dep on pkgconfig, but it
doesn't. Ah well.

> So the question is: which way to go: a)simple and fast as suggested
> by Enrico (test -f ), b) elegant by using pkgconfig or c) using %{dist} (once I
> figure out how to use it, since %if ( 0%{dist} == 0rhel3 ) terminates the
build ) ?

Try:
if "%{?dist}" == ".el3"
Comment 11 manuel wolfshant 2006-04-11 17:26:45 EDT
Thank you all for ideas. I have uploaded a new version, based on %{dist}. It can
be found at:
Spec file : http://wdl.lug.ro/linux/ssmtp/ssmtp.spec
SRPM : http://wdl.lug.ro/linux/ssmtp/ssmtp-2.61-5.src.rpm
Following Enrico's argument (actually that was my initial reasoning, too...) I
have added back the Provides: smtpdaemon.
 

%Changelog
* Tue Apr 11 2006 lonely wolfy <wolfy@pcnet.ro> 2.61-5
- cleaner hack for RHEL 3
- added back Provides: smtpdaemon
- correct typo in Provides
Comment 12 Patrice Dumas 2006-04-13 06:08:48 EDT
(In reply to comment #8)
> Enrico: sendmail in Core has the man pages listed in Provides, this I why I have
> included them. 

I don't think it is a good reason to include them. postfix and exim
don't do that and they are certainly right, I would even say that it
is a bug in sendmail package. No package should be requiring the man
page, and it is confusing at best.
Comment 13 manuel wolfshant 2006-04-22 09:18:24 EDT
sorry for the previous modification, first time I am using the depends on /
blocks part of buzilla. I am just trying to add the FE-NEEDSPONSOR field...
Comment 14 Michael J Knox 2006-08-01 20:10:28 EDT
Ok, I will review this one also. 
Comment 15 Michael J Knox 2006-08-20 20:09:29 EDT
Hey, just a quick ping to let you know that I am alive, just still in the throws
of unpacking/new job/etc etc. I hope to tidy this review up before/by the end of
the week. Thanks for your patience. 
Comment 16 Michael J Knox 2006-09-16 16:25:52 EDT
Sorry. Due to my stepping out for a while, I am unable to complete this review. 
Comment 17 Stewart Adam 2006-09-21 19:32:15 EDT
I'm not a review but I'm waiting on the sponsor of my package so I'll help...

Just so you know your SRPM link gives a 404, it seemed to be moved into a "rpms"
folder.

I ran rpmlint on your SRPM, and it gave only warnings, no errors, which is good
but if you'd like to clear them up anyways, here it is:

W: ssmtp macro-in-%changelog files
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

W: ssmtp macro-in-%changelog postun
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

W: ssmtp macro-in-%changelog buildroot
Macros are expanded in %changelog too, which can in unfortunate cases lead
to the package not building at all, or other subtle unexpected conditions that
affect the build.  Even when that doesn't happen, the expansion results in
possibly "rewriting history" on subsequent package revisions and generally
odd entries eg. in source rpms, which is rarely wanted.  Avoid use of macros
in %changelog altogether, or use two '%'s to escape them, like '%%foo'.

W: ssmtp mixed-use-of-spaces-and-tabs
The specfile mixes use of spaces and tabs for indentation, which is a
cosmetic annoyance.  Use either spaces or tabs for indentation, not both.
Comment 18 manuel wolfshant 2006-09-21 21:38:06 EDT
Thank you for the review. I am sorry for the annoyance due to reorganizing the
web site.

New spec and src.rpm are available at http://wdl.lug.ro/linux/rpms/ssmtp/ssmtp.spec
and
http://wdl.lug.ro/linux/rpms/ssmtp/ssmtp-2.61-6.src.rpm
respectively
Comment 19 Stewart Adam 2006-09-21 22:10:54 EDT
No problem - rpmlint now's clean, I looked over the spec and it seems valid.
Clean by as far as I can tell!
Comment 20 Patrice Dumas 2006-10-07 11:58:33 EDT
I have some comments:

* it is still wrong to provide the man pages, especially for 
  man pages corresponding with commands that do nothing in ssmtp

* it seems very dubious to me to
Provides:  %{_bindir}/mailq %{_bindir}/newaliases
  since these do nothing with ssmtp

* the patch ssmtp-2.61.6.patch seems to be a debian patch. I 
  think it should be marked as such, the file name would better
  be the one in debian, and it would also be nice to 
  have a comment telling what vulnerabilities are fixed.
  (I guess it is even possible to have an url in %Patch if you
  like).
  And it doesn't seems to be the latest version in debian
  unstable. Is there a reason?

* you could add -p switch to install when the file is from upstream
  since it keeps the timestamp.

Comment 21 manuel wolfshant 2006-10-09 04:48:22 EDT
Thank you for the new review.

As suggested, I have removed the "provides" for the man pages and stubs which do
nothing. I have also added the "-p" switch, but a quick look shows no
differences in the behaviour... Maybe because %doc already preserves the
timestamps and man pages are gzipped before being packaged. Nothing else is
preserved from upstream.

The fact that the included patches were retrieved from Debian and Mandrake
respectively is mentioned in the very first entry of the Changelog. I have
decided to rename the patches in order to maintain the more-or-less standard
policy of patch names used in RH. The included Debian patch is still at revision
6 because
- major change in revision 7 is IPv6; the others are just Debian related.
Unfortunately I have no IPv6 support around and cannot test
- major change in rev 8 is the switch from openssl to gnutls. For the time being
I cannot afford to test this either because all of the machines I run ssmtp on
are production machines.
Not to mention that the first listed change in rev7 is "ssmtp maintained via
alioth: http://alioth.debian.org/projects/ssmtp/" but the link says "This
Project Has Not Released Any Files" :) 
I will again into the SSL differences some time later, probably next month.



New versions of the spec file and SRPMS are available at
http://wdl.lug.ro/linux/rpms/ssmtp/.
Comment 22 Patrice Dumas 2006-10-09 05:06:44 EDT
(In reply to comment #21)
> Thank you for the new review.
> 
> As suggested, I have removed the "provides" for the man pages and stubs which do
> nothing. 

The provides for %{_mandir}/man5/ssmtp.conf.5.gz is still there,
although this file is allready listed as a ssmtp file, and besides 
such a provides is of no use.

> I have also added the "-p" switch, but a quick look shows no
> differences in the behaviour... Maybe because %doc already preserves the
> timestamps and man pages are gzipped before being packaged. Nothing else is
> preserved from upstream.

I was too lazy to check exactly but it may be relevant for 
other packages...

> The fact that the included patches were retrieved from Debian and Mandrake
> respectively is mentioned in the very first entry of the Changelog. I have
> decided to rename the patches in order to maintain the more-or-less standard
> policy of patch names used in RH. 

Indeed, I didn't remarked it... The mandrake patch is very simple
so no issue. But in may opinion it would be better (though not a 
blocker) to have, in comment near the Patch:, the full url to the debian 
patch. 

> The included Debian patch is still at revision
> 6 because
> - major change in revision 7 is IPv6; the others are just Debian related.
> Unfortunately I have no IPv6 support around and cannot test
> - major change in rev 8 is the switch from openssl to gnutls. For the time being
> I cannot afford to test this either because all of the machines I run ssmtp on
> are production machines.
> Not to mention that the first listed change in rev7 is "ssmtp maintained via
> alioth: http://alioth.debian.org/projects/ssmtp/" but the link says "This
> Project Has Not Released Any Files" :) 
> I will again into the SSL differences some time later, probably next month.

From a quick look at the latest debian patch, it seems ot me that 
the switch to gnutls hasn't been done very cleanly...

If there is no security related changes (as it seems to be the case)
it seems perfectly right to me not to use the latest patch. However
it also seems very clear to me that the debian patchset is the new 
upstream for the otherwise dead ssmtp package, so updating the package 
really means using the latest debian patches.

In my opinion, still, updating to the patchset 7 could be relevant, even 
if you cannot test ipv6, others could. I wouldn't personnaly make
that a blocker.
Comment 23 Patrice Dumas 2006-10-09 05:09:04 EDT
Also you should use /usr/sbin/alternatives or %{_sbindir}/alternatives
consistently and there is a missing Requires(preun) for alternatives.
Comment 24 Patrice Dumas 2006-10-09 05:22:03 EDT
I am a sponsor now. Looking at my first comment on the bug, it
seems like I allready noticed that the patch is a debian patch, 
but forgot later... Notice that I consistently ask for
this patch to be named like the debian one, however ;-).
This is still not a blocker.

As for being sponsored, you should have a look at
http://fedoraproject.org/wiki/Extras/HowToGetSponsored
in case you haven't allready.
Comment 25 manuel wolfshant 2006-10-09 08:01:02 EDT
I was living with the [wrong] impression that sendmail provides the main config
man page. Removed.
I have also fixed the unconsistent usage of %_sbindir and enabled IPv6. No need
for any patches, it is just a configure option. This should be more or less
equal with Debian's patch level 7.
At the first glance (therefore I might be wrong) Debian's v8 patch includes
-	LIBS="$LIBS -lssl"
+	LIBS="$LIBS /usr/lib/libgnutls-openssl.so"
which I think that breaks x86_64 compatibility. The same lines are included in
the revision 9 of the patch. As I have said, I am not going to include this
patch until I examine it closer. I will take care of that some time later, for
the moment I focus on cleaning another package, in order to submit it.

I was not able to find an URL for the version of patch I use, the only ones
still available seem to be revisions 2, 8 and 9. I have included a commented
line in the spec which points to the current Debian patch.
As I have said, I prefer to have the patches named similar to the other patches
used by the RH packaging system (where the filenames end in "patch" rather then
"diff").
Comment 26 Patrice Dumas 2006-10-09 09:35:33 EDT
(In reply to comment #25)

> At the first glance (therefore I might be wrong) Debian's v8 patch includes
> -	LIBS="$LIBS -lssl"
> +	LIBS="$LIBS /usr/lib/libgnutls-openssl.so"
> which I think that breaks x86_64 compatibility. 

That's exactly what I was referring to when saying that the switch
to gnutls wasn't done cleanly.

> I was not able to find an URL for the version of patch I use, the only ones
> still available seem to be revisions 2, 8 and 9. I have included a commented
> line in the spec which points to the current Debian patch.

Ok.

> As I have said, I prefer to have the patches named similar to the other patches
> used by the RH packaging system (where the filenames end in "patch" rather then
> "diff").

Patches may perfectly be named with .diff. But I have no problem
if you prefer .patch.

You should post an url to the updated src.rpm when you have changed it.
I knew where to search but it is much less convenient, and a reviewer
coming at the end of the review wouldn't find it.

I still see one issue, the %{version} in Patch0 will lead to something
wrong if it changes. It seems more prudent to hardcode the version 
instead. Not a blocker.

And also some compiler warning may seem worrisome, stil not a blocker in
my opinion.

Here is the formal review:

* rpmlint output is ignorable (right symlinks, virtual provides, 
  alternative provide):

W: ssmtp unversioned-explicit-provides MTA
W: ssmtp unversioned-explicit-provides smtpdaemon
W: ssmtp unversioned-explicit-provides %{_sbindir}/sendmail
W: ssmtp symlink-should-be-relative /usr/sbin/sendmail.ssmtp /usr/sbin/ssmtp
W: ssmtp symlink-should-be-relative /usr/bin/newaliases.ssmtp /usr/sbin/ssmtp
W: ssmtp symlink-should-be-relative /usr/bin/mailq.ssmtp /usr/sbin/ssmtp

* follows naming and packaging guidelines
* free software, licence included
* spec legible
* source match upstream:
957e6fff08625fe34f4fc33d0925bbc9  ../SOURCES/ssmtp_2.61.orig.tar.gz
* %files section right
* use almost latest version, as the latest version corresponds with 
  a debian patchset which may not be straightforward to include
* sane provides:
Provides: /usr/sbin/sendmail MTA config(ssmtp) = 2.61-8 smtpdaemon
  There is a controversy about smtpdaemon, so it is right to leave
  it to the packager.


This is potentially approved once you are sponsored. You
seem to be ready to follow the guidelines, you have the required
skills. The most important thing is to be sure that you'll 
remain interested in maintaining your packages in fedora.
I guess this will be the case since you didn't abandoned
the submission after 5 months... I am almost ready to sponsor
you.

To be sure I would prefer if you could show even more interest
in fedora extras by doing things advertized there:
http://fedoraproject.org/wiki/Extras/HowToGetSponsored
Comment 27 manuel wolfshant 2006-10-09 10:33:43 EDT
The most recent ssmtp.spec is / will be vailable (for as long as possible) at
http://wdl.lug.ro/linux/rpms/ssmtp/ssmtp.conf
The src.rpm is available at http://wdl.lug.ro/linux/rpms/ssmtp/ssmtp-2.61-8.src.rpm
Previous versions of the spec and src.rpm files, as well as some precompiled
binaries are available under http://wdl.lug.ro/linux/rpms/ssmtp. All of the
precompiled binaries were in use in my network at the time they were uploaded.

I intend to maintain the package for the simple reason that I actively use it.
Keeping the spec file clean is a small burden first but makes live easier
afterwards.
Comment 28 Patrice Dumas 2006-10-09 10:42:44 EDT
> I intend to maintain the package for the simple reason that I actively use it.
> Keeping the spec file clean is a small burden first but makes live easier
> afterwards.

Being a fedora maintainer is a bit more than maintaining some 
packages. It also means reviewing others packages, fixing bugs,
keeping specs up-to-date with changes in guidelines,
rebuilding for new releases, compiler changes or dependency changes,
watching others commits, discussing about fedora extras future, 
guidelines, and organization. Of course you don't have to be
involved that much when you've just become a maintainer, but I
hope you get the idea that it is being part of a developpers 
community, not strictly packaging.
Comment 29 Kevin Fenzi 2006-10-20 12:27:12 EDT
Note to Patrice: 

Submitter has another package submitted (ren) and was going to be sponsored, 
but the sponsor had to pull back due to time commitments. 
You may want to take a look at: 
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=196434
Comment 30 Patrice Dumas 2006-10-24 05:12:07 EDT
I am ready to sponsor you, Manuel. You should proceed as explained
here:

http://fedoraproject.org/wiki/Extras/Contributors

http://fedoraproject.org/wiki/Extras/Contributors#head-a89c07b5b8abe7748b6b39f0f89768d595234907
Comment 31 manuel wolfshant 2006-10-30 10:41:48 EST
In order to complete the submission of this package, I have just taken a deeper
look at the patches included by debian/unstable since my first submission ssmtp
package and a few questions have arisen.

Patch number 8 switches from using openssl to gnutls, the reason behind the
change being pointed in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=374327.
Quoting from there: 
"as written in debian/copyright ssmtp is now GPL. Your package is linked
against OpenSSL, which is not licensed under the GPL, this is a license
violation. Upstream should add an exception to the license (in COPYING)
which allows the linking with OpenSSL, this you should also write in
debian/copyright. The second choice would be to port ssmtp to gnutls which
is GPL."
Now the facts:
- ssmtp is GPL (the original tar.gz includes the file named COPYING which is the
GPLv2 license)
- openssl is BSDish (quote from pm -q --qf "%{license}\n" openssl) 
- gnutls is LGPL
I am far from being a legal expert (au contraire actually :) ) but I have just
read the LICENSE file included in the openssl rpm package and I am not convinced
at all about the need for the change. So. my question is: leaving aside the
obvious "better to be in sync with upstream", is this change (openssl -> gnutls)
really needed for FE ? Especially since ssmtp does NOT modify in any way the
openssl toolkit, it just links against the libraries ?
Comment 32 Patrice Dumas 2006-10-30 10:56:53 EST
(In reply to comment #31)

> Patch number 8 switches from using openssl to gnutls, the reason behind the
> change being pointed in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=374327.
> Quoting from there: 
> "as written in debian/copyright ssmtp is now GPL. Your package is linked
> against OpenSSL, which is not licensed under the GPL, this is a license
> violation. 

I'm a bit surprised that debian folks say such strange things. The GPL
cause any derivated work to be licenced under the GPL, but linking 
dynamically against a library doesn't make that library a derivative
work. If this was true, GPL programs couldn't be linked against 
proprietary libc on unix or windows.

In fact the opposite could be argued, linking against a library
forces to comply with the library licence, that's what leads to the
LGPL for libraries versus GPL, whenever one want to allow the library 
to be linked against GPL incompatible code.
Comment 33 manuel wolfshant 2006-10-30 11:31:31 EST
That is my point EXACTLY. But since my experience is very limited, I'd better
ask twice than bumping my head :)

Here is a short analysis of the up-to-date patches from debian unstable (the way
I see them; please bear with me if I am wrong and point me in the right direction):
- patch number 7 modifies some debian email addresses and activates IPv6 by
default; we do not really care about the first issue, second modification was
included in my spec file released on 2006-10-09;
- patch number 8 raises the ssl issue I would like clarifications for;
- patch number 9 a) activates cram-md5; this was already included in my spec
released 2006-04-09 b) fixes a portability issue introduced by the usage of a
glibc extension. Since the bug seems to occur on platforms non-supported by FE,
I contemplate the idea of including this patch but with a conditional (something
like rpmbuild --define "non-glibc 1" and a default value of 0 for this
variable). However, I assume this should not be a blocker for a FE package.
- all patches (7 to 10) add / correct some debconf translations.

Given the above, could you please let me know what modifications are still
needed,in order to get in shape for inclusion in FE ?
Comment 34 Patrice Dumas 2006-11-12 15:05:32 EST
(In reply to comment #33)

> Given the above, could you please let me know what modifications are still
> needed,in order to get in shape for inclusion in FE ?

none, it is APPROVED. Look like those patches are not needed on FE, if you
really want an advice on a specific issue, ask, otherwise I trust your
choices.

Sorry, I completely forgot to respond here :/
Comment 35 manuel wolfshant 2006-11-12 18:37:32 EST
Thank you, Patrice. Succesfully built the devel branch, submitted branch
requests for FC5 and FC6.
Will close this bug as soon as those branches are built, too.
Comment 36 manuel wolfshant 2006-11-13 17:34:25 EST
FC-5 and FC-6 sucessfully built. Closing.
Comment 37 manuel wolfshant 2007-03-05 08:10:20 EST
EPEL-4 ssmtp wolfy@nobugconsulting.ro
Comment 38 manuel wolfshant 2007-03-24 23:24:52 EDT
New Package CVS Request
=======================
Package Name: ssmtp
Short Description: Extremely simple MTA to get mail off the system to a Mailhub
Owners: wolfy@nobugconsulting.ro
Branches: EPEL-5
Comment 39 Dennis Gilmore 2007-03-25 13:34:06 EDT
branched for EL-5
Comment 40 manuel wolfshant 2014-10-29 12:11:36 EDT
Package Change Request
======================
Package Name: ssmtp
New Branches:  epel7
Owners: wolfy
Comment 41 Gwyn Ciesla 2014-10-29 12:28:08 EDT
Git done (by process-git-requests).

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