Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 8, 2017

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Aug 31, 2017

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
Copy link

dnfclas commented Aug 31, 2017

@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

return false;
}

Log.LogMessage (MessageImportance.Low, $" {nameof (AndroidSdk.JavaSdkPath)}: {javaSdkPath}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Copy link
Member

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 jonathanpeppers changed the title [build] java is no longer required in PATH [jdk] Don't require javac/jar in $PATH Sep 6, 2017
@@ -40,6 +44,12 @@ public override bool Execute ()

Log.LogMessage (MessageImportance.Low, $" {nameof (AndroidSdk.JavaSdkPath)}: {javaSdkPath}");

if (File.Exists (ConfigurationOperatingSystemProps.ItemSpec)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.

@@ -9,5 +9,8 @@
<HostTriplet64 Condition=" '$(HostTriplet64)' == '' ">x86_64-win32</HostTriplet64>
<HostCpuCount Condition=" '$(HostCpuCount)' == '' ">$([System.Environment]::ProcessorCount)</HostCpuCount>
<HostBits Condition=" '$(HostBits)' == '' ">64</HostBits>
<JavaSdkDirectory>@JAVA_HOME@</JavaSdkDirectory>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -67,6 +77,10 @@ public override bool Execute ()
</ItemGroup>
</When>
</Choose>
<PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...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.


namespace Xamarin.Android.BuildTools.PrepTasks
{
public class SetEnvironmentVar : Task
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -24,6 +30,11 @@
</Target>
<Target Name="_CleanProGuard"
AfterTargets="Clean">
<SetEnvironmentVar
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

jonpryor commented Sep 6, 2017

build

<MSBuild Projects="$(MSBuildThisFileDirectory)..\xa-prep-tasks\xa-prep-tasks.csproj" />
<JdkInfo
AndroidSdkPath="$(AndroidSdkPath)"
AndroidNdkPath="$(AndroidNdkPath)"
JavaSdkPath="$(JavaSdkDirectory)"
Output="$(_TopDir)\external\Java.Interop\bin\BuildDebug\JdkInfo.props"
Output ="$(_TopDir)\external\Java.Interop\bin\BuildDebug\JdkInfo.props">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The space before the = is weird.

<MSBuild Projects="$(MSBuildThisFileDirectory)..\xa-prep-tasks\xa-prep-tasks.csproj" />
<JdkInfo
AndroidSdkPath="$(AndroidSdkPath)"
AndroidNdkPath="$(AndroidNdkPath)"
JavaSdkPath="$(JavaSdkDirectory)"
Output="$(_TopDir)\external\Java.Interop\bin\BuildDebug\JdkInfo.props"
Output ="$(_TopDir)\external\Java.Interop\bin\BuildDebug\JdkInfo.props">
<Output TaskParameter="JavaSdkDirectory" PropertyName="_JavaSdkDirectory" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


//NOTE: Unix should accept blank extensions first and Windows last
FileExtensions = new string [(pathExts?.Length ?? 0) + 1];
FileExtensions [isUnix ? 0 : FileExtensions.Length - 1] = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

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
@jonpryor jonpryor merged commit 3dc3737 into dotnet:master Sep 8, 2017
@jonathanpeppers jonathanpeppers deleted the windows-java-path branch September 8, 2017 19:47
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Sep 18, 2017
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Sep 19, 2017
@github-actions github-actions bot locked and limited conversation to collaborators Feb 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants