[jdk] Don't require javac/jar in $PATH

[jonathanpeppers]

Windows machines do not include Java in their path by default, so it is
a better experience to not require it for the build.

Changes to make this happen:

• JavaCPath and JarPath are configured in
  Configuration.OperatingSystem.props
• Usage of javac and jar across the repo use appropriate variables
  now
• generate-os-info needs to set JavaCPath and JarPath
• Created a new <Ant> MSBuild task
• A couple places, Windows needs JAVA_HOME to be set, so we are doing
  this via Environment.SetEnvironmentVariable in a couple MSBuild tasks
• Configuration.OperatingSystem.props is conditional, so it is
  possible to build xa-prep-tasks without it
• Which needs to accept files without extensions last for Windows, PATHEXT should be empty on Unix

[dnfclas]

@jonathanpeppers,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

[jonpryor]

Current convention (e.g. from <BuildApk/>) is that output values should be logged with [Output]:

Log.LogMessage (MessageImportance.Low, $"  {nameof (JavaC)}: {JavaC}");
Log.LogMessage (MessageImportance.Low, $"  {nameof (Jar)}: {Jar}");

I realize that javaSdkPath isn't an output variable, but it can also be inferred from the JavaC path.

[jonpryor]

My biggest problem with this patch is naming-based: the <JavaPaths/> task, as a side-effect, sets the $JAVA_HOME environment variable, but this isn't obvious from the name. It thus results in "oddities" such as in the _AcceptAndroidSdkLicenses target where <JavaPaths/> is called but no outputs are recorded, nor is it obvious why _AcceptAndroidSdkLicenses needs to call <JavaPaths/>. (Does <AcceptAndroidSdkLicenses/> require that $JAVA_HOME be set? If it does, why doesn't it set it itself? etc.)

I'm not convinced that renaming <JavaPaths/> to e.g. <SetJavaPaths/> helps either, because then the <Output/> variables don't really make sense; why do they then exist?

In short, this feels "too magical" to me. :-(

I would instead suggest:

1. Renaming <JavaPaths/> to <GetJava̦Paths/>.
2. <GetJavaPaths/> does not call Environment.SetEnvironmentVariable().
3. Add a new <SetEnvironmentVariable/> task, which does call Environment.SetEnvironmentVariable().
4. Existing tasks which require an environment variable should themselves set it.

[jonpryor]

Alternate suggestion:

Instead of e.g. <Exec Command="&quot;$(_JavaC) ..." />, perhaps Xamarin.Android.Tools.BootstrapTasks/etc. should provide <JavaC/> and <Jar/> tasks, turning this:

<Exec
    Command="&quot;$(_Jar)&quot; cf &quot;$(_MonoAndroidJar)&quot; -C &quot;$(IntermediateOutputPath)jcw\bin&quot; ."
/>

into:

<Jar
    ToolPath="$(JavaSdkDirectory)"
    ToolExe="$(JarToolExe)"
    Options="cf &quot;$(_MonoAndroidJar)&quot; -C &quot;$(IntermediateOutputPath)jcw\bin&quot; ."
/>

[jonathanpeppers]

This is one spot I did something strange:

1. Configuration.OperatingSystem.props has to exist for xa-prep-tasks to build
2. A custom MSBuild task in xa-prep-tasks needs to replace @JAVA_HOME@ in Configuration.OperatingSystem.props, I put this in JdkInfo because it was doing the same thing for Java.Interop

Maybe there is a better way?

[jonpryor]

We already have a task which handles @...@ replacement: <ReplaceFileContents/>. I would suggest reusing that:

<ReplaceFileContents
    SourceFile="Properties\AssemblyInfo.cs.in"
    DestinationFile="$(IntermediateOutputPath)AssemblyInfo.cs"
    Replacements="@JAVA_HOME@=$(_TheJavaSdkPath)"
/>

This simply raises another question: where does $(_TheJavaSdkPath) come from? You should make JdkInfo.JavaSdkPath an [Output] property, which would allow you to set $(_TheJavaSdkPath) and use it in a later <ReplaceFileContents/> call.

[jonpryor]

1. Configuration.OperatingSystem.props has to exist for xa-prep-tasks to build

What we could do is add another conditional import ("Indirections, all the way down!") to Configuration.OperatingSystem.props:

<Import
    Project="$(MSBuildThisFileDirectory)Configuration.Jdk.Props"
    Condition="Exists('$(MSBuildThisFileDirectory)Configuration.Jdk.Props')"
/>

You could then place the $(JavaSdkDirectory), $(JavaCPath), and $(JarPath) values within build-tools\scripts\windows-Configuration.Jdk.Props.in.

[jonpryor]

If we're going to start replacing @...@, please rename the file to have a .in extension, to make it clearer that this is a template file.

[jonpryor]

...with the above <ReplaceFileContents/> approach in mind, you could remove this File.WriteAlltext() block here, and replace with an approach of additional [Output] properties within JdkInfo + a <ReplaceFileContents/> call.

[jonpryor]

We have code completion and (occasional) IDEs. We can use SetEnvironmentVariable. :-)

[jonpryor]

It looks like that the primary reason for <SetEnvironmentVar/> is for ant. Would it be better to have an <Ant/> task which calls ant and also calls Environment.SetEnvironmentVariable()?

<Ant
    Command="-f buildscripts\build.xml clean"
    ToolPath="$(AntToolPath)"
    ToolExe="$(AntToolExe)"
    JavaSdkPath="$(JavaSdkDirectory)"    
/>

[jonpryor]

build

[jonpryor]

The space before the = is weird.

[jonpryor]

This is indented too much; it should be 2 spaces from parent element column, not 4. (Attributes are 4 spaces.)

[jonpryor]

The isUnix check isn't necessary. On Unix, $PATHEXT will be empty, so FileExtensions.Length is only 1.

Instead, just make the null entry last:

FileExtensions  = new string [(pathExts?.Length ?? 0) + 1];
if (pathExts != null) {
    Array.Copy (pathExts, 0, FileExtensions, 0, pathExts.Length);
}
FileExtensions [FileExtensions.Length-1] = null;