[build] Don't use ls(1) to find JDK paths #314

jonpryor · 2018-05-07T20:29:17Z

On some machines, make prepare fails:

    build-tools/scripts/jdk.mk:130: *** missing separator.  Stop.
    make: *** [prepare-external] Error 2

Eventually, we "found" the "cause": This make fragment:

    $(shell ls -dtr $(_DARWIN_JDK_FALLBACK_DIRS) | sort | tail -1)

was dying a terrible horrible no good death:

    _DARWIN_JDK_ROOT=/Library/Java/JavaVirtualMachines/jdk1.8.0_161.jdk
    kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkin/sh: 49m: command not found

Turns Out™, ls(1) should be avoided, as its output is
unsafe (I'm not sure why, and I'm not able to repro the above
failure on my machine, but it's clearly bad).

Why are we using ls(1)? To sort by timestamp, via ls -dtr.
What's the recommended replacement?

If you truly need a list of all the files in a directory in order
by mtime so that you can process them in sequence, switch to perl,
and have your perl program do its own directory opening and sorting

LOL?

Which brings us to the solution: we don't want to use Perl -- we
want something plausibly cross-platform -- so let's use our existing
cross-platform dependency: MSBuild!

Update the <JdkInfo/> task so that in addition to probing a variety
of Windows-specific registry locations, it can now do jdk.mk-style
directory probing, allowing us to find the maximum installed JDK
version which is (optionally) less than $(JI_MAX_JDK).

Additionally, further enhance <JdkInfo/> so that it will also
check $JAVA_HOME and execute the following command:

    java -XshowSettings:properties -version 2>&1 | grep java.home

$JAVA_HOME is preferred, if specified and it fulfills the
requirements of $(JI_MAX_JDK).

Additionally, drastically simplify jdk.mk. Instead of computing the
desired JDK directory on every build, instead generate a new
bin/Build$(CONFIGURATION)/JdkInfo.mk file which contains the JDK
informatino. The make prepare target will generate this file.

This approach simplifes jdk.mk -- trying to maintain it was
beginning to give me a headache, so as part of this we're dropping
support for 32-bit JVMs on macOS, as if anyone uses those -- and also
brings the macOS/Linux build closer-in-spirit to Windows, which was
already using the <JdkInfo/> task.

grendello · 2018-05-07T20:32:46Z

build-tools/scripts/JdkDirectoryFinder.cs

+	[Output]
+	public string PreferredJdkRoot { get; set; }
+
+	static Regex VersionExtractor = new Regex (@"(?<version>[\d]+(\.\d+)+)");


make it faster by passing the 2nd parameter to the constructor: , RegexOptions.Compiled

jonathanpeppers

This is great, I was getting a failure with JDK 10 on VSTS:

2018-05-03T15:03:18.4207610Z   EXEC : error : Source option 5 is no longer supported. Use 6 or later. [/Users/vsts/agent/2.133.3/work/1/s/external/Java.Interop/src/Java.Interop/Java.Interop.csproj]
2018-05-03T15:03:18.4237280Z   /Users/vsts/agent/2.133.3/work/1/s/external/Java.Interop/src/Java.Interop/Java.Interop.targets(27,5): error MSB3073: The command ""/Library/Java/JavaVirtualMachines/jdk-10.0.1.jdk/Contents/Home/bin/javac" -source 1.5 -target 1.6 -d "obj/Debug/ji-classes" java/com/xamarin/java_interop/internal/JavaProxyObject.java java/com/xamarin/java_interop/internal/JavaProxyThrowable.java java/com/xamarin/java_interop/GCUserPeerable.java java/com/xamarin/java_interop/ManagedPeer.java" exited with code 2. [/Users/vsts/agent/2.133.3/work/1/s/external/Java.Interop/src/Java.Interop/Java.Interop.csproj]
2018-05-03T15:03:18.4255300Z 
2018-05-03T15:03:18.4273850Z     15 Warning(s)
2018-05-03T15:03:18.4292280Z     2 Error(s)

It looks to me this should fix it, since it's using System.Version to compare the maximum JDK version.

Let me see about triggering PR builds on VSTS.

jonathanpeppers · 2018-05-07T20:40:02Z

Oh but the macOS build failed.

jonpryor · 2018-05-07T20:56:47Z

@jgold6: Would you be able to test this PR locally to see if it works for you?

jonathanpeppers · 2018-05-08T13:51:54Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkDirectoryFinder.cs

+					dirs.Add (d);
+				}
+			}
+			dirs.Sort (NaturalStringComparer.Default);


Could we instead parse out to System.Version and sort by that? Then we wouldn't need the NaturalStringComparer.

LINQ would be ok here, to do something like:

var dirs = from d in Directory.EnumerateDirectories (JdksRoot) let n = Path.GetFileName (d) let m = VersionExtracter.Match (n) let r = Version.TryParse (m.Groups ["version"].Value, out Version v) where r && (maxVersion == null || v < maxVersion) orderby v desc select d; PreferredJdkRoot = dirs.FirstOrDefault ();

jonathanpeppers

This looks good:

The Windows build worked
I made a quick VSTS build on macOS that just runs make prepare all, and it worked on those build agents with JDK 9 & 10

grendello · 2018-05-23T09:55:36Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+			if (!File.Exists (javacPath)) {
+				Log.LogError ("Could not determine location `javac`.");
+				return false;
+			}


For the sake of staying DRY I'd put the File.Exists in a method (local or not) returning bool and just use

if (!FileExists (jarPath)) return false;

FileExists can construct error message from the path to get the missing binary name

grendello · 2018-05-23T09:57:32Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+				} else {
+					jdkJvmPath  = Path.Combine (java_home, "jre", "bin", "server", "libjvm.so");
+				}
+			}


Wouldn't it be better to determine the file extension somewhere at the start of the task and remove need for this if statement? The difference between macOS and Linux library names would need to stay, of course, but the "complexity" (and repetitiveness) of code would be reduced

The filename is completely different: libjvm.so (Linux) vs libjli.dylib (macOS). An if is needed somewhere.

The only other place for "file extension" is javac vs. java.exe.

I suppose what we could instead do is use a variation on FindExecutableInDirectory() to get the binary that exists in that directory, then we don't need to separate Windows from Unix.

This single filename has to be special-cased, yes, but the others don't need to be.

grendello · 2018-05-23T10:00:52Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+		{
+			if (string.IsNullOrEmpty (MaximumJdkVersion))
+				return null;
+			if (!MaximumJdkVersion.Contains (".")) {


MaximumJdkVersion.IndexOf (".", StringComparison.Ordinal) is going to be faster

The string is likely to be <= 10 characters in length. Does it matter?

Better question is this: which is clear in intent? I think .Contains() is clearer, but if y ou think .IndexOf() is clearer...

Contains is clearer, certainly, but I think we should strike balance between clarity and performance - too many tiny sacrifices of the latter add up. Granted, this particular code will run during make preapre but I think it's a good idea to "promote" code that preforms...

grendello · 2018-05-23T10:06:01Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+				.Where (v => maxVersion == null ? true : v.Version <= maxVersion)
+				.OrderByDescending (v => v.Version)
+				.Select (v => v.Path)
+				.ToList ();


While LINQ looks cool and is pretty concise, it may hurt performance - perhaps it would be better to do it the old fashioned way with loops?

Performance isn't an issue here; Clarity is. (Though it's a gigantic LINQ statement, so perhaps I lost on the clarity angle too.)

This is only used in make prepare. Unless it takes minutes -- and it won't -- performance isn't that important here.

It looks neat but it isn't clear :) You have to follow it carefully to know what's going on (and to infer the types)

I think the LINQ usage here is fine because it is only used as part of our build, not shipped to customers.

The alternative of using multiple loops to append to a List<T>, then sorting at the end--it is probably going to be just as verbose.

grendello · 2018-05-23T10:09:41Z

src/Java.Interop.BootstrapTasks/Java.Interop.BootstrapTasks/JdkInfo.cs

+
+		IEnumerable<string> GetJavaHomePathsFromJava ()
+		{
+			var java  = Path.DirectorySeparatorChar == '/' ? "java" : "java.exe";


Once again, this decision can be made in the task init - we use a finite set of binary names, they could be stored in variables in task init. Would save a small amount of time, but still would be savings.

I would prefer an OS.Platform check here rather than checking the DirectorySeparatorChar.

This assembly doesn't have an OS type yet. Is it worth creating one?

It's certainly more descriptive...

grendello · 2018-05-23T10:10:50Z

@jonp the PR builds just fine on Linux (Ubuntu 18.04)

jonathanpeppers · 2018-05-23T18:46:28Z

@jonpryor whoops something broke?

build-tools\scripts\PrepareWindows.targets (22, 5)
build-tools\scripts\PrepareWindows.targets(22,5): Error MSB4018: The "JdkInfo" task failed unexpectedly.
System.IO.DirectoryNotFoundException: Could not find a part of the path 'C:\Program Files (x86)\Java\jdk1.8.0_131\bin\server'.
at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
at System.IO.FileSystemEnumerableIterator`1.CommonInit()
at System.IO.FileSystemEnumerableIterator`1..ctor(String path, String originalUserPath, String searchPattern, SearchOption searchOption, SearchResultHandler`1 resultHandler, Boolean checkHost)
at System.IO.Directory.EnumerateFiles(String path, String searchPattern, SearchOption searchOption)
at Java.Interop.BootstrapTasks.JdkInfo.FindLibrariesInDirectory(String dir, String libraryName) in E:\A\_work\4\s\src\Java.Interop.BootstrapTasks\Java.Interop.BootstrapTasks\JdkInfo.cs:line 347
at Java.Interop.BootstrapTasks.JdkInfo.Execute() in E:\A\_work\4\s\src\Java.Interop.BootstrapTasks\Java.Interop.BootstrapTasks\JdkInfo.cs:line 70
at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute()
at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext()

grendello · 2018-05-23T19:08:19Z

SpaceZ?

Fixes: dotnet/android#1493 On some machines, `make prepare` fails: build-tools/scripts/jdk.mk:130: *** missing separator. Stop. make: *** [prepare-external] Error 2 Eventually, we "found" the "cause": This make fragment: $(shell ls -dtr $(_DARWIN_JDK_FALLBACK_DIRS) | sort | tail -1) was dying a terrible horrible no good death: _DARWIN_JDK_ROOT=/Library/Java/JavaVirtualMachines/jdk1.8.0_161.jdk kkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkkin/sh: 49m: command not found [Turns Out™][0], **ls**(1) should be *avoided*, as its output is *unsafe* (I'm not sure *why*, and I'm not able to repro the above failure on my machine, but it's *clearly* bad). [0]: http://mywiki.wooledge.org/ParsingLs Why are we using **ls**(1)? To sort by timestamp, via `ls -dtr`. What's the recommended replacement? > If you truly need a list of all the files in a directory in order > by mtime so that you can process them in sequence, switch to perl, > and have your perl program do its own directory opening and sorting LOL? Which brings us to the solution: we don't want to use Perl -- we *want* something plausibly cross-platform -- so let's use our existing cross-platform dependency: MSBuild! Update the `<JdkInfo/>` task so that in addition to probing a variety of Windows-specific registry locations, it can now do `jdk.mk`-style directory probing, allowing us to find the maximum installed JDK version which is (optionally) less than `$(JI_MAX_JDK)`. Additionally, further enhance `<JdkInfo/>` so that it will *also* check `$JAVA_HOME` and execute the following command: java -XshowSettings:properties -version 2>&1 | grep java.home `$JAVA_HOME` is preferred, if specified and it fulfills the requirements of `$(JI_MAX_JDK)`. Additionally, drastically simplify `jdk.mk`. Instead of computing the desired JDK directory *on every build*, instead generate a new `bin/Build$(CONFIGURATION)/JdkInfo.mk` file which contains the JDK informatino. The `make prepare` target will generate this file. This approach simplifes `jdk.mk` -- trying to maintain it was beginning to give me a headache, so as part of this we're dropping support for 32-bit JVMs on macOS, as if anyone uses those -- and also brings the macOS/Linux build closer-in-spirit to Windows, which was already using the `<JdkInfo/>` task.

jonpryor · 2018-05-23T19:32:59Z

@grendello: What does SpaceZ mean?

@jonathanpeppers: The FindLibrariesInDirectory() location was wrong for finding JVM.DLL. It's been fixed.

grendello · 2018-05-23T21:57:07Z

'C:\Program Files (x86)\Java\jdk1.8.0_131\bin\server'.
           ^     ^ SpaceZ!

jonpryor · 2018-05-24T01:34:25Z

@grendello: I see.

In point of fact, that wasn't the problem. The problem was a missing jre: it needed to be ...\ jdk1.8.0_131\jre\bin\server, but was instead ...\jdk1.8.0_131\bin\server.

jonpryor requested review from grendello and jonathanpeppers May 7, 2018 20:29

grendello reviewed May 7, 2018

View reviewed changes

grendello approved these changes May 7, 2018

View reviewed changes

jonathanpeppers reviewed May 7, 2018

View reviewed changes

jonpryor force-pushed the jonp-prepare-jdk-overflow branch from 71c65db to 6fa804b Compare May 7, 2018 20:53

jonpryor force-pushed the jonp-prepare-jdk-overflow branch from 6fa804b to 588bb33 Compare May 7, 2018 20:58

jonathanpeppers approved these changes May 7, 2018

View reviewed changes

jonpryor force-pushed the jonp-prepare-jdk-overflow branch from 588bb33 to 57d8b93 Compare May 8, 2018 13:04

jonathanpeppers reviewed May 8, 2018

View reviewed changes

jonpryor force-pushed the jonp-prepare-jdk-overflow branch 2 times, most recently from ad65288 to c99b860 Compare May 23, 2018 01:03

jonathanpeppers approved these changes May 23, 2018

View reviewed changes

grendello reviewed May 23, 2018

View reviewed changes

jonpryor force-pushed the jonp-prepare-jdk-overflow branch from c99b860 to 3719b76 Compare May 23, 2018 15:04

grendello approved these changes May 23, 2018

View reviewed changes

jonpryor force-pushed the jonp-prepare-jdk-overflow branch from 3719b76 to b6c5439 Compare May 23, 2018 19:32

jonpryor merged commit 4bd9297 into dotnet:master May 24, 2018

github-actions bot locked and limited conversation to collaborators Apr 14, 2024

[build] Don't use ls(1) to find JDK paths #314

[build] Don't use ls(1) to find JDK paths #314

Uh oh!

Conversation

jonpryor commented May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers commented May 7, 2018

Uh oh!

jonpryor commented May 7, 2018

Uh oh!

jonathanpeppers May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grendello commented May 23, 2018

Uh oh!

jonathanpeppers commented May 23, 2018

Uh oh!

grendello commented May 23, 2018

Uh oh!

jonpryor commented May 23, 2018

Uh oh!

grendello commented May 23, 2018

Uh oh!

jonpryor commented May 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jonpryor commented May 7, 2018 •

edited

Loading

jonathanpeppers May 8, 2018 •

edited

Loading

jonathanpeppers left a comment •

edited

Loading