[m-rev.] for post-commit review: implement PROPOSED searches for .a, .init, .jar and .dll files

[Julien Fischer]

On Fri, 13 Dec 2024 at 22:18, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
>
> On Thu, 12 Dec 2024 11:31:10 +1100 (AEDT), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > On Wed, 4 Dec 2024 23:37:50 +1100, Julien Fischer <jfischer at opturion.com> wrote:
> >
> > > On Wed, 4 Dec 2024 at 02:58, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
> > >
> > > > On Tue, 3 Dec 2024 19:10:19 +1100, Julien Fischer <jfischer at opturion.com> wrote:
> > > > > > I think that the conceptually cleanest solution of this bug is to make
> > > > > > the --libgrade option serve only the second purpose, eliminating (1)
> > > > > > from the preference order entirely. We would compute the set of installed
> > > > > > stdlib grades in handle_options.m just after we compute chosen_stdlib_dir,
> > > > > > and record the result in a new slot in the globals structure.
> > > > >
> > > > > That seems a reasonable suggestion.
> > > >
> > > > Then I will implement it, including the include/exclude-component bits.
> >
> > The attached diff does this; it is for review by anyone. The diff
> > adds a new, non-misleading name for the option now named --detect-libgrades.
>
> In the absence of a review by then, I intend to commit this diff
> tomorrow around noon.

> Get --output-stdlib-grades working again.
>
> The previous change to how detecting stdlib grade works exposed
> an ordering problem. The base of the problem was that
>
> - we had to detect the grades in which the standard library is installed
>   *first*, so that
>
> - we could convert this set to a sequence of compiler-generated
>   --libgrade options, so they could then be updated by
>
> - later, user-specified --libgrade options.
>
> All this happened before we constructed the first globals structure. The
> problem is that the code in handle_options.m that finds out what stdlib
> location we want to use *needs* the first globals structure. The result
> of this computation worked correctly only if the initial, effectively guessed
> stdlib location we used in step 1 above happened to be the final chosen
> stdlib location.
>
> The root cause of the problem was the use of --libgrade options
> for two related but nevertheless conceptually separate things:
>
> - the set grades in which the Mercury standard libray is installed
>   in its chosen location, and
>
> - the set of grade in which the user wants to install his/her libraries.
>
> The latter defaults to the former, but user options can tell the compiler
> to deviate from that default.
>
> The main part of the fix is to use --libgrade options only for the
> second notion (which it is already named after), and to store the
> set of stdlib grades not in option values at all, but in a new field
> in the globals structure. This field can then be filled with meaningful data
> *after* we have chosen the location of the Mercury stdlib.
>
> The main complication with using this approach is that mmc --make
> currently rebuilds the globals structure from scratch before building
> each target. (I am not sure whether this is necessary, but it does seem
> a nontrivial thing to change.) Doing this naively would cause us to detect
> the stdlib's grades once per target, which is obviously wasteful.
> The old code handled this by adding a --libgrade option for each stdlib grade
> to the list of options we build each target with; the new code passes
> the value in the new globals field instead.
>
> compiler/options.m:
> doc/user_guide.texi:
>     Change the --detect-libgrades option name to --detect-stdlib-grades.
>     Unlike the old name, the new name is not misleading. Leave the old
>     name as a synonym, for backward compatibility.
>     XXX Do we need backward compatibility in such a rarely-used option?

No, but it will need to remain for now. IIRC, the main use of the
--no-detect-libgrades option is in the configure script's "is the bootstrap
mmc up-to-date?" check.  (Once the new option has bootstrapped, the old synonym
can be deleted.)

...

> diff --git a/compiler/compute_grade.m b/compiler/compute_grade.m
> index 44270560d..7083a36ed 100644
> --- a/compiler/compute_grade.m
> +++ b/compiler/compute_grade.m
> @@ -48,7 +48,9 @@
>      % check that all the grade components are valid and that there are
>      % no duplicate grade components.
>      %
> -:- pred postprocess_options_libgrades(globals::in, globals::out,
> +    % XXX Actuall, I (zs) see no sanity checks here at all.
> +    %
> +:- pred handle_libgrade_component_incl_excl(globals::in, globals::out,
>      list(error_spec)::in, list(error_spec)::out) is det.
>
>      % The inverse of compute_grade: given a grade, set the appropriate options.
> @@ -242,7 +244,7 @@ check_grade_component_compatibility(Globals, Target, GC_Method, !Specs) :-
>
>  %---------------------------------------------------------------------------%
>
> -postprocess_options_libgrades(!Globals, !Specs) :-
> +handle_libgrade_component_incl_excl(!Globals, !Specs) :-
>      globals.lookup_accumulating_option(!.Globals, libgrades_include_components,
>          IncludeComponentStrs),
>      globals.lookup_accumulating_option(!.Globals, libgrades_exclude_components,
> @@ -265,7 +267,7 @@ postprocess_options_libgrades(!Globals, !Specs) :-
>      % string_to_grade_component(OptionStr, Comp, !Comps, !Specs):
>      %
>      % If `Comp' is a string that represents a valid grade component
> -    % then add it to !Comps. If it is not then emit an error message.
> +    % then add it to !Comps. If it is not, then emit an error message.
>      % `OptionStr' should be the name of the command line option for
>      % which the error is to be reported.
>      %
> @@ -274,6 +276,13 @@ postprocess_options_libgrades(!Globals, !Specs) :-
>      list(error_spec)::in, list(error_spec)::out) is det.
>
>  string_to_grade_component(FilterDesc, Comp, !Comps, !Specs) :-
> +    % XXX Including FilterDesc in the error messages seems odd, because
> +    % the resulting text can be read as implying that
> +    %
> +    % - a string is NOT a valid grade component for e.g. inclusion, but
> +    % - the same string IS a valid grade component for e.g. exclusion,
> +    %
> +    % even though of course that implication is false.
>      ( if grade_component_table(Comp, _, _, _, _) then
>          !:Comps = [Comp | !.Comps]
>      else if Comp = "erlang" then

Unrelated: at this point I think we can probably safely delete any remaining
references to the erlang grade.

The rest of the diff looks fine.

Julien.