[m-rev.] for post-commit review: document check_libgrades.m

[Julien Fischer]

On Wed, 20 Nov 2024 at 13:39, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
> I intend to resolve the discrepancy noted by the big new XXX
> by making -libgrade-install-check pay attention to the value
> of the MERCURY_STDLIB_DIR environment variable.
> Does anyone object to this?

I have no objections.

> And does anyone disagree with
> the description of the current situation in the new XXX comment?

See below.

> Document check_libgrades.m better.
>
> compiler/check_libgrades.m:
>     Document the operations of this module. Move some code around
>     to provide better hooks to hang the documentation on.
>
>     Add an XXX for an unnecessary and almost certainly unintended
>     discrepancy between the two halves of this module, which originally
>     started out in different modules.
>
>     Add "XXX LEGACY" markers where appropriate.
>
> diff --git a/compiler/check_libgrades.m b/compiler/check_libgrades.m
> index 57354974f..669f9864c 100644
> --- a/compiler/check_libgrades.m
> +++ b/compiler/check_libgrades.m
> @@ -35,6 +35,15 @@
>  :- import_module set.
>
>  %---------------------------------------------------------------------------%
> +%
> +% These predicates answer the question: given the specified location
> +% of the Mercury standard library, in which grades is the Mercury standard
> +% library installed there?

It may be worth extending this to say *why* we need to do this: in order
to set the default value of the libgrades option.

A little background: prior to commit ab20c86 the configure script would
determine the set of library grades to install and write the corresponding
options to set those as the default librades in the Mercury.config file.
After commit ab20c86, this information is determined at runtime (at least
when using mmc or mmc --make; mmake still hardcodes the libgrade set in
Mmake.vars.)

The reason for this is that it allows additional library grades to be
installed after the initial installation without having to update the
Mercury.config file.

> +%
> +% The location may be specified by either the value of the
> +% mercury_standard_library_directory option, or by the value of the
> +% MERCURY_STDLIB_DIR environment variable.
> +%
>
>  :- pred maybe_detect_stdlib_grades(option_table::in, options_variables::in,
>      maybe1(set(string))::out, list(string)::out, io::di, io::uo) is det.
> @@ -51,6 +60,38 @@
>      io::di, io::uo) is det.
>
>  %---------------------------------------------------------------------------%
> +%
> +% This predicate answers two questions, the first of which is related to
> +% but nevertheless quite different from the question above: given the
> +% specified location of the Mercury standard library, is the Mercury standard
> +% library installed there *in the grade given by the value of Globals*?

Globals is the mechanism, the intention is to answer the question: is the
standard library installed in the grade specified _by the user_ via the command
line options.

> +%
> +% The location may be specified *only* by the value of the
> +% mercury_standard_library_directory option.
> +%
> +% The second question is totally unrelated: are the libraries named by the
> +% mercury_libraries option installed in the grade given by Globals in the
> +% directories named by the mercury_library_directories and
> +% init_file_directories options?
> +%
> +
> +% XXX The difference with respect to the specification of the location
> +% of the Mercury standard library means that *if* --mercury-stdlib-dir
> +% is not explicitly set, then it is possible for
> +%
> +% - mmc --output-stdlib-grades to print the set of grades available in
> +%   the directory named by MERCURY_STDLIB_DIR, which may be nonempty, while
> +%
> +% - the check called for by --libgrade-install-check (which is enabled
> +%   by default) reports no errors, even though the grade specified by Globals,
> +%   whatever it is, is not known to be installed in any known directory
> +%   (since there are none).

I suspect the discrepancy is unintentional; most users (and certainly
the users who I added --libgrade-install-check for) are *not* pointing
their Mercury compiler's towards alternative stdlib installations via
the environment variable.

> +% Note that as far as I (zs) can tell, mmc does not actually need
> +% to know anything about installed grades;

Correct, that is a consequence of commit ab20c86. The only part of the
system that has hardcoded knowledge of which stdlib grades are installed
is mmake (via Mmake.vars).

> +% that info is needed only by
> +% the tools (e.g. ml) that process the *output* of mmc. Therefore neither
> +% of the two apparently-conflicting approaches above is intrinsically better
> +% for mmc itself.

...

> @@ -298,6 +341,44 @@ report_detected_libgrade(Stream, Grade, !IO) :-
>
>  %---------------------------------------------------------------------------%
>
> +    % This mutable is a cache that records whether the job of
> +    % maybe_check_libraries_are_installed has already been done
> +    % for a given set of parameters.
> +    %
> +    % I (zs) think that this cache is useful mostly when mmc --make
> +    % needs to compile more than one module. Those compilations are
> +    % almost always done with the same parameters, and so
> +    %
> +    % - there is no point in repeating the test, since the same inputs
> +    %   are guaranteed to yield the same outputs, while
> +    %
> +    % - there is point in avoiding tests when this can be done safely,
> +    %   because the file operations does by the test are expensive.

s/does/done/

...

> -:- pred check_library_is_installed(globals::in, string::in,
> +%---------------------------------------------------------------------------%
> +
> +    % Part 2 of the maybe_check_libraries_are_installed test:
> +    % are libraries named in the --mercury-libraries option installed?

Either the --mercury-library option, or mercury_libraries option.
(There's no such thing as --mercury-libraries.)

That looks fine otherwise.

Julien.