Hi Franck
On Sun, Mar 08, 2009 at 12:28:10PM +0100, Franck Joncourt wrote:
> [...]
> >> I do not know how to set the sender mail address when I use the mail
> >> command. Currently debarchiver fails to set it.
> >
> > Doesn't this work?
> > $mailcmd -s '$package $key' '$to' -- -f '$mailfrom'
> >
> > It was in the code before you changed it. That is why I have this check
> > about from address.
>
> No, I am not able to make it work either with the current version of
> debarchiver or from the command line. I get an email whose to header is
> malformed and I also get an error from the mailer-daemon.
Ok, I see. If that is the case then the code should be removed. Good to
spot that.
> [...]
> > Now to the comments. The comments are below each patch section.
> >
> > diff --git a/src/debarchiver.pl b/src/debarchiver.pl
> > index 2eaa9bd..ba6ef0e 100755
> > --- a/src/debarchiver.pl
> > +++ b/src/debarchiver.pl
> > @@ -72,9 +72,13 @@ use OpaL::read qw(readfile readcommand);
> > # 2007-10-09 Ola Lundqvist <o...@inguza.com>
> > # Changed force option to ignoredestcheck option.
> >
> > +my $use_sendmail = 0;
> > +my %cmds = ();
> > +$cmds{'sendmail'} = 'sendmail';
> > +$cmds{'mail'} = 'mail';
> > +
> > $copycmd = "cp -af";
> > $rmcmd = "rm -f";
> > -$mailcmd = "mail";
> > $movecmd = "mv";
> > $vrfycmd = "dscverify";
> > $cachedir = "/var/cache/debarchiver";
> >
> > I think we should rename this %cmds hash to %mailcmds. But also see
> > the comments below. I assume order is not defined, right?
>
> As a matter of fact, maybe other system commands such as dscverify,
> mv... could be added to the hash.
True.
> Then, by a simple call to check_commands, all of them could be checked
> and an error could be raise if there is a problem.
True. However the $mailcmd et al can be set from the config file so it
must be made backwards compatible. :-)
> The hash does not assume order.
>
> It works this way:
>
> cmds{$mycmd} = $mycmdwithfullpath
>
> if $mycmdwithfullpath is not a full path but only the command name, the
> check_commands function will try to find it in a set of common directories.
Ok, I see.
> > @@ -456,9 +460,19 @@ while ($_ = shift @ARGS2) {
> > elsif (/^--movecmd$/) {
> > $movecmd = shift @ARGS2;
> > }
> > - elsif (/^--mailcmd$/) {
> > - $mailcmd = shift @ARGS2;
> > - }
> > + elsif (/^--mailcmd$/) {
> > + my $usercmd = shift @ARGS2;
> > + if ($usercmd =~ m|^(.*\/)+(.*)|) {
> > + $cmds{$2} = $usercmd;
> > + $usercmd = $2;
> > +
> > + } else {
> > + $cmds{$usercmd} = $usercmd;
> > + }
> > + &check_commands({$usercmd => ''}, {});
> > + $cmds{'mail'} = $cmds{$usercmd};
> > + delete $cmds{'sendmail'};
> > + }
> > elsif (/^--mailfrom$/) {
> > $mailfrom = shift @ARGS2;
> > }
> >
> > The second question is about this. What does it do exactly.
> > If I would write
> > --mailcmd /usr/local/bin/special_mailcommand_opal
> >
> > Wouldn't that give quite strange results?
>
> If I have not made any mistake, it should only try to find
> special_mailcommand_opal in /usr/local/bin/. If it does not exist, it
> will try to find it in the set of common directories. If still no luck,
> debarchiver should raise an error and stop.
But shouldn't the $cmds{'mail'} be deleted as well then?
> Otherwise:
> * the cmds{'mail'} will be overwritten by
> /usr/local/bin/special_mailcommand_opal.
> * the sendmail reference will be removed from the hash
> => debarchiver will send emails using the same format as the mail comand
> for the special_mailcommand_opal command.
>
> > I think it may be better to have a configuration option that
> > tell whether it is sendmail format for the mail output or if it
> > is the mail format. The automatic detect code is good and can be
> > used to set the default.
>
> That sounds better. Something like --mailformat (sendmail|mail) with a
> default behavior ?
That would be a very good thing. I suggest sendmail will be the default
behaviour, but only if sendmail can not be found in the system.
> [...]
> > +
> > + } else {
> > +
> > + if (open(M, "| $cmds{'mail'} -s '$subject' '$toAddress'")) {
> > + print M $msg . "\n";
> > + close(M);
> > +
> > + } else {
> > + pdebug(2, "Could not execute $cmds{'mail'} $!");
> > + }
> > +
> > + }
> > +
> > + pdebug(5, "Mail exec done.");
> > }
> >
> >
> > ###############################################################################
> >
> >
> > Here you missed the set of from address... Apart from that it looks good.
>
> As mentionned above I have not been able to make it work :)
Ok.
> [...]
> > +sub check_commands() {
> > + my ($include_hr, $exclude_hr) = @_;
> > +
> > + my @path = qw(
> > + /bin
> > + /sbin
> > + /usr/bin
> > + /usr/sbin
> > + /usr/local/bin
> > + /usr/local/sbin
> > + );
> > +
> > + for my $cmd (keys %cmds) {
> >
> > Ahh ok here is the reason why this is a hash instead of variables. I see.
> > However this way order is not guaranteed... or?
>
> Yes, no order guaranteed.
>
> > +
> > + if (keys %$include_hr) {
> > + next unless defined $include_hr->{$cmd};
> > + }
> > + if (keys %$exclude_hr) {
> > + next if defined $exclude_hr->{$cmd};
> > + }
> > +
> > + unless (-x $cmds{$cmd}) {
> > +
> > + my $found = 0;
> > + pdebug(3, "$cmd not located/executable at $cmds{$cmd}\n");
> >
> > This almost always give a warning. Is that intentional?
>
> Should be an info.
>
> > + PATH: for my $dir (@path) {
> > + if (-x "${dir}/${cmd}") {
> > + $cmds{$cmd} = "${dir}/${cmd}";
> > + $found = 1;
> > + last PATH;
> > + }
> > + }
> > +
> > + if ($found) {
> > + pdebug(3,"Found $cmd at $cmds{$cmd}\n");
> >
> > I think it should be level 4 (info) instead of 3 (warning), right?
>
> You are right.
>
> [...]
Best regards,
// Ola
> Regards,
>
> --
> Franck Joncourt
> http://debian.org - http://smhteam.info/wiki/
>
--
--- Inguza Technology AB --- MSc in Information Technology ----
/ o...@inguza.com Annebergsslingan 37 \
| o...@debian.org 654 65 KARLSTAD |
| http://inguza.com/ Mobile: +46 (0)70-332 1551 |
\ gpg/f.p.: 7090 A92B 18FE 7994 0C36 4FE4 18A1 B1CF 0FE5 3DD9 /
---------------------------------------------------------------
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
