[m-rev.] diff: fix bug #339: --restricted-command-line broken with recent Java compilers

[Sebastian Godelet]

Hi Julien,

> -----Original Message-----
> From: reviews [mailto:reviews-bounces at lists.mercurylang.org] On Behalf Of
> Sebastian Godelet
> Sent: Thursday, July 16, 2015 4:47 PM
> To: 'Julien Fischer'
> Cc: reviews at lists.mercurylang.org
> Subject: Re: [m-rev.] diff: fix bug #339: --restricted-command-line broken with
> recent Java compilers
> 
> Hi Julien,
> 
> > -----Original Message-----
> > From: Julien Fischer [mailto:jfischer at opturion.com]
> > Sent: Wednesday, July 15, 2015 3:11 PM
> > To: Julien Fischer
> > Cc: Sebastian Godelet; reviews at lists.mercurylang.org
> > Subject: RE: [m-rev.] diff: fix bug #339: --restricted-command-line broken
> with
> > recent Java compilers
> >
> >
> > On Wed, 15 Jul 2015, Julien Fischer wrote:
> >
> > > On Wed, 15 Jul 2015, Sebastian Godelet wrote:
> > >
> > >> The compiler is compiled in a msys environment,
> > >> But I tested it in a normal Windows command shell.
> > >> In my Mercury.config file I have --env-type windows.
> > >> Nevertheless, if I open an msys shell and use:
> > >>
> > >> $ mmc --use-grade-subdirs --env-type msys -s java -m fmt_bug
> > >> cp: cannot stat `Mercury\\java\\i686-pc-
> mingw32\\Mercury\\bin\\fmt_bug':
> > No
> > >> such file or directory
> > >> Made symlink/copy of Mercury\java\i686-pc-
> > mingw32\Mercury\jars\fmt_bug.jar
> > >> mercury_compile.exe: error copying
> > >> `Mercury\java\i686-pc-mingw32\Mercury\bin\fmt_bug' to `fmt_bug',
> can't
> > open
> > >> input file: No such file or directory
> > >>
> > >> The same problem applies.
> > >
> > > Ok, I'll take a look at it.
> >
> > I haven't been able to reproduce this one.
> 
> I think the problem was introduced in commit
> f903c238fb283f7e4047fbf06c62e8c9307cc5c8:
> Fix problems with --use-grade-subdirs with C# and Java backends.
> There Windows batch files where made subdir dependent:
> 
> @@ -1181,7 +1192,7 @@
>  create_launcher_batch_file(Globals, MainModuleName, Pred,
> Succeeded, !IO) :-
>      Extension = ".bat",
>      module_name_to_file_name(Globals, MainModuleName, Extension,
> -        do_not_create_dirs, FileName, !IO),
> +        do_create_dirs, FileName, !IO),
I sorry I quoted the wrong code block, I wanted to quote this part:
diff --git a/compiler/file_names.m b/compiler/file_names.m
index 50b515c..fb8ab1f 100644
--- a/compiler/file_names.m
+++ b/compiler/file_names.m
@@ -524,6 +524,8 @@
     % The `.used' file isn't grade dependent itself, but it contains
     % information collected while compiling a grade-dependent `.c', `il',
     % etc file.
+file_is_arch_or_grade_dependent_2("").
+file_is_arch_or_grade_dependent_2(".bat").

module_name_to_file_name predicate relies on this fact to calculate the
bats/, bins/ etc directory component for the subdir path, hence my supplied patch
now correctly supplies the current extension in both calls. Using my patch Java batch file
creation now works for the Windows cmd environment, I'll now start compiling the patch you
just pushed upstream.

> 
> But in the compile_target_code module in the
> post_link_make_symlink_or_copy predicate
> module_name_to_file_name is called with an empty "" extension,
> thus yielding the "bin" directory instead of the "bats" sub-directory (this is how
> the internals work for module_name_to_file_name afaik).
> So when not compiling with --target-env windows this bug had no effect,
> only the combination of --use(-grade)-subdirs and Windows causes this.
> 
> diff --git a/compiler/compile_target_code.m
> b/compiler/compile_target_code.m
> index 82f4a91..eb2c280 100644
> --- a/compiler/compile_target_code.m
> +++ b/compiler/compile_target_code.m
> @@ -2652,12 +2652,10 @@ post_link_make_symlink_or_copy(ErrorStream,
> LinkTargetType, ModuleName,
>              )
>          then
>              ScriptExt = get_launcher_script_extension(Globals),
> -            module_name_to_file_name(Globals, ModuleName, "",
> -                do_not_create_dirs, OutputScriptName0, !IO),
> -            OutputScriptName = OutputScriptName0 ++ ScriptExt,
> -            module_name_to_file_name(NoSubdirGlobals, ModuleName, "",
> -                do_not_create_dirs, UserDirScriptName0, !IO),
> -            UserDirScriptName = UserDirScriptName0 ++ ScriptExt,
> +            module_name_to_file_name(Globals, ModuleName, ScriptExt,
> +                do_not_create_dirs, OutputScriptName, !IO),
> +            module_name_to_file_name(NoSubdirGlobals, ModuleName,
> ScriptExt,
> +                do_not_create_dirs, UserDirScriptName, !IO),
> 
>              same_timestamp(OutputScriptName, UserDirScriptName,
>                  ScriptSameTimestamp, !IO),
> 
> >
> > >> Again, without the --use-grade-subdirs the correct file gets copid.
> > >> I should mention that in the former case (Windows cmd), the subdir
> > >> Mercury\java\i686-pc-mingw32\Mercury\bats
> > >> Does contain the bat file, just it seems that the `cp' command is not
> > >> looking in that folder.
> > >>
> > >> On a side note, if I use --env-type msys, the java class path still uses
> > >> the c:/ Windows path convention in the
> > >> Shell script and thus cannot be executed (though I think that is a
> > >> different problem).
> > >
> > > I don't think the MSYS use case was really considered when we added
> > > support for launcher scripts.  (The reason is probably that it's
> > > somewhat awkward as it requires Unix conventions in some spots and
> > > Windows ones in others -- also, at the time I don't think the Mercury
> > > compiler had sufficient information about the environment to actually
> > > handle it properly anyway.)  I'll take a look at this one as well
> >
> > The following patch should fix the latter problem, although I don't have
> > a Windows machine at hand to test it at the moment.
> 
> I haven't yet tried your inline patch for this, it takes quite a while to compile the
> compiler on my machine (using Windows).
> On the same machine using Fedora, it builds much faster (x2-x3), my bet is all
> this shell script forking is much slower in msys 32-bit.
> I have not been able to build with any version of msys 64 bit on Windows,
> always failing while configuring Boehm GC not knowing about the target
> architecture.
> 
> >
> > Cheers,
> > Julien.
> >
> > diff --git a/compiler/module_cmds.m b/compiler/module_cmds.m
> > index 72fff72..bc68591 100644
> > --- a/compiler/module_cmds.m
> > +++ b/compiler/module_cmds.m
> > @@ -768,12 +768,16 @@ create_java_shell_script(Globals,
> MainModuleName,
> > Succeeded, !IO) :-
> >       (
> >           ( TargetEnvType = env_type_posix
> >           ; TargetEnvType = env_type_cygwin
> > -        ; TargetEnvType = env_type_msys
> >           ),
> >           create_launcher_shell_script(Globals, MainModuleName,
> >               write_java_shell_script(Globals, MainModuleName, JarFileName),
> >               Succeeded, !IO)
> >       ;
> > +        TargetEnvType = env_type_msys,
> > +        create_launcher_shell_script(Globals, MainModuleName,
> > +            write_java_msys_shell_script(Globals, MainModuleName,
> > JarFileName),
> > +            Succeeded, !IO)
> > +    ;
> >           % XXX should create a .ps1 file on PowerShell.
> >           ( TargetEnvType = env_type_win_cmd
> >           ; TargetEnvType = env_type_powershell
> > @@ -812,6 +816,44 @@ write_java_shell_script(Globals, MainModuleName,
> > JarFileName, Stream, !IO) :-
> >           "exec \"$JAVA\" jmercury.", ClassName, " \"$@\"\n"
> >       ], !IO).
> >
> > +    % For the MSYS version of the Java launcher script, there are a few
> > +    % differences:
> > +    %
> > +    % 1. The value of the CLASSPATH environment variable we construct for
> > +    % the Java interpreter must contain Windows style paths.
> > +    %
> > +    % 2. We use forward slashes as directory separators rather than back
> > +    % slashes since the latter require escaping inside the shell script.
> > +    %
> > +    % 3. The path of the Java interpreter must be a Unix style path as it
> > +    % will be invoked directly from the MSYS shell.
> > +    %
> > +:- pred write_java_msys_shell_script(globals::in, module_name::in,
> > +    file_name::in, io.text_output_stream::in, io::di, io::uo) is det.
> > +
> > +write_java_msys_shell_script(Globals, MainModuleName, JarFileName,
> > Stream, !IO) :-
> > +    get_mercury_std_libs_for_java(Globals, MercuryStdLibs),
> > +    globals.lookup_accumulating_option(Globals, java_classpath,
> > +        UserClasspath),
> > +    % We prepend the .class files' directory and the current CLASSPATH.
> > +    Java_Incl_Dirs = ["\"$DIR/" ++ JarFileName ++ "\""] ++
> > +        MercuryStdLibs ++ ["$CLASSPATH" | UserClasspath],
> > +    ClassPath0 = string.join_list(";", Java_Incl_Dirs),
> > +    ClassPath = string.replace_all(ClassPath0, "\\", "/"),
> > +
> > +    globals.lookup_string_option(Globals, java_interpreter, Java),
> > +    mangle_sym_name_for_java(MainModuleName, module_qual, ".",
> > ClassName),
> > +
> > +    io.write_strings(Stream, [
> > +        "#!/bin/sh\n",
> > +        "DIR=${0%/*}\n",
> > +        "DIR=$( cd \"${DIR}\" && pwd -W )\n",
> > +        "CLASSPATH=", ClassPath, "\n",
> > +        "export CLASSPATH\n",
> > +        "JAVA=${JAVA:-", Java, "}\n",
> > +        "exec \"$JAVA\" jmercury.", ClassName, " \"$@\"\n"
> > +    ], !IO).
> > +
> >   :- pred write_java_batch_file(globals::in, module_name::in, file_name::in,
> >       io.text_output_stream::in, io::di, io::uo) is det.
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >