[m-rev.] for review: add --install-method option

[Julien Fischer]

On Sun, 12 Nov 2023, Zoltan Somogyi wrote:

> On 2023-11-12 16:08 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
>
>> -    % This type specifies the command compiler uses to install files.
>> +    % This type specifies how the compiler should install files and
>> +    % directories. Values of this type only affect when the compiler does file
>> +    % installation (e.g. an install target with mmc --make); they do not affect
>> +    % file installation done by mmake.
>
> Is "an install target with mmc --make" just an example?

Yes. The point I was trying to get across there was these settings
do not affect how mmake installs files.

I have changed it to:

+    % This type specifies how the compiler should install files and
+    % directories. Values of this type only affect how the compiler itself does
+    % file installation; they do not affect file installation done by mmake.

> Is there are any other situation, current or foreseeable, in which we
> could use this setting? I suspect the answer is "no", in which case I
> would reword to avoid giving a misleading impression to readers.

File copying inside the compiler (using the value of
--install-file-command) is currently used for the following purposes:

    1. Installation targets with mmc --make.
    2. Copying files with mmc --make on systems on which symlinks are not
       available.
    3. Updating interface file.

>> +:- type install_method
>> +    --->    install_method_external
>> +            % Files and directories should be installed by invoking a command
>> +            % (e.g. cp or cp -R) via the shell of the underlying operation
>> +            % system. (See the file_install_cmd/0 type below.)
>> +
>> +    ;       install_method_internal.
>> +            % Files and directories should be installed by invoking an OS or
>> +            % target language call, or by using predicates implemented using
>> +            % standard Mercury file operations.
>
> I would s/install_method_external/install_method_external_cmd/, and
> would look for a similar addition to the function symbol as well (maybe "_code"
> as a suffix) to clarify the distinction.

Done.

>> diff --git a/compiler/handle_options.m b/compiler/handle_options.m
>> index 95759d8..679f82b 100644
>> --- a/compiler/handle_options.m
>> +++ b/compiler/handle_options.m
>> @@ -265,7 +265,7 @@ check_option_values(!OptionTable, Target, WordSize, GC_Method,
>>          WordSize = word_size_64,    % dummy
>>          BitsPerWordStr = string.int_to_string(BitsPerWord),
>>          WordSizeSpec =
>> -            [words("Invalid argument"), quote(BitsPerWordStr), +            [words("Invalid argument"), quote(BitsPerWordStr),
>>              words("to the"), quote("--bits-per-word"), words("option;"),
>>              words("must be either"), quote("32"), words("or"), quote("64"),
>>              suffix("."), nl],
>
> What is this doing?

I deleted some trailing whitespace in passing; ignore it.

>> @@ -846,6 +846,22 @@ convert_options_to_globals(ProgressStream, DefaultOptionTable, OptionTable0,
>>      OT_OptFrames0 = OptTuple0 ^ ot_opt_frames,
>>      OT_StringBinarySwitchSize0 = OptTuple0 ^ ot_string_binary_switch_size,
>>
>> +    lookup_string_option(OptionTable0, install_method, InstallMethodStr),
>> +    ( if (InstallMethodStr = "" ; InstallMethodStr = "external") then
>> +        InstallMethod = install_method_external
>> +    else if InstallMethodStr = "internal" then
>> +        InstallMethod = install_method_internal
>> +    else
>> +        InstallMethodSpec = [words("Error: the value of the"),
>> +            quote("--install-method"), words("option is"),
>> +            quote(InstallMethodStr), suffix(","),
>> +            words("but the only valid values are") ] ++
>> +            list_to_quoted_pieces_or(["external", "internal"]) ++
>> +            [suffix("."), nl],
>> +        add_error(phase_options, InstallMethodSpec, !Specs),
>> +        InstallMethod = install_method_external % Dummy value
>> +    ),
>
> The "or" in "the only valid values are `external' or `internal'" is wrong;
> that should be an "and". And I think using list_to_quoted_pieces_or here
> just obscures the intended message text.

Done.

Julien.