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

[Java.Interop-PerformanceTests] Support .NET Core 3.1 #808

Merged
merged 12 commits into from
Mar 8, 2021

Conversation

jonpryor
Copy link
Member

Build Java.Interop-PerformanceTests.csproj for netcoreapp3.1, and
run it using dotnet test.

This in turn requires building the tests/NativeTiming native library
for .NET Core.

Add a cmake-based build system for tests/NativeTiming.

Update core-tests.yaml so that dotnet test is used to run both
the net472 and netcoreapp3.1 versions of
Java.Interop-PerformanceTests.dll.

@jonpryor jonpryor requested a review from jpobst February 24, 2021 16:28
@jonpryor jonpryor force-pushed the jonp-perftests-netcore branch 4 times, most recently from 23be554 to c8bed5e Compare February 25, 2021 02:34
@jonpryor
Copy link
Member Author

After lots of hair pulling, the repro for the Mac - .NET Core failure requires two steps;

  1. Use the "dotnet" build scenario.
  2. Use the Release configuration
% make prepare-core CONFIGURATION=Release
% dotnet build Java.Interop.sln -c Release -t:Prepare
% dotnet build Java.Interop.sln -c Release -m:1 /v:diag > b.txt

Also note that -m:1 is required, otherwise you'll get different build failures.

What appears to be happening is that tests/NativeTiming/NativeTiming.csproj is trying to load bin/BuildDebug/JdkInfo.props -- the Debug configuration! -- which doesn't exist. Because it doesn't exist, @(JdkIncludePath) isn't set, and thus <jni.h> can't be found.

A previous version used <Project Sdk="Project Sdk="Microsoft.Build.NoTargets">, which emits a warning:

tests/NativeTiming/Directory.Build.targets : 
warning MSB4011: "…/Java.Interop/packages/microsoft.build.notargets/2.0.1/Sdk/Sdk.props"
cannot be imported again. It was already imported at
"…/Java.Interop/tests/NativeTiming/NativeTiming.csproj"

Fixing this warning doesn't fix the build failure.

@jonpryor
Copy link
Member Author

The problem is the .sln! If a .csproj isn't in the .sln, then it "ignores" dotnet build -c, apparently. :-/

@jonpryor jonpryor force-pushed the jonp-perftests-netcore branch 7 times, most recently from 1ce027f to 6b69342 Compare February 26, 2021 00:25
@jonpryor
Copy link
Member Author

While using ninja was suggested (via cmake -G Ninja), I stayed with Makefiles because they actually worked on CI without additional provisioning; ninja isn't pre-installed.

Additionally, the Windows cmake default of cmake -G "Visual Studio 16 2019" doesn't work (I didn't investigate why). Using cmake -G "NMake Makefiles" does work, and is preinstalled, so I went with that.

@jonpryor
Copy link
Member Author

…or all native compilation on Windows is possibly "out", as cl isn't in %PATH% either! https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4504944&view=logs&j=f31c9f97-4411-58e7-49ac-fc73f645e6b6&t=cbeb8522-7b64-5cc0-7bee-eff0e164a409

  cmake -G "NMake Makefiles" -S . -B obj\\Release\ "-DJDK_INCLUDE_LIST=C:\Program Files\Java\jdk8u282-b08\include;C:\Program Files\Java\jdk8u282-b08\include\win32"
  -- The C compiler identification is unknown
  CMake Error at CMakeLists.txt:3 (project):
    The CMAKE_C_COMPILER:
  
      cl
  
    is not a full path and was not found in the PATH.
  
    To use the NMake generator with Visual C++, cmake must be run from a shell
    that can use the compiler cl from the command line.  This environment is
    unable to invoke the cl compiler.  To fix this problem, run cmake from the
    Visual Studio Command Prompt (vcvarsall.bat).
  
    Tell CMake where to find the compiler by setting either the environment
    variable "CC" or the CMake cache entry CMAKE_C_COMPILER to the full path to
    the compiler, or to the compiler

@jonpryor jonpryor force-pushed the jonp-perftests-netcore branch 3 times, most recently from 25ae7d8 to 98e79ff Compare February 26, 2021 02:34
@jonpryor
Copy link
Member Author

getting cl.exe into %PATH% is "interesting"; executing vcvars64.bat (or any other vcvars*.bat script) fails: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4505234&view=logs&j=f31c9f97-4411-58e7-49ac-fc73f645e6b6&t=2bcaa12b-2f4b-5b1f-c519-10308f653190

**********************************************************************
** Visual Studio 2019 Developer Command Prompt v16.8.5
** Copyright (c) 2020 Microsoft Corporation
**********************************************************************
[ERROR:team_explorer.bat] Directory not found : "CommonExtensions\Microsoft\TeamFoundation\Team Explorer"
[ERROR:VsDevCmd.bat] *** VsDevCmd.bat encountered errors. Environment may be incomplete and/or incorrect. ***
[ERROR:VsDevCmd.bat] In an uninitialized command prompt, please 'set VSCMD_DEBUG=[value]' and then re-run
[ERROR:VsDevCmd.bat] vsdevcmd.bat [args] for additional details.
[ERROR:VsDevCmd.bat] Where [value] is:
[ERROR:VsDevCmd.bat]    1 : basic debug logging
[ERROR:VsDevCmd.bat]    2 : detailed debug logging
[ERROR:VsDevCmd.bat]    3 : trace level logging. Redirection of output to a file when using this level is recommended.
[ERROR:VsDevCmd.bat] Example: set VSCMD_DEBUG=3
[ERROR:VsDevCmd.bat]          vsdevcmd.bat > vsdevcmd.trace.txt 2>&1

Which is…something.

Trying a Horrible Hack in which I explicitly set %PATH% to include a directory that may (?) contain cl.exe:

variables:
  VSInstallDir: C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise
  Cl64Path: $(VSInstallDir)\VC\Tools\MSVC\14.28.29334\bin\Hostx64\x64

jobs:
- job: windows_build
  
  steps:
  - script: >
      echo "##vso[task.setvariable variable=PATH]%PATH%;$(Cl64Path)"
    displayName: 'Add cl to PATH'

which is guaranteed to break every time the VM is changed…

@jonpryor
Copy link
Member Author

Perhaps unsurprisingly, setting %PATH% also failed: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4505376&view=logs&j=10b5dba2-0c78-5c70-4079-bab96311b95e&t=3838b6a3-21fa-59b7-5217-f31a6517c4b7

Surprisingly (to me), the build logs don't print out what %PATH% is, so I can't tell if it's correct either.

Build `Java.Interop-PerformanceTests.csproj` for `netcoreapp3.1`, and
run it using `dotnet test`.

This in turn requires building the `tests/NativeTiming` native library
for .NET Core, which in turn requires moving away from the
Visual Studio for Mac (née Xamarin Studio) -based `.cproj` file.
Replace `NativeTiming.cproj` with a `Microsoft.Build.NoTargets`-
based `NativeTiming.csproj` file, so that it can be referenced and
built by `msbuild` and `dotnet build`.

Add a `cmake`+"Makefile"-based build system for `tests/NativeTiming`.

Update `core-tests.yaml` so that `dotnet test` is used to run both
the net472 and netcoreapp3.1 versions of
`Java.Interop-PerformanceTests.dll.`

Note (for posterity): if a `.*proj` file depends upon
`$(Configuration)`, it *must also* be added to `Java.Interop.sln`.
Failure to do so will mean that when building the project in
*Release* configuration, the *new* project will instead be built in
*Debug* configuration, resulting in all manner of head-scratching.
@jonpryor jonpryor force-pushed the jonp-perftests-netcore branch from 98e79ff to a14a39d Compare February 26, 2021 02:48
find all `**\cl.exe` files under the VS install dir?
[Last attempt][0] found a compiler!

    cmake -G "NMake Makefiles" -S . -B obj\\Release\ "-DJDK_INCLUDE_LIST=C:\Program Files\Java\jdk8u282-b08\include;C:\Program Files\Java\jdk8u282-b08\include\win32" "-DCMAKE_C_COMPILER=C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.28.29333\bin\Hostx64\x64\cl.exe"

It still failed:

    Syntax error in cmake code at

      D:/a/1/s/tests/NativeTiming/obj/Release/CMakeFiles/3.19.4/CMakeCCompiler.cmake:1

    when parsing string

      C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.28.29333\bin\Hostx64\x64\cl.exe

    Invalid escape sequence \P

`cmake` doesn't like DOS directory separators (`\`).

Replace with Unix dir separators (`/`).

[0]: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=4505481&view=logs&j=f31c9f97-4411-58e7-49ac-fc73f645e6b6&t=cbeb8522-7b64-5cc0-7bee-eff0e164a409
@jonpryor
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor
Copy link
Member Author

…and now we hit: https://stackoverflow.com/questions/9356135/link-fatal-error-lnk-1104-cannot-open-file-libcmt-lib

  LINK : fatal error LNK1104: cannot open file 'LIBCMT.lib'

Looks like I need to run vcvars32.bat, but my previous attempt to do so had failed. :-/

Maybe if I try the "item group search" idea and run it inside the <Exec/>?

After following: https://docs.microsoft.com/en-us/azure/devops/pipelines/apps/windows/cpp?view=azure-devops

The pipeline YAML is:

	# .NET Desktop
	# Build and run tests for .NET Desktop or Windows classic desktop solutions.
	# Add steps that publish symbols, save build artifacts, and more:
	# https://docs.microsoft.com/azure/devops/pipelines/apps/windows/dot-net

	trigger:
	- master

	pool:
	  vmImage: 'windows-latest'

	variables:
	  solution: '**/*.sln'
	  buildPlatform: 'Any CPU'
	  buildConfiguration: 'Release'

	steps:
	- task: NuGetToolInstaller@1

	- task: NuGetCommand@2
	  inputs:
	    restoreSolution: '$(solution)'

	- task: VSBuild@1
	  inputs:
	    solution: '$(solution)'
	    platform: '$(buildPlatform)'
	    configuration: '$(buildConfiguration)'

	- task: VSTest@2
	  inputs:
	    platform: '$(buildPlatform)'
	    configuration: '$(buildConfiguration)'

Meaning it looks like the *important* bit is to use `VSBuild@1`,
not `MSBuild@1`, presumably because `VSBuild` ensures that the C++
toolchain actually works…

Time to test that guess.

Plus other cleanup, e.g. declare "private" properties, etc.
jonpryor added 4 commits March 4, 2021 21:42
Is `cl` in path (6da1e49)?  ***NO***.

Furthermore, based on the sample docs:

  * https://docs.microsoft.com/en-us/azure/devops/pipelines/apps/windows/cpp?view=azure-devops
  * https://github.com/adventworks/cpp-sample/tree/master/cpp-sample

The sample doesn't invoke `cl` directly.  It uses a `.vcxproj`.

Let's Try That?

Update `cmake` to emit a `.vcxproj` file.  Let's see where *that* leads.
Context: dotnet#808 (comment)
Context: https://github.com/dotnet/runtime/blob/e1e46a8d2ca9c2c932c8ceb61f884c7c82351442/eng/native/init-compiler-and-cmake.cmd
Context: https://github.com/dotnet/runtime/blob/e1e46a8d2ca9c2c932c8ceb61f884c7c82351442/src/mono/mono.proj#L362

We tried calling `vcvars64.bat` during an earlier invocation, which
failed, but we were attempting to call it from YAML.

Instead, take a page out of the dotnet/runtime build system: Instead
of calling `vcvars64.bat` from YAML, instead call it
*from within MSBuild*, via `call path\to\vcvars64.bat && …`.

Thus, on Windows, we do:

	call "%VSINSTALLROOT%\VC\Auxiliary\Build\vcvarsall.bat x86_amd64 && ^
	  cmake -G "NMake Makefiles" -S . -B obj\Debug -DJDK_INCLUDE_LIST=…"
	…
	call "%VSINSTALLROOT%\VC\Auxiliary\Build\vcvarsall.bat x86_amd64 && ^
	  cd obj\Debug && ^
	  nmake VERSION=1

This *appears* to work from a "clean" shell.  What about CI?
Commit 93872cf worked, in that Windows built!

macOS, not so much, because `$VSINSTALLROOT` was set via YAML, so
macOS was *also* trying to `call path\to\vcvarsall.bat`, which
couldn't possibly work.

Remove the `VSSInstallRoot` setting in YAML; it *appears* that
`msbuild` will set this -- at least it does on *my* Windows box --
and return to using `MSBuild@1` and not `VSBuild@1`.
@jonpryor
Copy link
Member Author

jonpryor commented Mar 6, 2021

Current iteration has everything building except Windows + .NET Core.

@jonpryor
Copy link
Member Author

jonpryor commented Mar 6, 2021

My guess is that on Windows - .NET Framework, msbuild defines the VSINSTALLROOT environment variable -- which allows vcvarsall.bat to be found -- while Windows - .NET Core does not define VSINSTALLROOT.

What's funny is that the Windows .NET Core build does have some environment variables with Visual Studio in the value -- for VS 14:

VS140COMNTOOLS = C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\Tools\

jonpryor added 2 commits March 5, 2021 20:11
Commit 9b2d23b worked when using .NET Framework on Windows and
on macOS, but failed when using Windows + .NET Core, because
`dotnet build` *doesn't* define `%VSINSTALLROOT%` in that environment.

Update the `windows_dotnet_build` job so that `VSInstallRoot` is
defined.

Update the `NativeTiming` build so that it verifies that
`$(VSINSTALLROOT)` exists before trying to do anything with it.
Commit b66fc2f's attempt to only define `VSInstallRoot` for
`windows_dotnet_build` didn't work, in that `%VSINSTALLROOT%`
wasn't set, causing the build to fail.

Return to declaring `VSInstallRoot` as a global variable.

The `Exists('$(VSINSTALLROOT)')` guard from b66fc2f should prevent
macOS builds from failing.
@jonpryor
Copy link
Member Author

jonpryor commented Mar 6, 2021

Squash-and-merge commit message body:

Context: https://github.com/xamarin/java.interop/pull/808#issuecomment-786370513
Context: https://github.com/dotnet/runtime/blob/e1e46a8d2ca9c2c932c8ceb61f884c7c82351442/eng/native/init-compiler-and-cmake.cmd
Context: https://github.com/dotnet/runtime/blob/e1e46a8d2ca9c2c932c8ceb61f884c7c82351442/src/mono/mono.proj#L362

Build `Java.Interop-PerformanceTests.csproj` for `netstandard2.0`, and
run it using `dotnet test`.

This in turn requires building the `tests/NativeTiming` native library
for .NET Core, which in turn requires moving away from the
Visual Studio for Mac (née Xamarin Studio) -based `.cproj` file.
Replace `NativeTiming.cproj` with a `Microsoft.Build.NoTargets`-
based `NativeTiming.csproj` file, so that it can be referenced and
built by `msbuild` and `dotnet build`.

Update `core-tests.yaml` so that `dotnet test` is used to run both
the net472 and netcoreapp3.1 versions of
`Java.Interop-PerformanceTests.dll.`

Note (for posterity): if a `.*proj` file depends upon
`$(Configuration)`, it *must also* be added to `Java.Interop.sln`.
Failure to do so will mean that when building the project in
*Release* configuration, the *new* project will instead be built in
*Debug* configuration, resulting in all manner of head-scratching.

~~ cmake & Windows & C, Oh My! ~~

Add a `cmake`+"Makefile"-based build system for `tests/NativeTiming`.
Note that in order to build on Windows, `vcvarsall.bat` *must* be
executed *before* using any "native Visual Studio toolchain" commands
such as `cl.exe`.  Additionally, for reasons not currently understood,
running `vcvarsall.bat` *from YAML* -- on the hope/assumption that
this would add`cl.exe`/etc. to `%PATH%` and usable everywhere --
*did not work*.  (Note that `vcvarsall.bat` requires that you specify
which CPU architecture to support; we currently use `x86_amd64`.)

In order for the .NET Core-based build to *find* `vcvarsall.bat`, add
a new `VSInstallRoot` YAML variable.  The .NET Framework-based builds
already have a `%VSINSTALLROOT%` environmemnt variable, as do the
**Developer Command Prompt for VS\*** commands.

Thus, on Windows, we do:

	call "%VSINSTALLROOT%\VC\Auxiliary\Build\vcvarsall.bat x86_amd64 && ^
	  cmake -G "NMake Makefiles" -S . -B obj\Debug -DJDK_INCLUDE_LIST=…"
	
	call "%VSINSTALLROOT%\VC\Auxiliary\Build\vcvarsall.bat x86_amd64 && ^
	  cd obj\Debug && ^
	  nmake VERSION=1

@@ -6,10 +6,10 @@ steps:
displayName: Prepare Solution
inputs:
projects: Java.Interop.sln
arguments: '-c $(Build.Configuration) -target:Prepare'
arguments: '-c $(Build.Configuration) -target:Prepare /v:diag'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer not to add diag, as 99% of the time it just makes the logs harder to read. We can look at recording a binlog and saving it as an artifact.

@jonpryor jonpryor merged commit 5c756b1 into dotnet:main Mar 8, 2021
@jpobst jpobst added this to the 11.3 (16.10 / 8.10) milestone May 5, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 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.

2 participants