Re: Minor build fixes

[Gavin Smith]

On Thu, Oct 2, 2014 at 5:46 PM, Ken Brown <address@hidden> wrote:
> I just took over as the Cygwin maintainer of Texinfo, and I bumped into a
> few (easily fixed) build problems.  The first two exist in texinfo-5.2, and
> the rest only in the trunk.

Thanks for the report. Comments below:

> 1. The compiler produces lots of warnings about setmode being redefined.  It
> would be simple to silence the warnings, but the real issue in my opinion is
> that Cygwin should be treated the same as Unix, so that there's no need for
> setmode at all:
>
> Index: system.h
> ===================================================================
> --- system.h    (revision 5852)
> +++ system.h    (working copy)
> @@ -119,6 +119,13 @@
>      - text files can have their lines ended either with \n or with \r\n
> pairs;
>     These are all parameterized here except the last, which is
>     handled by the source code as appropriate (mostly, in info/).  */
> +
> +/* Cygwin is a Posix system even though it allows the use of O_BINARY.  */
> +#ifdef __CYGWIN__
> +# undef O_BINARY
> +# define O_BINARY 0
> +#endif
> +
>  #ifndef O_BINARY
>  # ifdef _O_BINARY
>  #  define O_BINARY _O_BINARY
> @@ -152,23 +159,13 @@
>  #  define HAVE_LONG_FILENAMES(dir)   (1)
>  #  define NULL_DEVICE  "NUL"
>  # endif  /* O_BINARY && !__MSDOS__ */
> -# ifdef __CYGWIN__
> -#  define DEFAULT_TMPDIR       "/tmp/"
> -#  define PATH_SEP     ":"
> -#  define STRIP_DOT_EXE        0
> -#  undef NULL_DEVICE
> -#  define NULL_DEVICE "/dev/null"
> -#  define PIPE_USE_FORK        1
> -# else  /* O_BINARY && !__CYGWIN__ */
> -#  ifdef __MINGW32__
> -#   define SET_SCREEN_SIZE_HELPER terminal_prep_terminal()
> -#  endif  /* _WIN32 */
> -#  define DEFAULT_TMPDIR       "c:/"
> -#  define PATH_SEP     ";"
> -#  define STRIP_DOT_EXE        1
> -#  define PIPE_USE_FORK        0
> -# endif /* O_BINARY && !__CYGWIN__ */
> -  /* Back to any O_BINARY system.  */
> +# ifdef __MINGW32__
> +#  define SET_SCREEN_SIZE_HELPER terminal_prep_terminal()
> +# endif  /* _WIN32 */
> +# define DEFAULT_TMPDIR        "c:/"
> +# define PATH_SEP      ";"
> +# define STRIP_DOT_EXE 1
> +# define PIPE_USE_FORK 0
>  # define FILENAME_CMP  mbscasecmp
>  # define FILENAME_CMPN mbsncasecmp
>  # define FOPEN_RBIN    "rb"
>

If I understand this right, O_BINARY is being checked to see if it is
a Posix-style system, right, but Cygwin has O_BINARY defined? O_BINARY
is not actually being set in system.h for the benefit of source code
including it? I noticed in info/filesys.c O_BINARY is used for opening
a file, so will it matter if this value is 0 under Cygwin?

The problem with setmode is that it is being redefined as "_setmode",
correct? Does anyone know what systems need to use _setmode instead of
setmode?

I see the SET_BINARY macro, whose expansion uses setmode, is used in
info/makedoc.c. Would it matter if setmode wasn't used here under
Cygwin?

Does it matter that the other macros will get the Unix definitions,
e.g. HAVE_DRIVE will always be false, comparison of filenames will be
case-sensitive, "\" will not be allowed as a directory separator?

> 2. The following patch provides the prototype for ioctl:
>
> Index: info/termdep.h
> ===================================================================
> --- info/termdep.h      (revision 5852)
> +++ info/termdep.h      (working copy)
> @@ -49,7 +49,7 @@
>  #  endif /* !HAVE_TERMIO_H */
>  #endif /* !HAVE_TERMIOS_H */
>
> -#ifdef GWINSZ_IN_SYS_IOCTL
> +#ifndef __MINGW32__
>  #  include <sys/ioctl.h>
>  #endif
>

I see the same thing (conditional on __MINGW32__) is done when
including sys/ioctl.h in info/ioctl.h and info/man.h. It looks like
GWINSZ_IN_SYS_IOCTL is set by the AC_HEADER_TIOCGWINSZ macro in
configure.ac. However, your patch is probably what we want because
we're not just including <sys/ioctl.h> to get TIOCGWINSZ (although I
don't understand why you would want the definition of TIOCGWINSZ and
/not/ have the definition of ioctl, so maybe I'm missing something?)

> 3. The following is needed for linking, now that info/info-utils.c uses
> libiconv:
>
> Index: info/Makefile.am
> ===================================================================
> --- info/Makefile.am    (revision 5852)
> +++ info/Makefile.am    (working copy)
> @@ -27,7 +27,7 @@
>    -DINFODIR=\"$(infodir)\"                      \
>    -DINFODIR2=\"$(datadir)/info\"
>
> -LDADD = $(top_builddir)/gnulib/lib/libgnu.a $(TERMLIBS) $(LIBINTL)
> +LDADD = $(top_builddir)/gnulib/lib/libgnu.a $(TERMLIBS) $(LIBINTL)
> $(LIBICONV)
>
>  EXTRA_DIST = README pcterm.c
>

This looks good. gnulib-tool should report on what should be added to
LDADD. I ran "../gnulib/gnulib-HEAD-d9361da/gnulib-tool --update
--dry-run" (ignore the path to gnulib-tool), and in the output it did
suggest $(LIBICONV) in LDADD.

> 4. info-utils.c wouldn't compile for me because Cygwin doesn't have the
> header <nl_types.h>.  But this doesn't seem to be needed; the compilation
> worked if I just removed that include:
>
> Index: info/info-utils.c
> ===================================================================
> --- info/info-utils.c   (revision 5852)
> +++ info/info-utils.c   (working copy)
> @@ -24,7 +24,6 @@
>  #include "info-utils.h"
>  #include "tag.h"
>
> -#include <nl_types.h>
>  #include <langinfo.h>
>  #if HAVE_ICONV
>  # include <iconv.h>
>

OK, it doesn't look like it's needed.

> 5. Now that infokeys is no longer a standalone program, the attempt to
> create a man page for it fails.  I assume the following is the right fix:

Yes, I would assume so.