I wonder if this shouldn't have an OS check before sending it.  

I'd suggest something like this pseudo code in a central place so it is called only once.

$delimiter = "\n";

if (is_windows) {
  $delimiter = "\n\r";
}

Then change the print statements to something like:

my($nwrite) = syswrite(STDLOG, sprintf("%s [%s] %s: %s%s", scalar localtime, $$, $level, $msg, $delimiter));

Created attachment 4786 [details]
using utils, replaced static delimiter

Hmm, when you say central place, do you mean a per file or is it possible to define this in a more central component?

Anyway, slightly reformatted patch (using Util qw(am_running_on_windows) for check and replaced static delimiter).

Daniel

(In reply to comment #2)
> Created an attachment (id=4786) [details]
> using utils, replaced static delimiter
> 
> Hmm, when you say central place, do you mean a per file or is it possible to
> define this in a more central component?

I meant doing something like what you did where this delimiter concept can be reused.  I can't recommend a better way than what you did though of course, it likely exists ;-)
 
> Anyway, slightly reformatted patch (using Util qw(am_running_on_windows) for
> check and replaced static delimiter).

I think I remember from a previous patch that using perl vars and substitutions is bad form.

I think you need to change your printf's etc. so that they use substitution to be consistent with the existing code.

printf STDERR ("%s [%d] %s: %s%s", $timestamp, $$, $level, $msg, $delimiter)  or warn "Error writing to log file: $!";

Regards,
KAM

Hmm, I don't really like the thought of defining a $delimiter variable...

Would using $/ instead of \n not already solve the problem without us adding special delimiter code? Yeah, $/ actually is the input record separator, but it is set according to the OS, unlike its output equivalent $\ (which defaults to undef, and generally effects print statements).

See perlvar and perlport, section Newlines.

Using PerlIO :crlf on the output filehandle should also solve this, requiring code adjustments in a different place.

See PerlIO.

In both cases I'm not sure though, if there is any negative effect on Mac OS systems, which uses CR rather than LF (Unix) or CRLF (Windows). Is that even still true since Mac OS X?

(In reply to comment #4)
> In both cases I'm not sure though, if there is any negative effect on Mac OS
> systems, which uses CR rather than LF (Unix) or CRLF (Windows). Is that even
> still true since Mac OS X?

Sorry, should have spent another minute before commenting.

Mac OS up to version 9 uses CR. Mac OS X of course uses LF, like any Unix-like system, such as Linux and *BSD.

Created attachment 5002 [details]
Revised patch to use sprintf with variable placeholder for delimiter

(In reply to comment #5)
> (In reply to comment #4)
> > In both cases I'm not sure though, if there is any negative effect on Mac OS
> > systems, which uses CR rather than LF (Unix) or CRLF (Windows). Is that even
> > still true since Mac OS X?
> 
> Sorry, should have spent another minute before commenting.
> 
> Mac OS up to version 9 uses CR. Mac OS X of course uses LF, like any Unix-like
> system, such as Linux and *BSD.

Using PerlIO :crlf over complicates things.  The delimiter code is only for logging and we just want to change logging to be a bit better.  

His code fixes that for windows in the most centralized way I can think of by changing the errors and log_message code in File.pm and Stderr.pm.

In most unix boxes, it'll use Stderr.pm and switch to Syslog.pm so this shouldn't be an efficiency issue.

If you really want to support OS9, Util.pm should have functionality added to it.  However, OSX is now over 10 years old.  OS9 support is not needed IMO.

Attached patch won't pass make test, though.  What am I doing wrong...?

(In reply to comment #6)
> Revised patch to use sprintf with variable placeholder for delimiter

Did you actually read comment 4? ;)

All that M::SA::Util::am_running_on_windows() code and explicitly setting your own custom $delimiter might not be necessary. Basically, replacing the 4 occurrences of \n with $/ should do the trick, and make Perl magically use the appropriate "newline" sequence.

Since $/ isn't a custom var, it should even be fine to use directly as a replacement for \n in the sprintf() string -- I'd be rather scared if it ever contains a %.

Created attachment 5003 [details]
Revised Carriage Return patch to use $/

(In reply to comment #7)
> (In reply to comment #6)
> > Revised patch to use sprintf with variable placeholder for delimiter
> 
> Did you actually read comment 4? ;)
> 
> All that M::SA::Util::am_running_on_windows() code and explicitly setting your
> own custom $delimiter might not be necessary. Basically, replacing the 4
> occurrences of \n with $/ should do the trick, and make Perl magically use the
> appropriate "newline" sequence.
> 
> Since $/ isn't a custom var, it should even be fine to use directly as a
> replacement for \n in the sprintf() string -- I'd be rather scared if it ever
> contains a %.

Sorry, I think I hyperfocused on why (am_running_on_windows()) wouldn't work.  

Attached is a compromise since I feel better sanitizing it to make sure it's ok.

This one passes make test!

(In reply to comment #8)
> Created attachment 5003 [details]
> Revised Carriage Return patch to use $/

  $delimiter =~ s/[\r\n]//g;

That's backwards, you wanted to weed out any *non* [\r\n] chars.

> Attached is a compromise since I feel better sanitizing it to make sure it's
> ok.

Sanitizing your variables always is a good idea, but still, I guess in this case really not necessary. If we (or anyone else) messes with the input record separator, it sure is done local()-ly. Otherwise, the code would break all over the place, and your STDERR logging is the least of your problems.

The input record separator $/ is what chomp() uses internally. Which we use in 95 places in 39 files -- without sanitizing it.

  $ find . -name '*.pm' | xargs grep -l chomp | wc -l
  39

Pending confirmation of $/ doing the expected on Windows systems (Daniel?), I'd prefer to drop that entire $delimiter dance, and simply use $/ instead of \n.


(As a side-note, the caveats in comment 4 and 5 about old Mac OS <= 9 appears to be a non-issue. \n is a logical newline, and evaluates appropriately even on old Mac OS systems. See perlport, section Newline.)

 
>   $delimiter =~ s/[\r\n]//g;
> 
> That's backwards, you wanted to weed out any *non* [\r\n] chars.

Thanks.  I meant $delimiter =~ s/[^\r\n]//g;

> Pending confirmation of $/ doing the expected on Windows systems (Daniel?), I'd
> prefer to drop that entire $delimiter dance, and simply use $/ instead of \n.

It's a fair argument but requires testing. 
 
> (As a side-note, the caveats in comment 4 and 5 about old Mac OS <= 9 appears
> to be a non-issue. \n is a logical newline, and evaluates appropriately even on
> old Mac OS systems. See perlport, section Newline.)

I was worried ;-)

Hmm, not really making a difference here using $/
The logfile still uses only a linefeed as separator.


Had a quick look to the perlport docs Kevin mentioned, wouldn't it be suitable using 

use Socket qw(:DEFAULT :crlf);

instead?

...

  my($nwrite) = syswrite(STDLOG, sprintf("%s[%s] %s: %s%s",
                                         $timestamp, $$, $level, $msg, $CRLF));

...

On my Windows it did the trick :)

Daniel

> Had a quick look to the perlport docs Kevin mentioned, wouldn't it be suitable
> using   use Socket qw(:DEFAULT :crlf)  instead?

No, that would always produce a \015\012, instead of a local line terminator.

About sanitizing $/, I agree with Karsten in Comment 9 that there is no
need to overdo it. If $/ is not a simple \n or \r\n, we are pretty much
lost in all other places. All we need to deal with is Windows and Unix.
If we end up on a a record-structured files (like VMS) or hypothetical
other weird systems, a simple wrestling with $/ won't work anyway.

As suggested, the PerlIO with :crlf would probably be the cleanest
solution, but buffering and locking on a log file with concurrent accesses
from child processes will get in the way, so it cannot be done lightly
without sufficient testing.

I'm inclined to the original simple approach using am_running_on_windows,
and having a "\n" for everybody else.

Not sure if using this custom line terminator is suitable for
die and warn calls too. If yes, there are hundreds of other calls
like these all over the place. Perl also has some special logic
regarding a \n in a die/warn argument text (to add line number or not).

A minor nit: a variable name $delimiter is not the best choice:
we are dealing with line terminators, not delimiters. And a shorter name
would be handy, as it may eventually end up in other places too.
Perhaps an: $eol .

Created attachment 5005 [details]
patch to make textfile logging in Windows use correct end of line separator

So anybody against the am_running_on_windows approach?

Otherwise, I propose the attached patch (just updated variable name from delimiter to eol as Mark noted). 

Daniel

(In reply to comment #13)
> Created attachment 5005 [details]
> patch to make textfile logging in Windows use correct end of line separator
> 
> So anybody against the am_running_on_windows approach?
> 
> Otherwise, I propose the attached patch (just updated variable name from
> delimiter to eol as Mark noted). 

Daniel,

I couldn't get the am_running_on_windows to make test.  Does it pass for you?

...
Beside the other thousand make tests that fail under Windows? ;)
...

No, having serious issues here with the am_running_on_windows as well:


Undefined subroutine &Mail::SpamAssassin::Logger::Stderr::am_running_on_windows
called at ../lib/Mail/SpamAssassin/Logger/Stderr.pm line 46.
Compilation failed in require at ../lib/Mail/SpamAssassin/Logger.pm line 72.
BEGIN failed--compilation aborted at ../lib/Mail/SpamAssassin/Logger.pm line 72.

Compilation failed in require at ../lib/Mail/SpamAssassin/Util.pm line 48.
BEGIN failed--compilation aborted at ../lib/Mail/SpamAssassin/Util.pm line 48.
Compilation failed in require at t/util_wrap.t line 25.
Failed 18/165 test programs. 74/1811 subtests failed.
NMAKE : fatal error U1077: 'C:\Perl\bin\perl.exe' : return code '0xff'
Stop.


Not sure what's causing this. 
I’ve just tested the patch by calling spamd and having a look at the EOL separators of the generated log files.

Daniel

> ...
> Beside the other thousand make tests that fail under Windows? ;)
> ...

Besides that. :-)


> No, having serious issues here with the am_running_on_windows as well:
> 
> 
> Undefined subroutine &Mail::SpamAssassin::Logger::Stderr::am_running_on_windows
> called at ../lib/Mail/SpamAssassin/Logger/Stderr.pm line 46.
> Compilation failed in require at ../lib/Mail/SpamAssassin/Logger.pm line 72.
> BEGIN failed--compilation aborted at ../lib/Mail/SpamAssassin/Logger.pm line
> 72.
> 
> Compilation failed in require at ../lib/Mail/SpamAssassin/Util.pm line 48.
> BEGIN failed--compilation aborted at ../lib/Mail/SpamAssassin/Util.pm line 48.
> Compilation failed in require at t/util_wrap.t line 25.
> Failed 18/165 test programs. 74/1811 subtests failed.
> NMAKE : fatal error U1077: 'C:\Perl\bin\perl.exe' : return code '0xff'
> Stop.
> 
> 
> Not sure what's causing this. 

I don't know either but a perl -c on one of the two pm's showed it might be a circular reference issue perhaps?  But am_running_on_windows doesn't pass test at the moment so it's a no-go patch.  

I want to get this fixed but believe it can wait for 3.4.1.  So I'm targeting it for 3.4.1 unless you have ideas that are fairly quick.  Want to try and focus on a new release sooner rather than later.

Daniel, did you ever get the patch working?  Would be nice to apply it and close out this issue.

(In reply to Kevin A. McGrail from comment #17)
> Daniel, did you ever get the patch working?  Would be nice to apply it and
> close out this issue.

Hi,

my name is Martin. I am Daniel's successor at JAM Software regarding the SpamAssassin for Windows project. I would also like to close out this issue. :-)

We've been using the `am_running_on_windows` approach here. Running spamd and looking at the log file indicates that it is working.

> But am_running_on_windows doesn't pass test at the moment
> so it's a no-go patch.

Which approach do you prefer? `$/` or `am_running_on_windows`? I'll try both. Which tests should I be primarily looking at. Hundreds of them are failing on Windows anyway. So, I am a bit lost. :-/

Hi Martin, nice to meet you.

For make test, I am discussing when it's tested in a unix environment.  So it has to pass there because we don't officially release a windows version.

For me, if you have a patch working in a live environment AND we can get it to pass tests under unix, that's the target patch.

Regards,
KAM

`am_running_on_windows` works but it doesn't pass the tests. Like you said, it's a circular dependency issue:

M::S::Logger::Stderr uses M::S::Util uses M::S::Logger uses M::S::Logger::Stderr

We have two options:

1. Put am_running_on_windows into it's own module (M::S::Util::Win32 ?).
2. Duplicate the code in M::S::Logger::Stderr

IMHO, the first option is preferrable. If you agree, I will open an issue and submit a patch.

> `am_running_on_windows` works but it doesn't pass the tests. Like you said,
> it's a circular dependency issue:
> 
> M::S::Logger::Stderr uses M::S::Util uses M::S::Logger uses
> M::S::Logger::Stderr
> 
> We have two options:
> 
> 1. Put am_running_on_windows into it's own module (M::S::Util::Win32 ?).
> 2. Duplicate the code in M::S::Logger::Stderr
> 
> IMHO, the first option is preferrable. If you agree, I will open an issue
> and submit a patch.

I think creating a new module just for that purpose is an overkill,
especially since it is not necessarily just about Windows, but about
a choice of a line terminator. A more general Windows-specific module
would be warranted if more platform-specific code would need to be
moved there.

My preference would be to just duplicate the line from Util.pm
  use constant RUNNING_ON_WINDOWS => ($^O =~ /^(?:mswin|dos|os2)/oi);
into M::S::Logger::Stderr and use the constant - keep it simple -
possibly adding a comment explaining the choice.

I think the module is "better programming" but the copy of a single line makes sense and avoids a code fork for an OS.

Created attachment 5337 [details]
Patch, use OS-appropriate line terminator

Here is my patch. As suggested, I have copied `use constant RUNNING_ON_WINDOWS => ($^O =~ /^(?:mswin|dos|os2)/oi);` over to StdErr.pm. It passes `make test` on Fedora 22, and works as intended on Windows.

Passes tests for me as well.

Committed revision 1710611.
Committed revision 1710612.