-
Notifications
You must be signed in to change notification settings - Fork 537
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
Conversation
@jonathanpeppers, |
return false; | ||
} | ||
|
||
Log.LogMessage (MessageImportance.Low, $" {nameof (AndroidSdk.JavaSdkPath)}: {javaSdkPath}"); |
There was a problem hiding this comment.
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.
My biggest problem with this patch is naming-based: the I'm not convinced that renaming In short, this feels "too magical" to me. :-( I would instead suggest:
|
Alternate suggestion: Instead of e.g. <Exec
Command=""$(_Jar)" cf "$(_MonoAndroidJar)" -C "$(IntermediateOutputPath)jcw\bin" ."
/> into: <Jar
ToolPath="$(JavaSdkDirectory)"
ToolExe="$(JarToolExe)"
Options="cf "$(_MonoAndroidJar)" -C "$(IntermediateOutputPath)jcw\bin" ."
/> |
215d396
to
10bd198
Compare
javac
/jar
in $PATH
@@ -40,6 +44,12 @@ public override bool Execute () | |||
|
|||
Log.LogMessage (MessageImportance.Low, $" {nameof (AndroidSdk.JavaSdkPath)}: {javaSdkPath}"); | |||
|
|||
if (File.Exists (ConfigurationOperatingSystemProps.ItemSpec)) { |
There was a problem hiding this comment.
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:
Configuration.OperatingSystem.props
has to exist for xa-prep-tasks to build- A custom MSBuild task in xa-prep-tasks needs to replace
@JAVA_HOME@
inConfiguration.OperatingSystem.props
, I put this inJdkInfo
because it was doing the same thing for Java.Interop
Maybe there is a better way?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
. :-)
src/proguard/proguard.targets
Outdated
@@ -24,6 +30,11 @@ | |||
</Target> | |||
<Target Name="_CleanProGuard" | |||
AfterTargets="Clean"> | |||
<SetEnvironmentVar |
There was a problem hiding this comment.
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)"
/>
build |
10bd198
to
29fa8da
Compare
<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"> |
There was a problem hiding this comment.
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" /> |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
29fa8da
to
9192828
Compare
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
andJarPath
are configured inConfiguration.OperatingSystem.props
javac
andjar
across the repo use appropriate variablesnow
generate-os-info
needs to setJavaCPath
andJarPath
<Ant>
MSBuild taskJAVA_HOME
to be set, so we are doingthis via
Environment.SetEnvironmentVariable
in a couple MSBuild tasksConfiguration.OperatingSystem.props
is conditional, so it ispossible to build xa-prep-tasks without it
Which
needs to accept files without extensions last for Windows,PATHEXT
should be empty on Unix