Re: Building sharutils 4.13.4 with MinGW

On 04/07/13 10:35, Eli Zaretskii wrote:
> Here are the MinGW-specific problems with building and testing this
> release (copy to gnulib list, because many issues are related to
> gnulib modules).
>
> 1. Compilation fails in lib/:
>
>     This is because idcache.h includes sys/types.h, but gnulib's
>     sys/types.h does not define uid_t and gid_t, and also there are no
>     pwd.h and grp.h in lib/.
>
>     My solution was to add private pwd.h and grp.h to lib/, and modify
>     idcache.h to include them.  But I guess a better solution would be
>     to import more from gnulib, or perhaps enhance the corresponding
>     gnulib modules.

Indeed.  "enhance the corresponding gnulib modules" because gnulib is
supposed to recursively include everything needed.

> 2. Compilation warning in lib/:

>     This is because functions getpwuid and getgrgid are unavailable and
>     there are no replacements for them in gnulib.
>
>     To solve this, I added the missing functions to idcache.c:

That would require gnulib cooperation, too.
Otherwise, I have to glue it into sources I control.

> 3. Compilation warnings in src/shar.c:
>
>       gcc -DLOCALEDIR=\"d:/usr/share/locale\" -DHAVE_CONFIG_H -I. -I..  -I../libopts -I. -I.. -I../lib -I../intl -Id:/usr/include -Wno-format-contains-nul -g3 -O2 -Wno-format-contains-nul -MT shar.o -MD -MP -MF .deps/shar.Tpo -c -o shar.o shar.c
>       shar.c: In function 'walktree':
>       shar.c:595:18: warning: assignment makes pointer from integer without a cast [enabled by default]
>       shar.c: In function 'set_submitter':
>       shar.c:1943:18: warning: initialization makes pointer from integer without a cast [enabled by default]
>       make[2]: *** [shar.o] Error 1
>
>     The problem here is with basename and dirname, whose declarations
>     are absent.  My solution was to add "#include <libgen.h>" to
>     lib/system.h (libgen.h in MinGW declares basename and dirname).
>     However, I wonder if gnulib has a better solution.

I would hope.  Thanks.

> 4. Link errors for shar.exe:
>     Solution: reorder in src/Makefile.in:

Pasted into my Makefile.am.

>     For the other errors: I wrote emulations for fork (always fails),
>     wait (always fails) and pipe (calls _pipe), and also add getuid,
>     getpwnam and getgrnam to idcache.c.  However, I think gnulib has
>     replacements for at least some of these functions.

Not that I can find.
> $ list=$(find * -type f)
> $ fgrep getuid $list
> $ fgrep getpwnam $list
> $ fgrep getgrnam $list
> $ echo "$list"|egrep "getuid|getpwnam|getgrnam"
> $


> 5. Link errors for unshar.exe:

>     Solution: add mkstemp to unshar.c.  Again, I think gnulib has a
>     module for that, so importing it will solve this.

Done.  (Added to gnulib modules)

> 6. uutest-1 test fails because the begin line doesn't match.  This is
>     because the mode bits on Windows are not real, beyond the user-read
>     and user-write bits.  So it would be nice if on Windows Diff would
>     be invoked like this:
>
>       DIFF="diff -I^begin"
>
> 7. shar-4 fails because it uses cmp to compare results with expected
>     ones, which fails due to line endings.  Suggest to use Diff
>     instead.

Could you suggest a patch for these two, too? :)  Maybe a script test
so that I know that I have to:
   sed 's/^begin [0-7][0-7][0-7] /begin 664/'
the result file.

> 8. The configure script botches the popen-binary test: the test
>     program does not include stdio.h, so the compilation fails for
>     reasons that have no relation to the issue being tested.

gnulib

> 9. Using -! or --more-help triggers an error message:
>
>       d:\usr\eli\utils\sharutils-4.13.4>uuencode -!
>       uuencode: illegal option -- !
>       uuencode (GNU sharutils) - encode a file into email friendly text
>       Usage:  uuencode [ -<flag> | --<name> ]... [<in-file>] <output-name>
>       Try 'uuencode --help' for more information.

>     This is confusing because --help does show these options as available.
>
>     The reason is that uuencode-options.c does this:
>
>       #if HAVE_WORKING_FORK
>       #define MORE_HELP_DESC  (uuencode_opt_strs+1045)
>       #define MORE_HELP_name  (uuencode_opt_strs+1090)
>       #define MORE_HELP_FLAGS (OPTST_IMM | OPTST_NO_INIT)
>       #else
>       #define MORE_HELP_DESC  NULL  <<<<<<<<<<<<<<<<<<<<<<<<<
>       #define MORE_HELP_name  NULL  <<<<<<<<<<<<<<<<<<<<<<<<<
>       #define MORE_HELP_FLAGS (OPTST_OMITTED | OPTST_NO_INIT)
>       #endif
>
>     which is unnecessary, since the relevant libopts code already knows
>     how to handle the case of !HAVE_WORKING_FORK.
>
>     => Fixed by enabling the !HAVE_WORKING_FORK block above.
>
>     Same in shar-opts.c and uudecode-opts.c.

Lovely.  This is a dilemma.  In order to facilitate i18n, I cannot compute help
text on the fly.  If I don't compute it on the fly, then it has to be the same
text for all platforms.  I guess I should set it up to handle it -- only handle
it as if it were plain help.  Rats.

All four *-opts.c have the issue.  I've modified the source template:
> #ifdef HAVE_WORKING_FORK
> #define MORE_HELP_DESC  ([=<junk>=])
> #define MORE_HELP_name  ([=<more junk>=])
> #define MORE_HELP_FLAGS (OPTST_IMM | OPTST_NO_INIT)
> #else
> #define MORE_HELP_DESC  HELP_DESC
> #define MORE_HELP_name  HELP_name
> #define MORE_HELP_FLAGS (OPTST_OMITTED | OPTST_NO_INIT)
> #endif

I am also certain that is not everything.  Basically, on platforms without fork(),
you'll just get --help with --more-help.

> 10.shar fails while processing files:
>
>       d:\usr\eli\utils\sharutils-4.13.4>src\shar src\encode.c > foo.shar
>       shar: Saving src/encode.c (text)
>       The system cannot find the path specified.
>
>     This is because emit_char_ct_validation invokes 'wc' with bad
>     redirection:
>
>       set LC_ALL=C & wc -c < 'src/encode.c'
>
>     The file from which stdin is redirected uses forward slashes and is
>     quoted with '..'.
>
>     Solution: use a slightly different command for MinGW to run 'wc'
>     locally:
>
>       set LC_ALL=C & wc -c "src/encode.c"

Hmmm.  That routine is *called* with quoting it doesn't handle well.
The ``quoted_name'' argument is the string "'src/encode.c'".
It is the same argument as is passed to finish_sharing_file.
Anyway, if in the end MinGW requires double quotes for path names
that might have directory characters, it is starting to look like
it might be better to:
       local_name=`localize 'src/encode.c'`
       LC_ALL=C wc -c < $local_name
with the "localize" function most commonly being "echo".

> --- sharutils-4.13.4.orig/src/shar.c	2013-03-23 00:23:48.000000000 +0200
> +++ sharutils-4.13.4/src/shar.c	2013-04-07 13:41:14.742531000 +0300
> @@ -1065,14 +1097,27 @@ emit_char_ct_validation (
>     /* Shell command able to count characters from its standard input.
>        We have to take care for the locale setting because wc in multi-byte
>        character environments gets different results.  */
> +#ifdef __MINGW32__
> +  /* Windows needs a slightly different format to run wc locally:
> +     redirection will fail because the file uses Unix forward
> +     slashes.  */
> +  static char const local_cmd[] = "set LC_ALL=C & wc -c";
> +#endif
>     static char const cct_cmd[] = "LC_ALL=C wc -c <";
>
>     FILE *pfp;
>     char wc[16 /* log MAX_INT + a few extra */];
> -  char *command = alloca (sizeof(cct_cmd) + strlen (quoted_name) + 2);
>
> +#ifdef __MINGW32__
> +  /* On Windows, disregard Unix-style shell quoting and quote by hand
> +     using "..".  */
> +  char *command = alloca (sizeof(local_cmd) + strlen (local_name) + 2);
> +  sprintf (command, "%s \"%s\"", local_cmd, local_name);
> +#else
> +  char *command = alloca (sizeof(cct_cmd) + strlen (quoted_name) + 2);
>     sprintf (command, "%s %s", cct_cmd, quoted_name);
> -  pfp = popen (command, "r");
> +#endif
> +  pfp = popen (command, freadonly_mode);

I know it is difficult to keep straight which commands get executed where.
What is being emitted in this function is shell code that gets executed
on the receiving machine.  In other words, shell code that can be interpreted
by shell-on-windows and shell-on-real-posix.  Selecting the form of the
output based on __MINGW32__ at shar compile time is incorrect.
So the patch needed would be some boilerplate functions that behave differently
for Mingw and the rest of the world and it gets selected at unshar time.
This "wc -c" code gets replaced with that function call.

> 11.shar.c uses /dev/null in freopen, but does not use the gnulib
>     freopen module, which makes /dev/null work on MS-Windows.

Isn't there a POSIX layer that can be added to windows?
"Portable" POSIX is hard enough without some system that
masquerades as almost, kind-of-like POSIXy.

> There are a few other, less important issues, like with using binary
> I/O in some places.  let me know if you want me to send diffs for
> those.
>
> HTH

Some of it most certainly is.  Now to the next (previous) one:

> I've built this distribution with MinGW tools on MS-Windows.  Quite
> unexpectedly for such a veteran package, it turned out to be an uphill
> battle.

I had to change some options (add xz compression and handle a behavior
change mandated by our Austin Group friends), plus the docs were all
out of date and things had gotten crufty.  So I bit off too much.

> 1. configure errors out:
>
>      configure: error: you must have sys_mman.h on your system

>    I don't understand why configure insists on having sys/mman.h and
>    sys/wait.h, because the code has appropriate fallbacks for when
>    they are not present.

I think the correct fix is to do the gnulib futzing and then
set ac_cv_header_sys_wait_h and ac_cv_header_sys_mman_h to "yes".
The libopts stuff should follow that.  (I don't think stuffing
gnulib stuff into libopts would work so well.)

> 2. There's a bug in text_mmap.c (in the part that falls back to
>    reading a file if mmap is unavailable): the txt_zero_fd member of
>    tmap_info_t structure was not initialized to -1, so stayed at zero
>    and was closed (twice) by close_mmap_files, which caused an invalid
>    argument debug message from the runtime library.

Thank you.

> 3. Using the -R option seems to bypass encoding and only records the
>    options to the RC file -- is that intended?  If so, it seems to be
>    undocumented (I initially thought it was a bug, and only by
>    stepping through the code found out that uudecode -R simply exits
>    after outputting the options to the RC file).

I've added the following sentence to the man page and info doc templates.
This will show up in ntp and tls documentation, too:
   The command will exit after updating the config file.
It basically turns the program into a config file editor.

> 4. When passed 2 or more base64-encoded files, uudecode barfs on the
>    second one:

> --- sharutils-4.13.4.orig/src/uudecode.c	2013-03-29 17:28:40.000000000 +0300
> +++ sharutils-4.13.4/src/uudecode.c	2013-04-07 09:38:10.818181900 +0300
> @@ -497,6 +518,7 @@ multiple input files.\n"));
>                error (0, errno, "%s", f);
>                exit_status |= UUDECODE_EXIT_NO_INPUT;
>              }
> +          do_base64 = false;
>          }
>      }

Thanks. Fixed.

>
> 5. Question: can /dev/stdout or - appear on the encoded file's begin
>    line?  If it can, then

>    doesn't redirect stdout in that case.  So it looks like uudecode
>    will continue writing to the previously decoded file, instead of
>    resetting stdout to the original stream/descriptor.

That is an inherited feature. :)  I likely wordsmith-ed the following,
but it is an old caveat:

> BUGS
>        If more than one file is given to uudecode and the -o option  is  given
>        or  more  than one name in the encoded files are the same the result is
>        probably not what is expected.

> 6. shar fails to record the time and the submitter of the archive:
>
>      # Made on  by <>.
>
>    The time was missing because print_header_stamp assumed %Z format
>    in strftime always takes at most 4 characters:
     static char const ftime_fmt[] = "%Y-%m-%d %H:%M %Z";
     /*
      * All fields are two characters, except %Y is four and
      * %Z may be up to 30 (?!?!)
      */
     char buffer[sizeof (ftime_fmt) + 64];

>    My solution was to reallocate the buffer if strftime returns zero:

     {
       size_t l =
         strftime (buffer, sizeof (buffer) - 1, ftime_fmt, local_time);
       if (l > 0)
         fprintf (fp, made_on_comment_z, buffer, OPT_ARG(SUBMITTER));
     }

>    The problem with submitter was because set_submitter assigned a
>    local buffer to the submitter option argument, without using
>    xstrdup:

>    Solution: use xstrdup inside the SET_OPT_SUBMITTER call.

Hmm.  I misremembered my own code.  Rehacked:

> static void
> set_submitter (void)
> {
>   char * buffer;
>   char * uname = getuser (getuid ());
>   size_t len   = strlen (uname);
>   if (uname == NULL)
>     fserr (SHAR_EXIT_FAILED, "getpwuid", "getuid()");
>   buffer = xmalloc (len + 2 + HOST_NAME_MAX);
>   memcpy (buffer, uname, len);
>   buffer[len++] = '@';
>   gethostname (buffer + len, HOST_NAME_MAX);
>   SET_OPT_SUBMITTER(buffer);
> }

> P.P.S.  Thanks for maintaining sharutils!

It was supposed to be easy, and I went and made it harder.
Thank you.

http://autogen.sourceforge.net/data/

has both a "pre2" sharutils and the autogen for creating it.
(You do not need the latter, as the "libopts" gets rolled into sharutils.)
The issues that really need gnulib fixes are still in sharutils.
I'm also still trying to figure out how to select which
    ac_cv_header_xxx_h
values should be set to "yes".

Thank you again.  Regards, Bruce