[java-interop] Windows build system support #816

jonpryor · 2021-03-11T22:13:33Z

Context: 5c756b1
Context: 08ff4db

With the cmake-based build system knowledge behind 5c756b1 in
mind, update src/java-interop to also use a cmake-based build
system, then add support so it can build on Windows.

This allows the Java.Interop-Tests.dll unit tests to also be run
on Windows, under both .NET Core and .NET Framework.

Note that this requires updating java_interop_lib_load() to also
include LOAD_LIBRARY_SEARCH_SYSTEM32:

LOAD_LIBRARY_SEARCH_SYSTEM32: %windows%\system32 is searched
for the DLL and its dependencies.

This specifically is needed so that JVM.DLL dependencies such as
KERNEL32.DLL can be resolved, allowing JVM.DLL to be loaded.

This allows .NET Core on Windows to run Java.Interop-Tests.dll!

> dotnet test bin\TestDebug-netcoreapp3.1\Java.Interop-Tests.dll
…
A total of 1 test files matched the specified pattern.
  Skipped Dispose_ClearsLocalReferences [11 ms]

Passed!  - Failed:     0, Passed:   617, Skipped:     1, Total:   618, Duration: 2 s - Java.Interop-Tests.dll (netcoreapp3.1)

Context: 5c756b1 Context: 08ff4db With the `cmake`-based build system knowledge behind 5c756b1 in mind, update `src/java-interop` to *also* use a `cmake`-based build system, then add support so it can build on Windows. This allows the `Java.Interop-Tests.dll` unit tests to *also* be run on Windows, under both .NET Core and .NET Framework. Note that this requires updating `java_interop_lib_load()` to also include [`LOAD_LIBRARY_SEARCH_SYSTEM32`][0]: * `LOAD_LIBRARY_SEARCH_SYSTEM32`: `%windows%\system32` is searched for the DLL and its dependencies. This specifically is needed so that `JVM.DLL` dependencies such as `KERNEL32.DLL` can be resolved, allowing `JVM.DLL` to be loaded. This allows .NET Core on Windows to run `Java.Interop-Tests.dll`! > dotnet test bin\TestDebug-netcoreapp3.1\Java.Interop-Tests.dll … A total of 1 test files matched the specified pattern. Skipped Dispose_ClearsLocalReferences [11 ms] Passed! - Failed: 0, Passed: 617, Skipped: 1, Total: 618, Duration: 2 s - Java.Interop-Tests.dll (netcoreapp3.1) [0]: https://docs.microsoft.com/windows/win32/api/libloaderapi/nf-libloaderapi-loadlibraryexw

jonpryor · 2021-03-11T22:16:30Z

src/java-interop/java-interop-logger.cc

 #include "java-interop-logger.h"

 #define LOG_VA_ARGS(_kind_,_category_,_format_)                                 \
 	do {                                                                    \
 		const char*     kind      = (_kind_);                           \
 		LogCategories   category  = (_category_);                       \
 		va_list args;                                                   \
-		va_start (args, (_format_));                                    \
+		va_start (args, _format_);                                      \


"Weird", yet required; without it, the MSVC compiler complains, because it considers (_format_) to be a C++ reference type, via decltype(…). This was the easiest way to bring sanity.

Will it work?

Commit 6ede47a asked: > Will it work? Lol, no. No it does not: SetUp failed for test fixture Java.InteropTests.TestTypeTests System.TypeInitializationException : The type initializer for 'Java.InteropTests.JavaVMFixture' threw an exception. ----> System.BadImageFormatException : An attempt was made to load a program with an incorrect format. (Exception from HRESULT: 0x8007000B) at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor) at System.Reflection.RuntimeConstructorInfo.Invoke(BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) at NUnit.Framework.Internal.Reflect.Construct(Type type, Object[] arguments) at NUnit.Framework.Internal.TypeWrapper.Construct(Object[] args) at NUnit.Framework.Internal.Commands.ConstructFixtureCommand.<.ctor>b__0_0(TestExecutionContext context) at NUnit.Framework.Internal.Commands.BeforeTestCommand.Execute(TestExecutionContext context) at NUnit.Framework.Internal.Execution.CompositeWorkItem.PerformOneTimeSetUp() --BadImageFormatException at Java.Interop.NativeMethods.java_interop_jvm_load_with_error_message(String path, IntPtr& message) at Java.Interop.JreRuntime.CreateJreVM(JreRuntimeOptions builder) in D:\a\1\s\src\Java.Runtime.Environment\Java.Interop\JreRuntime.cs:line 97 at Java.InteropTests.TestJVM..ctor(String[] jars, Dictionary`2 typeMappings) in D:\a\1\s\tests\TestJVM\TestJVM.cs:line 61 at Java.InteropTests.JavaVMFixture.CreateJavaVM() in D:\a\1\s\tests\Java.Interop-Tests\Java.Interop\JavaVMFixture.cs:line 15 at Java.InteropTests.JavaVMFixture..cctor() in D:\a\1\s\tests\Java.Interop-Tests\Java.Interop\JavaVMFixture.Partial.cs:line 12 …which is rather odd, actually; *why* `BadImageFormatException`? A plausible reason is that the "bitness" doesn't match, e.g. `dotnet` is 32-bit while `java-interop.dll` is 64-bit (or vice versa). The easiest way to tell is to download the built artifacts to see what they actually contain. Add a `PublishPipelineArtifact@` invocation so that we *can* download.

jonpryor · 2021-03-12T02:03:01Z

What's "interesting" is that if I download the new bin-dotnet artifacts, I can run them on my Windows machine:

> dotnet test TestRelease-netcoreapp3.1\Java.Interop-Tests.dll
…
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
  Skipped Dispose_ClearsLocalReferences [11 ms]

Passed!  - Failed:     0, Passed:   617, Skipped:     1, Total:   618, Duration: 2 s - Java.Interop-Tests.dll (netcoreapp3.1)

Furthermore, java-interop.dll is a 64-bit binary…at least according to file(1) on WSL:

% file TestRelease-netcoreapp3.1/java-interop.dll
TestRelease-netcoreapp3.1/java-interop.dll: PE32+ executable (DLL) (GUI) x86-64, for MS Windows

More carefully reading the execution logs, it's running C:\hostedtoolcache\windows\dotnet\dotnet.exe. I thus have to guess that this is a 32-bit app, and thus can't load a 64-bit library.

Which raises the next question: how do I force use of a 64-bit dotnet binary?

Also, is `dumpbin` in %PATH% on this machine?

In c14ebb6 + 6babd68, we learned that `C:\hostedtoolcache\windows\dotnet\dotnet.exe` *is* in fact a 64-bit process. Thus, a verification check: is the conjecture that we're in a 32-bit process correct? Add some debug writelines to find out!

Context: https://discord.com/channels/732297728826277939/732297952676020316/819978878948868166 Context: microsoft/vstest#2571 Looks like an updated version of `dotnet test` *might* fix my "test architecture" conundrum, so let's try using .NET 5.0.103!

jonpryor · 2021-03-12T17:48:41Z

For future reference, in order for Java.Interop-Tests.dll to work on Windows:

> dotnet test bin\TestDebug-netcoreapp3.1\Java.Interop-Tests.

We need to use .NET 5.0.103. Earlier versions of .NET Core run our unit tests in a 32-bit process, which insta-fails, as we (currently) only build java-interop.dll for 32-bit.

Alternatively, we need to figure out how to build java-inteorp.dll for both 32-bit and 64-bit Windows…. Which we'd need to do in order to run tests under .NET Framework.

A problem since 6ede47a is that 32-bit and 64-bit execution environments (1) are A Thing, and (2) won't be going away. Thus building `java-interop.dll` for only one architecture isn't a scalable solution. "Embrace" this, and rework the `src/java-interop` build system to run `cmake` *separately* for 32-bit and 64-bit libraries.

This reverts commit 08348d3. We know the answer; we were!

This reverts commit c14ebb6.

This reverts commit 6babd68. `dotnet.exe` was a 64-bit binary.

This reverts commit 0969a84. Setting TargetPlatform didn't help.

Commit f4d1109 changed cmake targets. Fixity fix.

It built locally! :-/

…why isn't it present?! "For good measure", append `$(PrepareNativeToolchain)` with `&& echo ABI && `, not just `&&`. (Note extra command *plus* trailing space.)

Commit 9468871 showed that having every `RunCmake.proj` call-site include `&&` is cumbersome & annoying; move that into `RunCmake.proj`. Add a new `runNativeDotnetTests` parameter to `core-tests.yaml`, so that we can run `Java.Interop-Tests.dll` on Windows+.NET Core.

jonpryor · 2021-03-14T14:14:52Z

Something to mention for runNativeDotnetTests is that the JVM we find is 64-bit, while .NET Framework is defaulting to a 32-bit process, which means the JVM can't be loaded. (Same as before!)

It's still desirable to show that CI can run dotnet test Java.Interop-Tests.dll on Windows, and runNativeDotnetTests will allow selective execution…

This reverts commit 300f1c4. The "Why" is that it was NativeTiming that had the issue. Doh.

This reverts commit e134baf. Things are now working; we don't need to publish the `bin` dir.

All hail target batching? ALSO: If using `@(None)`/etc. into a sub-directory, as `src/java-interop` does for e.g. `$(OutputPath)win-x64\java-interop.dll`, you MUST: 1. Use `%(Link) to have the *relative* sub-dir + filename, AND 2. You MUST use `\` for the sub-dir! With these two incantations, `<Copy/>` will *ignore* any errors about the source files not existing, IFF the source & destination file paths are IDENTICAL. (String-wise identical!) Thus if you use `\`, you're 'fine' -- strings compare -- but if you use `/`, you're in for pain and suffering.

grendello · 2021-03-16T21:06:51Z

build-tools/scripts/NativeToolchain.targets

+      <PrepareNativeToolchain>call "$(_Vcvarsall)" </PrepareNativeToolchain>
+    </PropertyGroup>
+    <PropertyGroup>
+      <CmakeGenerator Condition=" '$(OS)' != 'Windows_NT' ">-G "Unix Makefiles"</CmakeGenerator>


If you can use Ninja, it tends to be faster

Alas, attempting to use ninja broke on the macOS CI. :-(

src/java-interop/CMakeLists.txt

jpobst · 2021-03-16T21:17:15Z

I realize it's probably not caused by this PR, per-se, but this adds 48 C/C++ warnings to the build.

Is it possible to fix or suppress them?

Otherwise, LGTM.

jonathanpeppers · 2021-03-16T21:13:13Z

build-tools/scripts/RunCmake.proj

+      <_SourceDir>$(CmakeSourceDir.Replace('%5c', '/'))</_SourceDir>
+      <_BuildDir>$(CmakeBuildDir.Replace('%5c', '/'))</_BuildDir>
+      <_ExtraArgs>$(CmakeExtraArgs.Replace('%5c', '/'))</_ExtraArgs>


Since this is already working, this is fine probably fine. But was the main issue here the <Exec/> task?

Maybe it would work to make a small <UnixExec/> task that subclasses <Exec/> and replaces all the \ with / in Command? Then base.Execute() would succeed?

@jonathanpeppers : the problem isn't "unix", the problem is double quotes. :-)

"General guidelines", at least on Unix, is to quote anything which could possibly have a space…such as path names! The problem is when:

The "path name" is a directory name, and

The directory name ends with \.

The result is that you escape the quote!

$(_Prepare) $(CmakePath) $(CmakeGenerator) -S "$(_SourceDir)"

Here, $(_SourceDir) will be e.g. $(MSBuildThisFileDirectory), which aways ends with \. If we didn't "fix" $(_SourceDir), we'd have:

make -S "C:\some\directory\" …

The attempt to "sanely" quote the directory name results in an invalid command line.

Does this mean that s,\,/,g is "best"? No; we could instead do:

<_SourceDir>$(CmakeSourceDir.TrimEnd('%5c'))</_SourceDir>

But! If a value can be "part of" the Cmake files, e.g. Cmake variables, those values must use /, not \, because Cmake otherwise attempts to treat \ as an escape character. Thus, $(CmakeExtraArgs) must replace \ with /, as it contains Cmake property overrides that contain paths.

In neither case does a <UnixExec/> class help, unless a "command global" s,\,/,g is desired. Note however that we don't "fixup" $(PrepareNativeToolchain) or $(CmakePath).

jonathanpeppers · 2021-03-16T21:17:17Z

src/java-interop/Directory.Build.targets

+    <_JavaInteropLibName Condition=" '$(OS)' != 'Windows_NT' And Exists ('/Library/Frameworks/') ">libjava-interop.dylib</_JavaInteropLibName>
+    <_JavaInteropLibName Condition=" '$(OS)' != 'Windows_NT' And !Exists ('/Library/Frameworks/') ">libjava-interop.so</_JavaInteropLibName>
+    <_JavaInteropLibName Condition=" '$(OS)' == 'Windows_NT' ">java-interop.dll</_JavaInteropLibName>


We have a lot of existing code using $(OS), but I think the proper check is to use these:

$([MSBuild]::IsOSPlatform ('osx')) $([MSBuild]::IsOSPlatform ('windows')) $([MSBuild]::IsOSPlatform ('linux'))

I've seen DevCom issues where someone had %OS% set to Windows on their Dell PC!

jonathanpeppers · 2021-03-16T21:18:08Z

tests/NativeTiming/Directory.Build.targets

+  <ItemGroup Condition=" '$(OS)' == 'Windows_NT' ">
+    <_NativeTimingLib Include="CMakeLists.txt">
+      <Arch>x86_amd64</Arch>
+      <Dir>win-x64\</Dir>
+    </_NativeTimingLib>
+    <_NativeTimingLib Include="CMakeLists.txt">
+      <Arch>x86</Arch>
+      <Dir>win-x86\</Dir>
+    </_NativeTimingLib>
+  </ItemGroup>
+
+  <ItemGroup Condition=" '$(OS)' != 'Windows_NT' ">


More $(OS) checks.

jonpryor · 2021-03-16T21:26:06Z

Meanwhile:

So! Suppose you want an "output file" which is in a subdirectory of $(OutputPath), e.g. -- as done here -- $(OutputPath)win-x64\java-interop.dll.

If you try the "obvious" thing:

<ItemGroup>
  <Content Include="$(OutputPath)win-x64\java-interop.dll" />
</ItemGroup>

This fails:

error MSB3030: Could not copy the file…

It fails before any tarts which could create the specified file have a chance to execute.

To make it work, two things must be done:

Use %(Link), with the relative directory (not including $(OutputPath)!) and filename.
Though Shalt Use \ Everywhere!

Thus:

<!-- Good -->
<ItemGroup>
  <Content Include="$(OutputPath)win-x64\java-interop.dll">
    <Link>win-x64\java-interop.dll</Link>
  </Content>
</ItemGroup>

This works. It works because:

The <AppendTargetPath/> task uses %(Link) as an override when setting %(TargetPath).
The _CopyOutOfDateSourceItemsToOutputDirectory target uses $(OutDir)%(TargetPath) for the Copy.DestinationFiles property.
The <Copy/> ignores errors when SourceFiles[I] and DestinationFiles[i] are identical.

This is why \ is required on Windows; if / is used in the %(Link) path, e.g.

<!-- Bad! -->
<ItemGroup>
  <Content Include="$(OutputPath)win-x64/java-interop.dll">
    <Link>win-x64/java-interop.dll</Link>
  </Content>
</ItemGroup>

it will fail, because <Copy/> believes that SourceFiles and DestinationFiles to differ:

Building target "_CopyOutOfDateSourceItemsToOutputDirectory" completely.
…
Task "Copy"
  Task Parameter:OverwriteReadOnlyFiles=False
  Task Parameter:
      DestinationFiles=
          C:\…\bin\Debug\win-x64/java-interop.dll
          C:\…\bin\Debug\win-x86/java-interop.dll
  Task Parameter:
      SourceFiles=
          C:\…\bin\Debug\win-x64\java-interop.dll
          C:\…\bin\Debug\win-x86\java-interop.dll
error MSB3030: Could not copy the file "C:\…\bin\Debug\win-x86\java-interop.dll" because it was not found.

(I guess something is normalizing @(_SourceItemsToCopyToOutputDirectory), which is why SourceFiles contains \, even though the original @(Content) used /.)

TL;DR: Avoid subdirectories within @(None)/@(Content)/etc. If you can't avoid them, ensure that %(Identity) and %(Link) use \ and not / when building on Windows.

This reverts commit eb4eab1. Context: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4563922&view=logs&jobId=2df166ff-203c-5833-cfba-e6fc8650c634&j=2df166ff-203c-5833-cfba-e6fc8650c634&t=8b9d5ac8-ce15-5110-c9f4-439f0fc78aa2 Looks like the macOS CI bot doesn't have `ninja` in `$PATH`: _RunCmake: cmake -G "Ninja" -S "/Users/runner/work/1/s/src/java-interop/" -B "/Users/runner/work/1/s/src/java-interop/obj/Release-netcoreapp3.1/" "-DJDK_INCLUDE_LIST=/Library/Java/JavaVirtualMachines/adoptopenjdk-8.jdk/Contents/Home/include;/Library/Java/JavaVirtualMachines/adoptopenjdk-8.jdk/Contents/Home/include/darwin" "-DJNI_C_PATH=/Users/runner/work/1/s/src/java-interop/obj/Release-netcoreapp3.1/jni.c" && cmake --build "/Users/runner/work/1/s/src/java-interop/obj/Release-netcoreapp3.1/" -v -- Configuring incomplete, errors occurred! See also "/Users/runner/work/1/s/src/java-interop/obj/Release-netcoreapp3.1/CMakeFiles/CMakeOutput.log". EXEC : CMake warning : CMake was unable to find a build program corresponding to "Ninja". CMAKE_MAKE_PROGRAM is not set. You probably need to select a different build tool. [/Users/runner/work/1/s/build-tools/scripts/RunCmake.proj] Revert the attempt to use Ninja.

Fix C4996 wrt `strdup`, by just `#define`ing it to `_strdup`. Fix C4700: java-interop-logger.cc(88,0): Warning C4700: uninitialized local variable 'category' used This was due to macro expansion, and the Microsoft C++ compiler didn't like a variable referring to a variable in a parent scope: log_error (LogCategories category, const char *format, ...) { do { LogCategories category = category; // C4700 ... } } Fix this by renaming the variable declared in the macro. Fix `Directory.Build.targets` so that changes to `*.cc` files cause things to be rebuilt.

Context: dotnet/msbuild#539 (comment) Sometimes the `%OS%` environment variable is set, which overrides the normally expected `$(OS)` MSBuild property. Instead of using `$(OS)`, use `$([MSBuild]::IsOSPlatform(name))`.

jonpryor commented Mar 11, 2021

View reviewed changes

jonpryor added 2 commits March 11, 2021 17:31

Enable tests on Windows+.NET Core!

6ede47a

Will it work?

jonpryor added 5 commits March 12, 2021 08:17

Explicitly set RunConfiguration.TargetPlatform=x64 ?

0969a84

What *is* dotnet.exe? 32-bit or 64-bit?

6babd68

Also, is `dumpbin` in %PATH% on this machine?

Take 2?

c14ebb6

*Are* we in a 32-bit process?

08348d3

In c14ebb6 + 6babd68, we learned that `C:\hostedtoolcache\windows\dotnet\dotnet.exe` *is* in fact a 64-bit process. Thus, a verification check: is the conjecture that we're in a 32-bit process correct? Add some debug writelines to find out!

Install .NET 5.0.103.

e4be4ed

Context: https://discord.com/channels/732297728826277939/732297952676020316/819978878948868166 Context: microsoft/vstest#2571 Looks like an updated version of `dotnet test` *might* fix my "test architecture" conundrum, so let's try using .NET 5.0.103!

jonpryor added 10 commits March 12, 2021 19:27

Revert "*Are* we in a 32-bit process?"

ea91ce2

This reverts commit 08348d3. We know the answer; we were!

Revert "Take 2?"

c45db89

This reverts commit c14ebb6.

Revert "What *is* dotnet.exe? 32-bit or 64-bit?"

53531d8

This reverts commit 6babd68. `dotnet.exe` was a 64-bit binary.

Revert "Explicitly set RunConfiguration.TargetPlatform=x64 ?"

d1eadab

This reverts commit 0969a84. Setting TargetPlatform didn't help.

Fix NativeTiming build

94c694a

Commit f4d1109 changed cmake targets. Fixity fix.

Now it doesn't build on Windows. Why?

300f1c4

It built locally! :-/

Build is failing because the && isn't present.

e17afe3

…why isn't it present?! "For good measure", append `$(PrepareNativeToolchain)` with `&& echo ABI && `, not just `&&`. (Note extra command *plus* trailing space.)

Doh; NativeTiming was missing &&

9468871

Cleanup!

e30a583

Commit 9468871 showed that having every `RunCmake.proj` call-site include `&&` is cumbersome & annoying; move that into `RunCmake.proj`. Add a new `runNativeDotnetTests` parameter to `core-tests.yaml`, so that we can run `Java.Interop-Tests.dll` on Windows+.NET Core.

jonpryor added 3 commits March 14, 2021 10:16

Revert "Now it doesn't build on Windows. Why?"

958372c

This reverts commit 300f1c4. The "Why" is that it was NativeTiming that had the issue. Doh.

Revert "Upload bin dir fir windows_dotnet_build"

95ec6a1

This reverts commit e134baf. Things are now working; we don't need to publish the `bin` dir.

jonpryor force-pushed the jonp-java-interop-windows-build branch from affae8c to cc2ef8b Compare March 16, 2021 19:41

jonpryor requested review from jonathanpeppers, jpobst and grendello March 16, 2021 19:58

"Multitarget" NativeTiming as well

7016a28

grendello approved these changes Mar 16, 2021

View reviewed changes

jonathanpeppers reviewed Mar 16, 2021

View reviewed changes

jonpryor added 5 commits March 16, 2021 18:00

Use Ninja instead of Makefiles.

eb4eab1

Avoid $(OS)

44f83ce

Context: dotnet/msbuild#539 (comment) Sometimes the `%OS%` environment variable is set, which overrides the normally expected `$(OS)` MSBuild property. Instead of using `$(OS)`, use `$([MSBuild]::IsOSPlatform(name))`.

set CMAKE_C_STANDARD to 11

4f46058

jpobst approved these changes Mar 16, 2021

View reviewed changes

jonpryor added 2 commits March 16, 2021 19:49

Remove diagnostic messages.

0bccb2d

ITEM GROUP

edf6d9d

jonathanpeppers approved these changes Mar 17, 2021

View reviewed changes

jonpryor merged commit 3824b97 into dotnet:main Mar 17, 2021

jpobst added this to the 11.3 (16.10 / 8.10) milestone May 5, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java-interop] Windows build system support #816

[java-interop] Windows build system support #816

jonpryor commented Mar 11, 2021

jonpryor Mar 11, 2021

jonpryor commented Mar 12, 2021

jonpryor commented Mar 12, 2021

jonpryor commented Mar 14, 2021

grendello Mar 16, 2021

jonpryor Mar 16, 2021

jpobst commented Mar 16, 2021 •

edited

Loading

jonathanpeppers Mar 16, 2021

jonpryor Mar 16, 2021

jonathanpeppers Mar 16, 2021

jonathanpeppers Mar 16, 2021

jonpryor commented Mar 16, 2021 •

edited

Loading

[java-interop] Windows build system support #816

[java-interop] Windows build system support #816

Conversation

jonpryor commented Mar 11, 2021

jonpryor Mar 11, 2021

Choose a reason for hiding this comment

jonpryor commented Mar 12, 2021

jonpryor commented Mar 12, 2021

jonpryor commented Mar 14, 2021

grendello Mar 16, 2021

Choose a reason for hiding this comment

jonpryor Mar 16, 2021

Choose a reason for hiding this comment

jpobst commented Mar 16, 2021 • edited Loading

jonathanpeppers Mar 16, 2021

Choose a reason for hiding this comment

jonpryor Mar 16, 2021

Choose a reason for hiding this comment

jonathanpeppers Mar 16, 2021

Choose a reason for hiding this comment

jonathanpeppers Mar 16, 2021

Choose a reason for hiding this comment

jonpryor commented Mar 16, 2021 • edited Loading

jpobst commented Mar 16, 2021 •

edited

Loading

jonpryor commented Mar 16, 2021 •

edited

Loading