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

[Julien Fischer]

On Sun, 22 Oct 2023, Zoltan Somogyi wrote:

> 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.

Done.

>> 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".

Done.

>
>> --- a/runtime/mercury_runtime_util.c
>> +++ b/runtime/mercury_runtime_util.c

...


>> @@ -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.

Done.

>> +// 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.

I have changed that comment to:

// Strip any directory components from the argument.
// On Windows, this will also strip the ".exe" extension.
// A ".exe" extension on other systems (e.g. Linux) will be left alone.

> Either this comment, and the additions to the user facing documentation,
> should make clear that the stripping of .exe happens only on windows,

Done.

> 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.

I would be a bit suprised if it were stripped on a non-Windows systems,
if only because me including it would be fairly unusual. My expectations
on Windows would be diffferent: there file extensions are often hidden
or omitted.

...

>> @@ -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.

You're right, I had the behaviour strrchr() backwards.
I've fixed the log message.

> The diff is fine otherwise.

Thanks for the review.

Julien.