Re: [PATCHES] patches for compiling GNU emacs 21.2 under Cygwin

[Eli Zaretskii]

> From: Joe Buehler <address@hidden>
> Date: Thu, 26 Sep 2002 15:10:30 -0400
> 
> Attached are patches required to get emacs 21.2 up and running under Cygwin.
> 
> Some of the patches have already been submitted, and so are split into a 
> separate
> attachment.  I include them here for completeness, in case they never made it
> into CVS yet.
> 
> Others are submitted here for the first time -- I have been waiting for my VP
> to sign a copyright assignment for FSF.  I just got it and will get it in
> the mail tomorrow.

First, thank you for your contribution.  And sorry for such a long
delay in reviewing these patches.

Below are some comments based on reading the patches.  (I cannot
actually apply the patches and try using them, since I don't have the
Cygwin development enviroment installed.)

Please provide ChangeLog entries for each change.

Please also explain the changes the reason for which is not
self-evident.  For example, in this change to Makefile.in:

 -         (cd $(docdir); chmod a+r DOC*; rm DOC); \
 +         (cd ${docdir}; chmod a+r DOC*; if test "`echo DOC-*`" != "DOC-*"; \
 +              then rm DOC; fi); \

why is it necessary to add this test?  Is there something special in
the way the Cygwin port of Bash expands wildcards?

The purpose of this change is also not clear enough:

 +#ifdef CANNOT_DUMP
 +  FRAME_FOREGROUND_PIXEL(f) = FACE_TTY_DEFAULT_FG_COLOR;
 +  FRAME_BACKGROUND_PIXEL(f) = FACE_TTY_DEFAULT_BG_COLOR;
 +#endif

What is special about systems that cannot dump that requires this
fragment?

 -  (if (memq system-type '(hpux dgux usg-unix-v irix linux gnu/linux))
 +  (if (memq system-type '(hpux dgux usg-unix-v irix linux gnu/linux cygwin))

Since we are introducing a new value for system-type, it should be
documented in the ELisp manual, I think.

In this change:

 +#ifdef CYGWIN
 +#define _CYGWIN_EXE_SUFFIX .exe
 +#else
 +#define _CYGWIN_EXE_SUFFIX
 +#endif
 +
  install: ${archlibdir}
         @echo
         @echo "Installing utilities for users to run."
         for file in ${INSTALLABLES} ; do \
           $(INSTALL_PROGRAM) $(INSTALL_STRIP) $${file} ${bindir}/$${file} ; \
 -        chmod a+rx ${bindir}/$${file}; \
 +        chmod a+rx ${bindir}/$${file}_CYGWIN_EXE_SUFFIX; \
         done
         for file in ${INSTALLABLE_SCRIPTS} ; do \
           $(INSTALL_PROGRAM) ${srcdir}/$${file} ${bindir}/$${file} ; \
           chmod a+rx ${bindir}/$${file}; \
         done

I don't really understand why is it necessary to introduce
_CYGWIN_EXE_SUFFIX.  On Windows, there's no need to run "chmod a+rx"
on *.exe programs.  If the issue is that there's no etags, say, but
etags.exe, and that chmod will print an error message if run on a
non-existent file, why not test if $$(file) exists and only run chmod
if it does?  Isn't that a simpler and cleaner solution?
 
 --- src/Makefile.in    2001-12-17 09:09:32.000000000 -0500
 +++ src/Makefile.in    2002-08-02 11:59:25.000000000 -0400
 @@ -836,7 +836,11 @@
  emacs: temacs ${etc}DOC ${lisp}
  #ifdef CANNOT_DUMP
         rm -f emacs
 +#ifdef CYGWIN
 +      ln temacs.exe emacs
 +#else
         ln temacs emacs
 +#endif

I think it's cleaner to introduce the EXE variable here, make it
empty on all platforms except Cygwin, and then use it everywhere,
instead of adding many #ifdef's.

 +/* let's not be adventurous */
 +#define SYSTEM_MALLOC 1

Why is that a good idea?  Isn't it better to use gmalloc and ralloc?
That should make an Emacs whose memory footprint does not grow that
much, I think.  What is so ``adventurous'' in using that?

Also, what about many "#ifdef WINDOWSNT" and "#ifdef DOS_NT" that we
have in the sources--are none of them appropriate for the Cygwin
build?  For example, what about setting system_eol_type (on coding.c)
to CODING_CR_LF?  Or about the part of editfns.c that gets the user
id from the enviroment variable USERNAME?  Or support for drive
letters in file names in fileio.c?  Isn't it better not to lose these
and other Windows-specific features?

I think Someone(tm) should also go through all the *.el files, look
for code that's specific to system-type windows-nt, and see which ones
should also work for system-type cygwin.