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

[Zoltan Somogyi]

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

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

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

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

The rest is fine.

Zoltan.