[build] Use $(VSINSTALLDIR), not $(VSINSTALLROOT) #1048
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Context: 3824b97 Context: a3de91e Context: https://learn.microsoft.com/en-us/cpp/build/reference/msbuild-visual-cpp-overview?view=msvc-170 I do not remember why commit 3824b97 went with `$(VSINSTALLROOT)`, but the `%VSINSTALLROOT%` environment variable is *not* set within the Visual Studio Command Prompt `VsDevCmd.bat`. This means that when building `src/java-interop` on Windows off of CI, by default it won't do anything, because `$(NativeToolchainSupported)` won't be set. Replace all usage of `$(VSINSTALLROOT)` with `$(VSINSTALLDIR)`, which *is* documented, and *is* set by `VsDevCmd.bat`. This should allow Windows builds to produce `java-interop.dll` without grief.
but why! Pull out the diagnostic logging!
jonpryor
force-pushed
the
jonp-use-vsinstalldir
branch
from
35d923a to
7cbfe43
Compare
Because that's what exists on CI; for the ".NET Framework" +
VS 2022 CI bot, courtesy a 449MB build log, VS 2022 has:
VsInstallRoot = C:\Program Files\Microsoft Visual Studio\2022\Enterprise
among many, many other env vars which contain `\2022\Enterprise`.
The .NET Core build, meanwhile, has *none of that*. It also shows
significantly fewer environment variables; 55 lines of env vars
vs. 209 for .NET Framework. (Added security?)
Make a squircle: update the `$(_VcvarsallPath)` check to support
*both* `$(VsInstallRoot)` (.NET Framework CI) and `$(VsInstallDir)`.
The .NET Core Windows build will explicitly set `VsInstallDir`.
Hopefully this results in a viable build environment.
Mostly. Even though the .NET Core build logs *still* don't contain `VsInstallDir` -- part of the `variables:` block in `azure-pipelines.yaml` -- the logs *do* contain `NativeToolchainSupported = True`. The updated `VsInstallDir` check works! What *doesn't* work is the actual src/java-interop build (?!) Task "Exec" (TaskId:943) Task Parameter:Command=call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64 && cmake -G "NMake Makefiles" -S "D:/a/_work/1/s/src/java-interop/" -B "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" "-DJDK_INCLUDE_LIST=C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include;C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include/win32" "-DJNI_C_PATH=D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/jni.c" && cmake --build "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" -v (TaskId:943) call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64 && cmake -G "NMake Makefiles" -S "D:/a/_work/1/s/src/java-interop/" -B "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" "-DJDK_INCLUDE_LIST=C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include;C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include/win32" "-DJNI_C_PATH=D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/jni.c" && cmake --build "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" -v (TaskId:943) ********************************************************************** (TaskId:943) ** Visual Studio 2022 Developer Command Prompt v17.3.5 (TaskId:943) ** Copyright (c) 2022 Microsoft Corporation (TaskId:943) ********************************************************************** (TaskId:943) [ERROR:VsDevCmd.bat] *** VsDevCmd.bat encountered errors. Environment may be incomplete and/or incorrect. *** (TaskId:943) [ERROR:VsDevCmd.bat] In an uninitialized command prompt, please 'set VSCMD_DEBUG=[value]' and then re-run (TaskId:943) [ERROR:VsDevCmd.bat] vsdevcmd.bat [args] for additional details. (TaskId:943) [ERROR:VsDevCmd.bat] Where [value] is: (TaskId:943) [ERROR:VsDevCmd.bat] 1 : basic debug logging (TaskId:943) [ERROR:VsDevCmd.bat] 2 : detailed debug logging (TaskId:943) [ERROR:VsDevCmd.bat] 3 : trace level logging. Redirection of output to a file when using this level is recommended. (TaskId:943) [ERROR:VsDevCmd.bat] Example: set VSCMD_DEBUG=3 (TaskId:943) [ERROR:VsDevCmd.bat] vsdevcmd.bat > vsdevcmd.trace.txt 2>&1 (TaskId:943) CMake Error at CMakeLists.txt:3 (project): (TaskId:943) … ##[warning]EXEC(0,0): Warning : CMAKE_CXX_COMPILER not set, after EnableLanguage ##[warning]EXEC(0,0): Warning : CMAKE_C_COMPILER not set, after EnableLanguage Since .NET Framework builds now, remove the diagnostic logging for .NET Framework in 7cbfe43. Update `RunCmake.proj` so that we do as the warning suggests: add `set VSCMD_DEBUG=3` as part of the build.
Now that we have `set VSCMD_DEBUG=3`, we have *moar input*: [DEBUG:VsDevCmd.bat] calling "ext\clang_cl.bat" (TaskId:5320) [ERROR:ext\clang_cl.bat] init:FAILED code:1 (TaskId:5320) `ext\clang_cl.bat` is failing. Unfortunately, my local VS install doesn't have `clang_cl.bat`, and the log file doesn't have much additional information. :-( Some "random" searching later, and we hit: https://developercommunity.visualstudio.com/t/vcvars32bat-fails-after-upgrade-to-vs-2017-version/375841 > The main issue with the error message " init:FAILED code:1" was, it was just a mere message to the user, it was not a actual error message in my case, for me the actual error was there was no trailing "\" appended for environment variable "VSINSTALLDIR" (?!) Update `azure-pipelines.yaml` so that `VSINSTALLDIR` ends with `\`.
Context: 3824b97 Context: a3de91e Context: 5c756b1 Context: https://learn.microsoft.com/en-us/cpp/build/reference/msbuild-visual-cpp-overview?view=msvc-170 There is an "interesting" dichotomy between local Windows builds and CI builds of xamarin/Java.Interop regarding the `%VSINSTALLROOT%` environment variable: 1. `%VSINSTALLDIR%` is set automatically on CI for .NET Framework. 2. `%VSINSTALLROOT%`is *not* set automatically for .NET Framework or .NET Core, which is why commit 5c756b1 added `VSInstallRoot` to the *global* variables block in `azure-pipelines.yaml`. 3. `%VSINSTALLDIR%` *is* set, but `%VSINSTALLROOT%` is *not* set when using the [**Developer Command Prompt For…**][0] shortcut item. The result of this is that `src/java-interop` is built on CI, thus ensuring that it continues to build and works via unit tests (yay), but it is *not* easy to build in a local checkout. The build doesn't fail; it just does not produce per-arch `java-interop.dll` files. (Handy way to check for this: in a diagnostic build log, search for `NativeToolchainSupported = True`. If you don't see it, you don't have Windows support.) This makes things "weird" when trying to help people build the repo on Windows. Furthermore, CI *didn't* use `%VSINSTALLDIR%`, because it was not set when building for .NET Core; `%VSINSTALLROOT%` was set "consistently" for "everyone" (except for local builds). Try to restore some degree of sanity: *probe* for and support *both* `%VSINSTALLDIR%` *and* `%VSINSTALLROOT%`, with `%VSINSTALLROOT%` taking priority, and `%VSINSTALLDIR%` as fallback. Update `azure-pipelines.yaml` so that only .NET Core builds set the `VSINSTALLDIR` environment variable. .NET Framework builds will use the implicitly set `%VsInstallRoot%` env var. Finally, for .NET Core builds, `%VSINSTALLDIR%` ***MUST*** [end with a trailing `\`][1]. If it doesn't, the build will fail in "mysterious" ways: Task "Exec" (TaskId:943) Task Parameter:Command=call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64 && cmake -G "NMake Makefiles" -S "D:/a/_work/1/s/src/java-interop/" -B "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" "-DJDK_INCLUDE_LIST=C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include;C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include/win32" "-DJNI_C_PATH=D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/jni.c" && cmake --build "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" -v (TaskId:943) call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvarsall.bat" x86_amd64 && cmake -G "NMake Makefiles" -S "D:/a/_work/1/s/src/java-interop/" -B "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" "-DJDK_INCLUDE_LIST=C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include;C:/hostedtoolcache/windows/Java_Temurin-Hotspot_jdk/8.0.345-1/x64/include/win32" "-DJNI_C_PATH=D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/jni.c" && cmake --build "D:/a/_work/1/s/src/java-interop/obj//Release-net7.0/win-x64/" -v (TaskId:943) ********************************************************************** (TaskId:943) ** Visual Studio 2022 Developer Command Prompt v17.3.5 (TaskId:943) ** Copyright (c) 2022 Microsoft Corporation (TaskId:943) ********************************************************************** (TaskId:943) [ERROR:VsDevCmd.bat] *** VsDevCmd.bat encountered errors. Environment may be incomplete and/or incorrect. *** (TaskId:943) [ERROR:VsDevCmd.bat] In an uninitialized command prompt, please 'set VSCMD_DEBUG=[value]' and then re-run (TaskId:943) [ERROR:VsDevCmd.bat] vsdevcmd.bat [args] for additional details. (TaskId:943) [ERROR:VsDevCmd.bat] Where [value] is: (TaskId:943) [ERROR:VsDevCmd.bat] 1 : basic debug logging (TaskId:943) [ERROR:VsDevCmd.bat] 2 : detailed debug logging (TaskId:943) [ERROR:VsDevCmd.bat] 3 : trace level logging. Redirection of output to a file when using this level is recommended. (TaskId:943) [ERROR:VsDevCmd.bat] Example: set VSCMD_DEBUG=3 (TaskId:943) [ERROR:VsDevCmd.bat] vsdevcmd.bat > vsdevcmd.trace.txt 2>&1 (TaskId:943) CMake Error at CMakeLists.txt:3 (project): (TaskId:943) … ##[warning]EXEC(0,0): Warning : CMAKE_CXX_COMPILER not set, after EnableLanguage ##[warning]EXEC(0,0): Warning : CMAKE_C_COMPILER not set, after EnableLanguage [0]: https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170#developer_command_file_locations [1]: https://developercommunity.visualstudio.com/t/vcvars32bat-fails-after-upgrade-to-vs-2017-version/375841
jonpryor
force-pushed
the
jonp-use-vsinstalldir
branch
from
15abb3c to
ab4b9e3
Compare
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
Author
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
Nov 1, 2022
Fixes: dotnet/java-interop#1034 Fixes: dotnet/java-interop#1051 Context: 938b2cb Changes: dotnet/java-interop@e1ee4b1...5318261 * dotnet/java-interop@53182615: [build] Update Microsoft.* NuGet package versions (dotnet/java-interop#1055) * dotnet/java-interop@8e18c909: [generator] Avoid C#11 delegate cache overhead. (dotnet/java-interop#1053) * dotnet/java-interop@2d8b6d24: [generator] More AttributeTargets on SupportedOSPlatformAttribute (dotnet/java-interop#1054) * dotnet/java-interop@7dfbab67: [generator] Add [SupportedOSPlatform] to bound constant fields (dotnet/java-interop#1038) * dotnet/java-interop@1720628a: [generator] Mark generated .cs files as generated (dotnet/java-interop#1052) * dotnet/java-interop@f498fcf5: [Java.Interop] Avoid some method group conversions (dotnet/java-interop#1050) * dotnet/java-interop@16e1ecd4: [build] Use $(VSINSTALLDIR), not $(VSINSTALLROOT) (dotnet/java-interop#1048) * dotnet/java-interop@8e4c7d20: [Hello-Core] Add "low level" sample. (dotnet/java-interop#1047) Additionally, remove `$(LangVersion)`=10 from `Mono.Android.csproj`, which was a hack to work around a size regression due to delegate caching in C# 11; see also 938b2cb, dotnet/java-interop@8e18c909. Co-authored-by: Jonathan Pobst <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: 3824b97
Context: a3de91e
Context: https://learn.microsoft.com/en-us/cpp/build/reference/msbuild-visual-cpp-overview?view=msvc-170
I do not remember why commit 3824b97 went with
$(VSINSTALLROOT), but the%VSINSTALLROOT%environment variable is not set within the Visual Studio Command PromptVsDevCmd.bat.This means that when building
src/java-interopon Windows off of CI, by default it won't do anything, because$(NativeToolchainSupported)won't be set.Replace all usage of
$(VSINSTALLROOT)with$(VSINSTALLDIR), which is documented, and is set byVsDevCmd.bat. This should allow Windows builds to producejava-interop.dllwithout grief.