Bug 691441 - how r/w -> rb/wb breaks regression?
Summary: how r/w -> rb/wb breaks regression?
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Graphics Library (show other bugs)
Version: master
Hardware: PC Linux
: P4 normal
Assignee: Chris Liddell (chrisl)
URL:
Keywords:
Depends on:
Blocks: 691520
  Show dependency tree
 
Reported: 2010-07-06 03:19 UTC by Hin-Tak Leung
Modified: 2012-07-20 13:52 UTC (History)
3 users (show)

See Also:
Customer:
Word Size: ---


Attachments
configure.ac and a couple of other files patch (2.55 KB, patch)
2010-07-21 08:55 UTC, Chris Liddell (chrisl)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Tak Leung 2010-07-06 03:19:50 UTC
Somebody probably should look at why "r", "w" -> "rb", "wb" (which should only affect systems which distinguish text/binary file modes, and should have no effect on unix systems) from the cygwin/mingw contributed batch for ghostscript breaks almost every regression test:

http://ghostscript.com/pipermail/gs-regression/2010-July/012702.html
Comment 1 Alex Cherepanov 2010-07-06 15:33:41 UTC
popen() doesn't like "wb" on Linux but accepts it on Windows.

I wonder what's a good way to fix this? Perhaps, this should be yet another
issue for autoconf.
Comment 2 Hin-Tak Leung 2010-07-07 02:02:29 UTC
(In reply to comment #1)
> popen() doesn't like "wb" on Linux but accepts it on Windows.
> 
> I wonder what's a good way to fix this? Perhaps, this should be yet another
> issue for autoconf.

Don't know what's the best way, but any solution probably involves having some conditionals of the form 

#if TEXT_MODE_DIFF_BIN_MODE
...
#else
...
#endif

somewhere, since the 'b' is required on some platforms but problematic on others.
Comment 3 Chris Liddell (chrisl) 2010-07-21 08:55:34 UTC
Created attachment 6519 [details]
configure.ac and a couple of other files patch

Add a test for the configure script to discover the current platform support the "b" option for popen(), and use a preprocessor directive to apply the result when required.

Posting here for Hin-Tak and/or Alex to cast eye over the proposed changes, any problems/objections, please shout, if I hear nothing, I'll assume we're all happy.
Comment 4 Alex Cherepanov 2010-07-22 02:50:55 UTC
The only thing missing is a corresponding change for MSVC and other
non-autoconf platforms.
Comment 5 SaGS 2010-07-22 03:58:16 UTC
A typo:    #define GP_FMODE_WB "b"
should be  #define GP_FMODE_WB "w"
Comment 6 Chris Liddell (chrisl) 2010-07-22 16:32:28 UTC
Oops, thanks for the correct SaGS!


Alex, Can't we just rely on the default fallback I put in gp.h for platforms that a) don't use configure, and/or b) don't care about the "r"/"rb" difference?

I ask because there are several (OS/2, vms etc) that I would have no way of testing changes to the make files.
Comment 7 Hin-Tak Leung 2010-08-11 20:20:59 UTC
(In reply to comment #6)
> Alex, Can't we just rely on the default fallback I put in gp.h for platforms
> that a) don't use configure, and/or b) don't care about the "r"/"rb"
> difference?

mingw doesn't use configure, but still care r/rb . So probably something like
    if defined(__CYGWIN__) || defined(__MINGW32__)
is still needed.
Comment 8 Chris Liddell (chrisl) 2010-08-12 14:05:25 UTC
(In reply to comment #7)
> (In reply to comment #6)
> 
> mingw doesn't use configure, but still care r/rb . So probably something like
>     if defined(__CYGWIN__) || defined(__MINGW32__)
> is still needed.

Is there a (good) reason for that? Other packages for it seem to rely on a configure script, so it seems reasonable for us to do so, and avoid adding platform specific preprocessor macros in the source.
Comment 9 Hin-Tak Leung 2010-08-12 14:50:21 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > 
> > mingw doesn't use configure, but still care r/rb . So probably something like
> >     if defined(__CYGWIN__) || defined(__MINGW32__)
> > is still needed.
> 
> Is there a (good) reason for that? Other packages for it seem to rely on a
> configure script, so it seems reasonable for us to do so, and avoid adding
> platform specific preprocessor macros in the source.

There are platform-specific macros in various places - mostly in */*_.h's (e.g. stdio_.h, unistd_.h) so at least your new changes are compatible in the general direction and better than put them in *.c's. configure is a shell thing, which is somewhat extra to the mingw ethos. it is probably not important either way.
Comment 10 Chris Liddell (chrisl) 2012-07-20 13:52:32 UTC
I committed a change a while back which sorts this out.