The patch has a few unnecessary white space changes.

The fontpath treatment seems wrong, or at least potentially confusing - the windows convention is ";" to separate path elements.

Also some of the comments suggest that windows build generated this way is subtly buggy? Requiring users to install gtk runtime libs is also unacceptable.

Also, you should make sure you do not break the usual and much more well-tested windows build (with MSVC) - since you only change two files, gs/base/gp_mswin.h gs/base/gs.mak, that's probably taken for granted, but you still need to do that. BTW, MSVC Express is available for free download, and it works well with wine, if that helps. I helped fixed the more important wine-related bug(s) a few years ago; there is still an open one with a simple work-around - see wine's bug tracker if you are interested. The one about the "+" syntax in cmd copy.

Anyway, I suggest you should test that it does not affect MSVC build, and address the unnecessary gtk dependency; I'll leave Chris to decide the rest.

Thank you, Hin-Tak, for your comments.  gp_mswin.h works well under MSVC, but I should move the MinGW-specific code from gs.mak to mingw-fs.mak to avoid the syntax clashes with MSVC (pardon! I should have seen that...)

The patch does not require that users install gtk, but rather provides them with an option to use Ghostscript with gtk under windows, one which currently does not exist.  And since it is possible to have several installations of Ghostscript on a single machine, a user could now cross-develop an application for both Windows and Linux using Ghostscript, and see the results with both gtk and gdi.  That being said, the console-level communication between gtk+ under windows and gsx needs to be fixed, which I believe can best be done independent of the work on the MinGW build scripts.

As for the FontPath treatment: it is correct.  The colon is later changed into the system's path delimiter (;), which takes places within the application.  This can be verified by executing gsc -h or gsx -h.

Created attachment 9142 [details]
MSys/MinGW buils scripts: REPLACEMET of the original patch

Greetings,

Thanks again for your feedback.  The new REPLACEMENT patch takes a somehow different approach to the handling of FontPath and GS_LIB_DEFAULT under MinGW, specifically in the form of a single line added to gs.mak, and which reads $(MINGW_FONTPATH_PATCH_OR_EMPTY_COMMAND)

As its names suggest, this variable is only set under MSys/MinGW (at the bottom of unixhead.mak) to call an external shell script, mingw-fp.sh, which replaces the backslash of GS_LIB_DEFAULT with a forward slash.  With all other platforms, this variable is empty, and thus does not affect the build process.

To remove all doubts, the above has been tested with MSVC, MSys/MinGW, and Linux/GCC, and was found to be fully working.

To be on the safe side, the prototype definition at the bottom of gp_mswin.h is now surrounded by #ifdef __GNUC__ ... #endif, and thus has no impact on the MSVC build.  This, too, has been tested, and was found to be fully working.

As for the white space changes: these should be all fixed now, or at least that what is showing on my end using WinMerge...

See also bug 691520 , bug 691555 , bug 693356 .

FWIW, also note that support for windows build with two other compilers (borland & watcom) other than MSVC were recently removed.

Yes indeed - but I think this is a somehow different case, specifically since it allows MinGW to be integrated into the unix scripts, and thus continue to be supported in the future without any additional overhead.  The only excpetion to that would be a change in the dependencies of core objects such as mkromfs, which is very unlikely.

Note also that compilation with older MSVC compilers, include MSVC 6, is no longer possible -- at least not without some hacking done.  The reason is that some of the current code uses "new" types such as long long, which MSVC 6 does not support.

I'm afraid I have more issues with this patch-set even than Hin-Tak - and I haven't been through it a lot of detail yet, but.....

I've worked very hard to keep single platform definitions out of the top level Makefile(.in) - things like:
"MINGW_BUILD=@MINGW_BUILD@"

I've also worked hard to keep conditionals out of the Makefiles, so all the:
"ifeq ($(MINGW_BUILD), 1)"

will need to be handled in some less GNU make specific fashion. *Especially* the ones in base/*.mak files (except the mingw specific ones, as they will always be used with GNU make).

I prefer to avoid platform specific stuff in what should be platform agnostic makefiles, so the use of "MINGW_FONTPATH_PATCH_OR_EMPTY_COMMAND" in gs.mak goes against that.

Calling out to the shell is, again, (GNU) make specific, and shouldn't be done in unixhead.mak.

I think most of the above can be resolved by rejigging how and what values are set by configure.

Thank you, Chris, for your comments and feedback!

As I was working on the patch, my main two goals were:

1) to minimize as much as possible the changes to configure.ac and the various .mak files, and ensure that whatever happens under MSys/MinGW, the build under all other platforms would not be affected by it.

2) to integrate the MSys/MinGW build process into the existing Unix build system, so that there is no need to separately maintain it in the future.

My working assumption was that _ifeq_ would be available on all unix-like platforms, however if that is not true please let me know.


As for MINGW_FONTPATH_PATCH_OR_EMPTY_COMMAND: gs.mak is already not 100% platform agnostic, specifically since it passes GS_LIB_DEFAULT to gconfigd.h without handling the possible presence of a backslash.  As it were, two alternatives to MINGW_FONTPATH_PATCH_OR_EMPTY_COMMAND, which I consider to be more intrusive, would be to either a) add a mingw-specific .mak file between gs.mak and psromfs.mak, or b) add a feature to echogs that would allow the substitution of the backslash with a forward slash.


Just to make sure: there is no call to the shell from unixhead.mak -- what takes place is a mere definition of MINGW_FONTPATH_PATCH_OR_EMPTY_COMMAND with a reference to the shell, yet without executing it.


One other thing about ifeq_ has to do with mkromfs: the dependencies of mkromfs are found in unix-aux.mak, and are obviously different under Windows.  So if one were to avoid _ifeq_ altogether, one would have to either a) split unix-aux.mak, with dependencies for mkromfs configured in two separate .mak files, of which only one would be included in the build process, or b) set these dependencies already in configure.ac.  In my view, the incidental use of _ifeq_ is a somehow more elegant solution (or at least the lesser evil), but I can also see why you would prefer to avoid it...

(In reply to comment #7)
(In reply to comment #8)

Yes, I believe conditionals are GNU-make-ism. By MSVC in my earlier comment, I did mean nmake + MSVC. (as well as through the bundled project files).

One way on mingw/cygwin/linux to avoid GNU-make-ism is to try bmake. bmake is BSD make and available on some combos of mingw/cygwin/linux; although there are some already some GNU-make-ism in some of the makefiles as well as GNU-bash-ism in some of the configure.

As noted above, some GNU-isms have crept in, but I'm trying to remove them as I find them.

ifeq doesn't exist in Sun make, and as Hin-Tak mentioned, not in BSD make.

If you look around the makefiles, (particularly the target "punning" using the "_0"/"_1" suffixes) there are quite a few hoops we go through to cope with not have conditionals on all make implementations.

GS_LIB_DEFAULT is used as the Postscript compatible path to the lib files, and always uses forward slashes as directory separators. If necessary, the directory separators are replaced by the platform specific file system code.

Thanks for pointing out the absence of _ifeq_ from the Sun and BSD make systems.  That definitely complicates things a bit...

The main problem with GS_LIB_DEFAULT is that the presence of an un-escaped backslash in gconfigd.h in the first place makes it impossible to handle it correctly from within the code.  So as it seems, this variable's value must be manipulated before gconfigd.h gets included by any of the other source files.

That being said, and given that the patch's reliance on _ifeq_ is an apparent obstacle, may I suggest that for the time being, a reference to the patch is added to the documentation, perhaps to 

http://ghostscript.com/doc/current/Make.htm

This would provide MSys/MinGW users with an easy way to compile Ghostscript from source, yet without risking the build process under Sun or BSD.

Assuming that everything goes well, we could then avoid using _ifeq_ in the build scripts by way of introducing a few more variables -- all to be set by ./configure -- and thus keep Makefile(.in) and the base/*.mak files as platform agnostic as possible.

I can certainly put a link in the documentation to the patch, my worry is that the Ghostscript build changes with fair regularity, so the danger of the patch failing to apply cleanly is quite clear and present!

As I said above, I reckon the conditionals can (almost?) all be resolved by rejigging stuff a bit. For example, instead of:
  # select platform: unix or mingw
  ifeq ($(MINGW_BUILD), 1)
    PLATFORM=mingw_
  else
    PLATFORM=unix_
  endif	

I would add something like:
  PLATFORM=unix$(SUBPLATFORM)_

Then in Makefile.in:
  SUBPLATFORM=@SUBPLATFORM@

and have the configure script set "SUBPLATFORM" to "" for most Unix systems, but to "_mingw" as required. Then the "PLATFORM" value for mingw would be "unix_mingw_"

So the approach, in the platform independent makefiles, is not mingw specific, but is a general solution for any future Unix-like variations that stray too far from the Unix norm to "just work".

Instead of the "MINGW_FONTPATH_PATCH_OR_EMPTY_COMMAND" workaround, two thoughts occur to me. One is to have the explicit path separators in Makefile.in replaced with autoconf markers @DIR_SEP@ and have configure set them appropriately for ordinary Unix or mingw. The other means splitting the "$(gconfigd_h)" target into multiple targets, as we've done for others (like the "SHARE_*" targets), using the "_mingw" SUBPLATFORM setting outlined above. Then the $(gconfigd_h)_mingw target can be in the mingw specific makefile.

I can give more detail on that if required.


For the inclusion of the mingw specific makefiles in the toplevel, instead of the conditional, see how we currently handle the CUPS and "contrib" makefiles. In fact, a slightly neater way is that I recently added a "stub" makefile, which is empty except for a few comments. Instead of having configure substitute the entire "include" lines, you can simply have it replace the file names so the mingw builds would include mingw-fs.mak and mingwlib.mak, but the non-mingw builds would include stub.mak twice.



Oh, and please use C style comments, not C++, we're trying not to require a C99 compiler just yet!

That's all for now.....

Thanks Chris - let us then move all the conditionals and logic to configure.ac and split the relevant target information files (most importantly those containing gconfigd_h and mkromfs), to be then handled in a way similar to CUPS and CONTRIB.

With @DIR_SEP@ we might be introducing one adventure too much, not to mention the fact that if a user passes --with-fontpath=c:\Users\Rumpelstiltskin\my-legendary-font-directory to configure, then we are back to square one.


Any ideas regarding the best way to update the .mak-dependency information in the builds that do not use configure?  One obvious example where this is needed is winlib.mak, which currently has

     !include $(GLSRCDIR)\gs.mak

I believe the easiest would be to add

     !include $(GLSRCDIR)\gconfgen.mak ('gen' as in 'generic'),

however if anything else comes to mind please let me know...

*** Bug 693555 has been marked as a duplicate of this bug. ***

Sorry, I missed the additional points before, and I haven't time to look at this more thoroughly.....

(In reply to comment #13)
> With @DIR_SEP@ we might be introducing one adventure too much, not to
> mention the fact that if a user passes
> --with-fontpath=c:\Users\Rumpelstiltskin\my-legendary-font-directory to
> configure, then we are back to square one.

For that case, we can easily have code in configure to modify a user supplied path appropriately.

> Any ideas regarding the best way to update the .mak-dependency information
> in the builds that do not use configure?  One obvious example where this is
> needed is winlib.mak, which currently has
> 
>      !include $(GLSRCDIR)\gs.mak
> 
> I believe the easiest would be to add
> 
>      !include $(GLSRCDIR)\gconfgen.mak ('gen' as in 'generic'),
> 
> however if anything else comes to mind please let me know...

Ideally, I'd like to avoid meddling with the non-configure builds as far as possible. Two things come to mind if you have no choice: for *Windows only* files (that is, ones only intended for use with nmake) we can use conditionals. Secondly, use the "target punning" I mentioned in comment #10.

(In reply to comment #15)
> (In reply to comment #13)
> > With @DIR_SEP@ we might be introducing one adventure too much, not to
> > mention the fact that if a user passes
> > --with-fontpath=c:\Users\Rumpelstiltskin\my-legendary-font-directory to
> > configure, then we are back to square one.
> 
> For that case, we can easily have code in configure to modify a user
> supplied path appropriately.

Understood, that should definitely work.


> > Any ideas regarding the best way to update the .mak-dependency information
> > in the builds that do not use configure?  One obvious example where this is
> > needed is winlib.mak, which currently has
> > 
> >      !include $(GLSRCDIR)\gs.mak
> > 
> > I believe the easiest would be to add
> > 
> >      !include $(GLSRCDIR)\gconfgen.mak ('gen' as in 'generic'),
> > 
> > however if anything else comes to mind please let me know...
> 
> Ideally, I'd like to avoid meddling with the non-configure builds as far as
> possible. Two things come to mind if you have no choice: for *Windows only*
> files (that is, ones only intended for use with nmake) we can use
> conditionals. Secondly, use the "target punning" I mentioned in comment #10.


I need to look deeper into this, however I believe that the above is the only place where such "meddling" might be needed:)  But sure, I'm more than happy to use the other approach you suggested.

> 
> I need to look deeper into this, however I believe that the above is the
> only place where such "meddling" might be needed:)  But sure, I'm more than
> happy to use the other approach you suggested.

I would need to find time to look in more detail, too. It's possible what I've suggested isn't viable, but I'd like to know for sure.

NOTE: an issue came up on gs-devel that on mingw the stdin/stdout/stderr file descriptors are not "binary" by default, and they need to be set to binary for piping to work correctly.

As far as I am aware, Ghostscript now builds and works on MSYS/MinGW "out of the box" - I have heard from several people using it, and a couple of recent bug reports when we've added stuff not supported on MinGW (pread/pwrite, for example).

So, I'm closing this as I think we've addressed all these related issues via other routes.