[m-rev.] for review: check_libgrades

[Julien Fischer]

On Sun, 21 Apr 2024, Zoltan Somogyi wrote:

> <to be filled in when the XXX NEWs are resolved>
> 
> compiler/check_libgrades.m:
>     Check whether the libraries exist just once, by recording the result
>     the first time, and later just using those results.
> 
> compiler/mercury_compile_main.m:
>     Move a large block of code, which is mostly one large comment,
>     of out of the do_op_mode_args predicate. Add two new XXX to the moved code,
>     and one to do_op_mode_args itself.

While reviewing this, I thought of the following situation.
Consider two programs 'a' and 'b' in the same directory, with a Mercury.options
file containing:

     MCFLAGS-b = --ml not_installed

and the invocation :

     $ mmc --make a b

This appears to bypass the existing libgrades check entirely :-(
Whether the libgrade check should cache stuff, depends on what the expected
behaviour for the above is.  Is the granularity of the check a superset of
all libraries used by all targets built in an invocation, or is it more specific
that that?

...

> diff --git a/compiler/mercury_compile_main.m b/compiler/mercury_compile_main.m
> index bcecf3f53..69061fb0c 100644
> --- a/compiler/mercury_compile_main.m
> +++ b/compiler/mercury_compile_main.m

...

> @@ -920,6 +885,10 @@ do_op_mode_args(ProgressStream, ErrorStream, Globals,
>          ExtraObjFiles = []
>      ),
> 
> +    % XXX NEW If LibgradeCheckSpecs is nonempty, we know that the linking step
> +    % will fail.
> +    % I (zs) don't think we should execute the following linking code
> +    % in that case, though I could be persuaded otherwise.

At a guess, this behaviour has been introduced by the one of the many
refactorings of op modes and / or handling or error messages.  There is
certainly no point in progressing to the link step if the library grade
check has failed.

> @@ -991,6 +960,62 @@ do_op_mode_args(ProgressStream, ErrorStream, Globals,
>          Statistics = no
>      ).
> 
> +:- pred check_libraries_are_installed_if_needed(globals::in,
> +    op_mode_invoked_by_mmc_make::in, list(error_spec)::out,
> +    io::di, io::uo) is det.
> +
> +check_libraries_are_installed_if_needed(Globals, InvokedByMmcMake,
> +        Specs, !IO) :-
> +    % We used to do this check once per compiler arg, which was quite wasteful.
> +    % However, there was an almost-justifiable reason for that. The check
> +    % used to be done in the now-deleted predicate process_compiler_arg_build,
> +    % the part of what is now setup_and_process_compiler_arg that happens
> +    % after the setup. This meant that check_libraries_are_installed
> +    % was called not with the original globals (which is Globals here),
> +    % but with the globals created by setup_for_build_with_module_options.
> +    % However, the only options in the globals structure that
> +    % check_libraries_are_installed pays attention to are
> +    %
> +    % - global settings, such as libgrade_install_check and
> +    %   mercury_libraries, and
> +    %
> +    % - grade options.
> +    %
> +    % No sensible Mercury.options file will contain options that touch
> +    % the value of any option in either of those categories. It shouldn't
> +    % be able to touch the values of the relevant options in the first
> +    % category, because they don't have any names which would allow them
> +    % to be specified. They *could* touch the values of grade options, but
> +    % the result will be a program whose modules can't be linked together
> +    % due to incompatible grades. (XXX It would be nice if we detected
> +    % such errors in Mercury.options files *without* letting the issue
> +    % through to the linker.)

The rationale for all of that behaviour was that when I originally implemented
libgrade checks, there was no such thing as op modes and when invoking the
compiler to build a single file program, process_compiler_arg_build was the
most convenient place to do the check.

Julien.