[m-rev.] systematic problem with tests/valid* for C#

[Peter Wang]

On Tue, 03 Oct 2023 23:24:23 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> On 2023-10-02 19:24 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:
> >> We have long had .all_cs as a synonym for .cs for this exact reason
> >> (just as we have .all_os as a synonym for .os), but stopped short of
> >> switching the meaning of a target with a .cs suffix to mean a C# source file.
> >>
> >> Would anyone object to making that step now?
> > 
> > No.
> 
> The attached diff does this. It bootchecks in C grades with and without --intermod-opt.
> It bootchecks in Java grade with the same number of test failures as before,
> and it bootchecks in C# with a reduced number of test fails, because the diff
> fixes the old fails caused by the mismatch in the meaning of target cs, as intended.
> 
> >> Specifically, I propose that both mmake and mmc --make should have the
> >> rule that ONLY the suffix  ".all_Xs" means "all .X files".
> > 
> > That's fine.
> 
> I made a start on this, but found a problem that prevented me from going there
> immediately. The problem is explained in the third point added to NEWS.md.
> 
> The diff is for review by anyone. The changes in it should be reflected by changes
> in the user guide, but I would like to see your review comments for NEWS.md
> before I make those changes.
> 
> This is a sort-of unusual diff to review, because the main possible source of error in it
> is not the presence of some change in the attached diff, but the *absence* of a change
> in some file that would be needed to conform the changes that *are* in the diff.
> I would like reviewers to tell me if they know of any places that need such an update.
> 
> For the change to NEWS.md, I am seeking feedback both about its wording, and
> the use of markup. For example, is there some standard way to communicate
> the meta-syntactic nature of `name` and `program`  in the newly added text?
> And I would also like to know whether we should talk about C#-related mmake vars,
> given that we don't support use of mmake for C#.
> 
> Zoltan.

> Get "make cs"/"mmc --make x.cs" build a C# file ...
> 
> ... instead of building a bunch of .c files.
> 
> Our tradition of adding an "s" at the end of a suffix to mean "all of the
> files with the original suffix" had a problem when we added C# as a target
> language. Until then, just as "os" stood for ".o files" when it occurred
> as either a mmake target, mmc --make target, or mmake variable name component.
> "cs" likewise stood for ".c files", but was now also needed to mean ".cs file".
> We coped by keeping "cs" meaning ".c files", and adding "csharp" as a target
> name synonym to mean ".cs file".
> 
> This diff keeps that synonym, but it changes
> 
> - the name needed to refer to ".c files" from "cs" to "all_cs"
> - the name needed to refer to ".o files" from "os" to "all_os"
> - the name needed to refer to ".pic_o files" from "pic_os" to "all_pic_os"
> - the name needed to refer to ".cs files" from "css" to "all_css"
> - the name needed to refer to ".java files" from "javas" to "all_javas"
> - the name needed to refer to ".opt files" from "opts" to "all_opts"
> - the name needed to refer to ".trans_opt files"
>         from "trans_opts" to "all_trans_opts"

(It's too bad that .css is a common file extension as well.
At least the Mercury compiler is unlikely ever to generate CSS files.)

> 
> It would be nice if we could apply this same change to all other similar
> target names and mmake variable name suffixes, such as "ints" and "int3s",
> but so of those names are already in use to mean semantically different
> things. All of the names above that used to have the form "<ext>s" and
> now have the form "all_<ext>s" stood for all the files with extension
> ".<ext>" that are prerequites for building a linked target, i.e. an executable
> or a library. But the mmake variable name suffixes ".all_mihs", ".all_mhs"
> and ".all_int0s" each stand for something subtly different: the names of
> files that *may or may not exist", but which, if they do exist, should be
> deleted by a clean or realclean target.
> 
> To make this breaking change easier to handle by users, this diff does
> not simply redefine the meaning of ".all_int0s". (It does change the meaning
> of the "cs" target, but the fact this will happen at some time has been
> announced ages ago.) Instead, it defines three new mmake var suffixes,
> ".mihs_to_clean", ".mhs_to_clean" and ".int0s_to_clean", which are
> synonyms for ".all_mihs", ".all_mhs" and ".all_int0s" respectively,
> and announces that ".all_mihs", ".all_mhs" and ".all_int0s" are being
> deprecated, and will have the above change of semantics applied to them
> in the future.
> 

Apart from the Mercury system itself, I doubt that users are depending
on the variables in .dep files. AFAICT they are not documented either.

> NEWS.md:
>     Announce the breaking change.
> 
> compiler/make.top_level.m:
>     Stop treating the target "cs" as meaning "build all the .c files
>     for this program".
> 
>     The code of classify_target_2 has long been semidet, but only in a way
>     that was not apparent to the compiler. Change the code to allow the
>     compiler to see its semidet nature while keeping the algorithm the same,
>     except for the change in the paragraph above.
> 
>     This includes keeping e.g. "ints" as meaning "build all the .int/.int2
>     files needed by this program".
> 
> compiler/write_deps_file.m:
>     Stop generating mmake variables with suffixes ".cs", ".os", ".pic_os",
>     ".javas" and ".css". The mmake variables with suffixes ".all_cs",
>     ".all_os", ".all_pic_os", ".all_javas" and ".all_css" already existed.
>     All had the same value as the mmake variable without the "all",
>     with one exception: ".cs". However, in this case, the old (and still
>     current) value of ".all_cs" is what the value of ".cs" *should* have been.
> 
>     Simplify some code.
> 
> The following changes in compiler/*.m are only cosmetic, but they helped me
> rule out possible source of problems with incomplete versions of this diff.
> 
> compiler/file_names.m:
>     Add a version of a fact_table_file_name_return_dirs which does not
>     return directories, since most of its callers don't need that info.
> 
> compiler/make.program_target.m:
>     Clarify code by making variable names more descriptive,
> 
> compiler/make.file_names.m:
> compiler/make.module_target.m:
>     Conform to the changes above,
> 
> scripts/Mmake.rules:
> scripts/Mmake.vars.in:
> 
> browser/Mmakefile:
> compiler/Mmakefile:
> deep_profiler/Mmakefile:
> grade_lib/Mmakefile:
> library/Mmakefile:
> mdbcomp/Mmakefile:
> mfilterjavac/Mmakefile:
> profiler/Mmakefile:
> slice/Mmakefile:
> ssdb/Mmakefile:
>     Rename os to all_os, cs to all_cs, css to all_css, javas to all_javas,
>     and opts to all_opts. (There were no occurrences of trans_opts to rename.)
> 
>     Replace [s as sh command names in actions.
> 
> tools/bootcheck:

You didn't write anything for bootcheck.

> diff --git a/NEWS.md b/NEWS.md
> index 1726dd440..66942f031 100644
> --- a/NEWS.md
> +++ b/NEWS.md
> @@ -58,6 +58,79 @@ Changes that may break compatibility
>  
>  * We have dropped support for the x86 (32-bit) version of Cygwin.
>  
> +* We have changed the meaning of `mmc --make name.cs`.
> +
> +  We have long had two conventions for the names of mmc --make targets
> +  for building all the files of a particular kind needed by a program.
> +  The original convention was
> +
> +  - target `program.cs` builds the .c files of all the modules of `program`
> +  - target `program.os` builds the .o files of all the modules of `program`
> +
> +  A later convention added some synonyms:
> +
> +  - target `program.all_cs` builds the .c files of all the modules of `program`
> +  - target `program.all_os` builds the .o files of all the modules of `program`
> +
> +  We added this second convention when we added C# as a target language,
> +  because, due to the suffix for C# files being `.cs`, targets of the form
> +  `name.cs` became possibly ambiguous, with the new possible meaning being
> +  `build the .cs file for the named module`. At the time, we kept the old
> +  meaning of target `name.cs`, adding target `csharp` to mean `build the
> +  .cs file of the named module`.
> +
> +  We have now changed the meaning of target `name.cs` to mean
> +  `build the .cs file of the named module`. If you want to build
> +  all the .c files of a program, you can specify `program.all_cs`
> +  as the target.
> +

Replace the backticks around `build the .cs file of the named module`,
otherwise it will be rendered in a monospace font.

I think the rationale is too long for an inconsequential change.
The only mention of a "all [type] files" target in the user guide
is to a 'MAIN-MODULE.all_ints' target. It should suffice to say:

  * We have changed the meaning of `mmc --make name.cs`.

    The `mmc --make` target `name.cs` now means "build the .cs file of
    the named module`. To build all the .c files of a program, use
    `program.all_cs` as the target.

> +* For similar reasons, the compiler has stopped putting into
> +  `program.dep` files (where `program` is the name of any program)
> +  the definitions of mmake variables with the following names:
> +  
> +    `program.cs`
> +    `program.os`
> +    `program.pic_os`
> +    `program.javas`
> +    `program.css`
> +
> +  which list the names the names of the `.c`, `.o`, `.pic_o`, `.java` and
> +  `.cs` files of all the modules of the program. All references of these
> +  mmake variables should be replaced to refer to one of the following
> +  mmake variables:
> +
> +    `program.all_cs`
> +    `program.all_os`
> +    `program.all_pic_os`
> +    `program.all_javas`
> +    `program.all_css`
> +
> +  XXX Should we mention the change to .css at all?
> +

I would omit this item entirely, or make it more concise, e.g.

  * Some variables in compiler-generated `.dep` files have been renamed:

    program.cs		-> program.all_cs
    program.os		-> program.all_os
    program.pic_os	-> program.all_pic_os
    ...

> +* We have deprecated the names of three more mmake variables in favor of
> +  newly introduced replacements. These variables are
> +
> +    `program.all_mhs`
> +    `program.all_mihs`
> +    `program.all_int0s`
> +
> +  Their values were defined as the set of files with suffixes `.mh`, `.mih`
> +  and `.int0` that MAY, or MAY NOT, exist for modules of the named program.
> +
> +  We are deprecating these variable names because their meaning conflicts
> +  with the meaning of the other mmake variables mentioned above,
> +  whose values all contain the names of the files that definitely MUST
> +  exist during the building of the program.
> +
> +  Since the only intended purpose of these three variable names
> +  was to specify a set of files that should be deleted during cleanup,
> +  we recommend that all references of these three mmake variables should be
> +  replaced by references to the newly defined mmake variables
> +
> +    `program.mhs_to_clean`
> +    `program.mihs_to_clean`
> +    `program.int0s_to_clean`
> +

Similarly for this item.

The NEWS file for each release is rather long already. My experience
with reading the changelogs of other projects is that the longer it is
and filled with irrelevant (to me) minutia, the more likely I will not
bother reading it. We should avoid expending too many words on each
change, but especially on uninteresting changes.

> diff --git a/scripts/Mmake.rules b/scripts/Mmake.rules
> index d7b78c5a7..d1c77035c 100644
> --- a/scripts/Mmake.rules
> +++ b/scripts/Mmake.rules
> @@ -19,7 +19,7 @@ main_target: $(MAIN_TARGET)
>  
>  #-----------------------------------------------------------------------------#
>  
> -# Beware that the order of suffixes is significant.
> +# Note that the order of suffixes is significant.
>  .SUFFIXES: .m .err \
>  		.int0 .int .int2 .int3 .opt .trans_opt \
>  		.dep .depend .dv .doit .ugly \
> @@ -379,9 +379,20 @@ lib%.install_library: lib%.$A lib%.$(EXT_FOR_SHARED_LIB) \
>  lib%.install_grades:
>  	rm -rf tmp_dir && \
>  	mkdir tmp_dir && \
> -	grade_files="$(foreach ext,$(GRADE_SUBDIR_EXTS),$($*.$(ext)s))" && \
> +	grade_vars_o="$(foreach ext,$(GRADE_SUBDIR_EXTS),$*.$(ext)s)" && \
> +	grade_vars="$(foreach mve,$(GRADE_SUBDIR_MVEXTS),$*.$(mve))" && \
> +	grade_files_o="$(foreach ext,$(GRADE_SUBDIR_EXTS),$($*.$(ext)s))" && \
> +	grade_files="$(foreach mve,$(GRADE_SUBDIR_MVEXTS),$($*.$(mve)))" && \
> +	echo "grade_vars_o" >> /tmp/XYZZY \
> +	echo "$$grade_vars_o" >> /tmp/XYZZY \
> +	echo "grade_vars" >> /tmp/XYZZY \
> +	echo "$$grade_vars" >> /tmp/XYZZY \
> +	echo "grade_files_o" >> /tmp/XYZZY \
> +	echo "$$grade_files_o" >> /tmp/XYZZY \
> +	echo "grade_files" >> /tmp/XYZZY \
> +	echo "$$grade_files" >> /tmp/XYZZY \

I assume you mean to delete these lines.

Peter