[m-rev.] for review: fix runtime program basename on Windows

[Zoltan Somogyi]

On 2023-10-22 03:05 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
> Also, none of the uses of the program basename in the runtime currently account
> for the presence of the .exe executable extension on Windows. This diff makes
> it so that the .exe extension is not required in those uses.

I would say something like: After this diff, users won't have to, and in fact will not be
allowed to, include the .exe extension in the program name in these contexts.


> diff --git a/doc/user_guide.texi b/doc/user_guide.texi
> index ed2a031..517c21f 100644
> --- a/doc/user_guide.texi
> +++ b/doc/user_guide.texi
> @@ -10803,6 +10803,8 @@ The options given in this environment variable apply to every program;
>  the options given in an environment variable
>  whose name is of the form @env{MERCURY_OPTIONS_ at var{progname}}
>  apply only to programs named @var{progname}.
> +(Note that @var{progname} does @emph{not} include the @file{.exe} on those
> +systems that use it.)

I would add "extension" after ".exe".

> --- a/runtime/mercury_runtime_util.c
> +++ b/runtime/mercury_runtime_util.c
> @@ -1,7 +1,7 @@
>  // vim: ts=4 sw=4 expandtab ft=c
> 
>  // Copyright (C) 2001-2002, 2006 The University of Melbourne.
> -// Copyright (C) 2014, 2016, 2018 The Mercury team.
> +// Copyright (C) 2014, 2016, 2018, 2023 The Mercury team.
>  // This file is distributed under the terms specified in COPYING.LIB.
> 
>  // This module contains utility functions for the rest of the Mercury runtime.
> @@ -10,9 +10,14 @@
> 
>  #include    "mercury_imp.h"
>  #include    "mercury_runtime_util.h"
> +#include    "mercury_string.h"
> +#include    "mercury_windows.h"
> 
>  #include    <stdio.h>
>  #include    <string.h>
> +#if defined(MR_WIN32) && !defined(MR_CYGWIN)
> +   #include <wchar.h>
> +#endif
> 
>  #ifdef MR_HAVE_UNISTD_H
>    #include  <unistd.h>
> @@ -148,3 +153,39 @@ MR_setenv(const char *name, const char *value, int overwrite)
>    #error "MR_setenv: unable to define"
>  #endif
>  }
> +
> +// XXX TODO: Cygwin -- strip .exe extension if present.
> +const char *
> +MR_get_program_basename(const char *program_name)
> +{
> +    const char  *basename;
> +
> +    #if defined(MR_WIN32) && !defined(MR_CYGWIN)
> +
> +        wchar_t wname[_MAX_FNAME];
> +
> +        int err = _wsplitpath_s(MR_utf8_to_wide(program_name),
> +            NULL, 0,  // Ignore drive.
> +            NULL, 0,  // Ignore directories.
> +            wname, _MAX_FNAME,
> +            NULL, 0   // Ignore .exe extension.
> +        );
> +        if (err != 0) {
> +            MR_fatal_error("Could not split path");
> +        }
> +        basename = MR_wide_to_utf8(wname, NULL);

Based on

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/
splitpath-s-wsplitpath-s?view=msvc-170

I would change the type of err to errno_t.

> +// Strip any directory components and the ".exe" extension (if present) from
> +// the argument.
> +extern const char   *MR_get_program_basename(const char *);

This comment implies that this function *always* strips ".exe".
On linux, people don't *have* to add a .exe extension to executables,
but they *can*, and the non-windows path won't strip that off.

Either this comment, and the additions to the user facing documentation,
should make clear that the stripping of .exe happens only on windows,
or the non-windows path should also strip off ".exe". I don't have a preference
for either course of action. I think the latter is simpler in the long term,
but it *would* be a breaking change.

>  #endif  // MERCURY_RUNTIME_UTIL_H
> diff --git a/runtime/mercury_wrapper.c b/runtime/mercury_wrapper.c
> index ae768e2..e6ba7bc 100644
> --- a/runtime/mercury_wrapper.c
> +++ b/runtime/mercury_wrapper.c
> @@ -55,6 +55,7 @@ ENDINIT
>  #include    "mercury_deep_profiling.h"
>  #include    "mercury_memory.h"          // for MR_copy_string()
>  #include    "mercury_memory_handlers.h" // for MR_default_handler
> +#include    "mercury_runtime_util.h"    // for MR_get_program_basename()
>  #include    "mercury_thread.h"          // for MR_debug_threads
>  #include    "mercury_threadscope.h"
> 
> @@ -1076,17 +1077,9 @@ MR_process_environment_options(void)
>          gen_env_options = (char *) "";
>      }
> 
> -    // Find out the program's name, stripping off any directory names.
> -    // XXX WINDOWS: the path separator is incorrect for non-Cygwin Windows.
> -    // It also does not handle drive qualified paths. And presumably the
> -    // name of the program-specific options should not include the .exe
> -    // extension.
> -    progname = MR_progname;
> -    for (s = progname; *s != '\0'; s++) {
> -        if (*s == '/') {
> -            progname = s + 1;
> -        }
> -    }

Given that there is no break statement after the assignment to progname,
I think this code did strip off all directory names from the pathname,
not just one. I would therefore delete the comments about this in the log message.

> +    // Find out the program's name, stripping off any directory names and the
> +    // .exe extension on those systems that use it.
> +    progname = MR_get_program_basename(MR_progname);
> 
>      // Build the program-specific option's name: MERCURY_OPTIONS_progname.
>      mercury_options_len = strlen(MERCURY_OPTIONS);
> @@ -1407,6 +1400,7 @@ struct MR_option MR_long_opts[] = {
>      { NULL,                             0, 0, 0 }
>  };
> 
> +
>  static void
>  MR_process_options(int argc, char **argv)
>  {
> @@ -2341,12 +2335,7 @@ MR_matches_exec_name(const char *option)
>      char        *s;
>      const char  *exec_name;
> 
> -    s = strrchr(MR_progname, '/');
> -    if (s == NULL) {
> -        exec_name = MR_progname;
> -    } else {
> -        exec_name = s + 1;
> -    }

This also worked (for ASCII paths) since strrchr locates the LAST /, not the first.

The diff is fine otherwise.

Zoltan.