search

lukka / run-cmake

GitHub Action to build C++ applications with CMake (CMakePresets.json), Ninja and vcpkg on GitHub.
MIT License
171 stars 19 forks source link

Regression: Windows is now setting `VCPKG_ROOT` to MSVC path. #113

Closed bwrsandman closed 1 year ago

bwrsandman commented 1 year ago

Not sure if it's related to #111 but my builds are suddenly grabbing vcpkg from visual studio.

VCPKG_ROOT: C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\vcpkg

This results in the following error when configuring

 Running command '"C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\vcpkg\vcpkg.exe"' with args '^"env^",^"--bin^",^"--include^",^"--tools^",^"--python^",^"--triplet^",^"x64-windows^",^"set^"' in current directory 'C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\vcpkg'.

Note that RUNVCPKG_VCPKG_ROOT is properly set. This happens only on windows runners.

One month ago, the root was properly set

I have a somewhat atypical setup using run-cmake which configures once to then build debug and release in the same job. My first config step has the correct root but the following runs don't.

https://github.com/openblack/openblack/blob/master/.github/workflows/ci-vcpkg.yml#L66-L95

bwrsandman commented 1 year ago

A workaround which works is adding the following

        env:
          VCPKG_ROOT: ${{github.workspace}}/vcpkg
bwrsandman commented 1 year ago

Additionally, it's injecting more paths into the second run... The following get repeated and it might be causing an error:

'cmd.exe' is not recognized as an internal or external command,

The duplicated paths

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\bin\HostX64\x64
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\VC\VCPackages
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\TestWindow
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\TeamFoundation\Team Explorer
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Current\bin\Roslyn
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Team Tools\Performance Tools\x64
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Team Tools\Performance Tools
C:\Program Files (x86)\Microsoft SDKs\Windows\v10.0A\bin\NETFX 4.8 Tools\x64\
C:\Program Files (x86)\HTML Help Workshop
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\FSharp\Tools
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\Extensions\Microsoft\CodeCoverage.Console
C:\Program Files (x86)\Windows Kits\10\bin\10.0.22621.0\\x64
C:\Program Files (x86)\Windows Kits\10\bin\\x64
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\\MSBuild\Current\Bin\amd64
C:\Windows\Microsoft.NET\Framework64\v4.0.30319
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\Tools\
C:\Windows\system32
C:\Windows
C:\Windows\system32\Wbem
C:\Windows\system32\WindowsPowerShell\v1.0\
D:\a\openblack\openblack\vcpkg_installed\x64-windows\bin
D:\a\openblack\openblack\vcpkg_installed\x64-windows\tools
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\Llvm\x64\bin
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\CommonExtensions\Microsoft\CMake\Ninja
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\Common7\IDE\VC\Linux\bin\ConnectionManagerExe
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\vcpkg
lukka commented 1 year ago

@bwrsandman thanks for reporting this issue! There are at least two problems caused by the introduction of Visual Studio 2022 17.6 in the GitHub runners. Since VS 17.6 includes vcpkg.exe the developer command prompt:

The run-cmake uses the vcpkg env command to setup the environment variables for building with MSVC on Windows. This happens when on Windows, when CC nor CXX are defined, and when VCPKG_ROOT is defined: whan all three are satisfied all the env variable are set using addPath().

The former could be solved by defining the VCPKG_ROOT at build time, with matching behavior of local vs CI build. For example, the CMakePresets.json would be the good place where to define the expected location of VCPKG_ROOT, as it is being done in CMakePresets.json of this C++ project template

For the latter, run-vcpkg@v11 adds to the PATH the path to the vcpkg executable that has been setup by the action. Would be using run-vcpkg@v11 an option for you? I am afraid I have not ported the fix onto run-vcpkg@v10

Regarding the duplicated paths defined in PATH, that happens because each run of run-cmake is going to augment the PATH from the Developer Command Prompt value at each run, which may not be the best option indeed, and the desired PATH should just be set when spawning cmake to avoid side effects.

bwrsandman commented 1 year ago

I've tried run-vcpkg@v11 and it doesn't seem to change much, the issue is really in run-cmake.

lukka commented 1 year ago

@bwrsandman that's right, the run-cmakeaction should not augment "globally" the environment by injecting all the Visual Studio Developer Command Prompt's variables, but it should be only setting the environment for the lifespan of the launched cmake process. But this won't be enough anyway: you must define VPCKG_ROOT before running cmake to ensure that its value is going to be the one you need, that is defining it in the CMakePresets.json (please take a look at my previous message on how this could be done.). This is necessary to override the one defined in the Visual Studio Developer command prompt.

Another observation, for the run-vcpkg step, is that you should drop the usage of appendedCacheKey input, since the key management (or rather the whole caching of the built packages) is totally up to vcpkg in v11, and that is what replaced the best effort caching mechanism of v10.

Let me know if all of this makes sense, or if you see other solutions to the problem.

bwrsandman commented 1 year ago

Testing out run-vcpkg@v11 with the pure configuration and it seems to have solved all my issues.

How does this new cache work? Are all the jobs sharing the same vcpkg cache? If so, how does it differentiate ubuntu with gcc and clang?

Also, putting the vcpkg path in the CMakePresets.json didn't work. I had to put this in workflow file.

lukka commented 1 year ago

@bwrsandman that is great to hear!

Testing out run-vcpkg@v11 with the pure configuration and it seems to have solved all my issues.

Could you share more details about your solution? A link to a commit id or to your working workflow file would help as well.

How does this new cache work? Are all the jobs sharing the same vcpkg cache? If so, how does it differentiate ubuntu with gcc and clang?

run-vcpkg delegates cache management to vcpkg which uses Binary Caching backed onto GitHub Action cache. The granularity of the stored artifacts is for package, for each of them (I believe, I am not aware of the details) should have a key which is computed from everything needed, platform and toolset likely included.

Also, putting the vcpkg path in the CMakePresets.json didn't work. I had to put this in workflow file.

I am not entirely sure I comprehend why it did not work, and I wonder if the exported variables (done by run-cmake when setting up the env var for MSVC) are taking priority somehow on what the CMakePresets.json defines. In principle it should not be the case.

bwrsandman commented 1 year ago

Could you share more details about your solution? A link to a commit id or to your working workflow file would help as well. This is the commit which fixed my most important workflow.

Sure, I had something that looked more like hosted-ninja-vcpkg_submod.yml but after looking at your template, I decided to try something more like hosted-pure-workflow.yml. This prevented my path from being polluted and gave me more flexibility to set custom defines such as -DCMAKE_EXPORT_COMPILE_COMMANDS=ON and building debug and release together which has previously cut my build times and the number of runners used.

The actual VCPKG_ROOT issue is resolved by hardcoding the environment variable.

This is what I had before

      - uses: lukka/get-cmake@latest

      - name: Restore artifacts, or setup vcpkg for building artifacts
        uses: lukka/run-vcpkg@v10
        id: runvcpkg
        with:
          vcpkgJsonGlob: 'vcpkg.json'
          # Prevent ubuntu-latest and ubuntu-latest (clang) from having the same key
          appendedCacheKey: ${{matrix.cc}}
            # doNotCache: true

      - name: Configure CMake+vcpkg+Ninja to generate.
        uses: lukka/run-cmake@v10
        with:
          configurePreset: 'ninja-multi-vcpkg'
          configurePresetCmdString: "[`--preset`, `$[env.CONFIGURE_PRESET_NAME]`, `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON`, `-DOPENBLACK_WARNINGS_AS_ERRORS=ON`]"

      - name: Run CMake+vcpkg+Ninja+CTest to build/test (Debug).
        uses: lukka/run-cmake@v10
        with:
          configurePreset: 'ninja-multi-vcpkg'
          configurePresetCmdString: "[`--preset`, `$[env.CONFIGURE_PRESET_NAME]`, `-DOPENBLACK_WARNINGS_AS_ERRORS=ON`]"
          buildPreset: 'ninja-multi-vcpkg-debug'
          testPreset: 'ninja-multi-vcpkg-debug'

      - name: Run CMake+vcpkg+Ninja+CTest to build/test (Release).
        uses: lukka/run-cmake@v10
        with:
          configurePreset: 'ninja-multi-vcpkg'
          configurePresetCmdString: "[`--preset`, `$[env.CONFIGURE_PRESET_NAME]`, `-DOPENBLACK_WARNINGS_AS_ERRORS=ON`]"
          buildPreset: 'ninja-multi-vcpkg-release'
          testPreset: 'ninja-multi-vcpkg-release'

I have two workflows using run-vcpkg and these are the changes:

  1. VCPKG CI which is a standard test for ubuntu-22.04, mac-latest, windows-latest and also ubuntu-22.04 with clang and ubuntu-20.04 with and without clang. Each permutation builds and runs ctest for debug and release in the same job. They export a compile database and other binary artifacts for later tests.
  2. Cross Compile which tests building on ubuntu and windows for non-native targets: x86-windows, 4 flavors of android and emscripten. These build debug and release in the same job but do not run ctests. They require host-side vcpkg packages to be built to compile shaders (vcpkg install bgfx[tools]). I am still working on these tests as I am trying to get rid of custom presets for android and emscripten in order to replace them with ninja + triplets.
lukka commented 1 year ago

@bwrsandman thanks for all the info! I have pushed onto run-cmake@main the changes to prevent env var are set inappropriately for the subsequent steps (and not just the current step execution). You may try uring run-cmake@main if it helps you to shorten the workflow, and let me know if the problem you experienced are not gone.

Note I hae not released the changes yet, and I'll create a release only after I run my own validation tests.

bwrsandman commented 1 year ago

I tested the combination of run-cmake@main and env: VCPKG_ROOT: ${{github.workspace}}/vcpkg and this definitely fixes the issues with PATH and the fail about not finding cmd.exe goes away.

bwrsandman commented 1 year ago

I think I will probably stick to the pure configuration because I find it easier to read and understand. On second thought, having annotations is too important.

Is this configuration supported? I expect the first lukka/run-cmake@main to build the packages and the next two to only build the project. Also, can configurePresetAdditionalArgs be specified only once in the configure step?

      - uses: lukka/get-cmake@latest

      - name: Restore from cache and setup vcpkg executable and data files.
        uses: lukka/run-vcpkg@v11
        with:
          vcpkgJsonGlob: 'vcpkg.json'

      - name: Run CMake+vcpkg to build packages.
        uses: lukka/run-cmake@main
        with:
          configurePreset: 'ninja-multi-vcpkg'
          configurePresetAdditionalArgs: "['-DOPENBLACK_WARNINGS_AS_ERRORS=ON']"

      - name: Run CMake+Ninja to build the code (Debug).
        uses: lukka/run-cmake@main
        with:
          configurePreset: '${{ matrix.preset }}'
          configurePresetAdditionalArgs: "['-DOPENBLACK_WARNINGS_AS_ERRORS=ON']"
          buildPreset: '${{ matrix.preset }}-debug'

      - name: Run CMake+vcpkg to build the code (Release).
        uses: lukka/run-cmake@main
        with:
          configurePreset: '${{ matrix.preset }}'
          configurePresetAdditionalArgs: "['-DOPENBLACK_WARNINGS_AS_ERRORS=ON']"
          buildPreset: '${{ matrix.preset }}-release'
flichtenheld commented 1 year ago

I'm running into the same regression. Mainly that VCPKG_ROOT is just overridden to point to MSVC. See https://github.com/OpenVPN/openvpn-gui/actions/runs/5254425067 for an action that worked a few weeks ago but now suddenly fails. I tried using run-vcpkg@11 but doesn't seem to solve that particular issue. Will look into the run-cmake@main changes to see whether they resolve it.

flichtenheld commented 1 year ago

@lukka run-cmake@main doesn't seem to fix anything for my use-case. See e.g. https://github.com/flichtenheld/openvpn-gui/actions/runs/5258007838/jobs/9501641706

VCPKG_ROOT is set correctly by run-vcpkg but then run-cmake "ignores" it by using the value from vcpkg env.

The relevant parts of the workflow are 

strategy:
  matrix:
    arch: [x86, x64, arm64]
    ovpn3:
      - preset: ""
        name: ""
        upload_name: ""
      - preset: -ovpn3
        name: "- ovpn3"
        upload_name: "_ovpn3"

name: 'msvc - ${{matrix.arch}} ${{ matrix.ovpn3.name }}'
runs-on: windows-latest
steps:
- uses: actions/checkout@v3
- uses: lukka/get-cmake@latest

- name: Restore artifacts, or setup vcpkg (do not install any package)
  uses: lukka/run-vcpkg@v11
  with:
    vcpkgDirectory: '${{ runner.workspace }}/b/vcpkg'
    vcpkgGitCommitId: "1ba9a2591f15af5900f2ce2b3e2bf31771e3ac48"

- name: Run CMake consuming CMakePreset.json and vcpkg.json by mean of vcpkg.
  uses: lukka/run-cmake@main
  with:
    configurePreset: '${{ matrix.arch }}-release${{ matrix.ovpn3.preset }}'
    buildPreset: '${{ matrix.arch }}-release${{ matrix.ovpn3.preset }}'
lukka commented 1 year ago

@bwrsandman thank you for trying out run-cmake@main, I'll run a set of validation test before releasing it (as v10 or as v11? This may be a breaking change, but it fixes an existing issue at the same time.....

@flichtenheld thank you for the info as well, I think that the problem you are seeing is that run-cmake, before running cmake, is setting up the environment according to vcpkg env, which means whatever was defined in VCPKR_ROOT is going to be overridden if Visual Studio 2022 17.6 is going to be used (because it is setting the value of VCPKG_ROOT overriding any existing value).

There are many solutions, depending on what is your scenario:

Feedback is welcome as usual.

lukka commented 1 year ago

Is this configuration supported? I expect the first lukka/run-cmake@main to build the packages and the next two to only build the project. Also, can configurePresetAdditionalArgs be specified only once in the configure step?

      - uses: lukka/get-cmake@latest

      - name: Restore from cache and setup vcpkg executable and data files.
        uses: lukka/run-vcpkg@v11
        with:
          vcpkgJsonGlob: 'vcpkg.json'

      - name: Run CMake+vcpkg to build packages.
        uses: lukka/run-cmake@main
        with:
          configurePreset: 'ninja-multi-vcpkg'
          configurePresetAdditionalArgs: "['-DOPENBLACK_WARNINGS_AS_ERRORS=ON']"

      - name: Run CMake+Ninja to build the code (Debug).
        uses: lukka/run-cmake@main
        with:
          configurePreset: '${{ matrix.preset }}'
          configurePresetAdditionalArgs: "['-DOPENBLACK_WARNINGS_AS_ERRORS=ON']"
          buildPreset: '${{ matrix.preset }}-debug'

      - name: Run CMake+vcpkg to build the code (Release).
        uses: lukka/run-cmake@main
        with:
          configurePreset: '${{ matrix.preset }}'
          configurePresetAdditionalArgs: "['-DOPENBLACK_WARNINGS_AS_ERRORS=ON']"
          buildPreset: '${{ matrix.preset }}-release'

@bwrsandman Sorry i missed your questions before, here below what I think are the answers.

In the first run-cmake you need to define the variable definition (as you are already doing), then that is going to be cached and it is not needed anymore in subsequent steps (this is CMake doing it). On the 2nd and 3rd run-cmake you could drop the configurePreset (and configurePresetAdditionalArgs) and just keep the build preset, it will just run the build (and skip the configuration that already happened). Let me know if it is working as I think it does :)

flichtenheld commented 1 year ago

@bwrsandman thank you for trying out run-cmake@main, I'll run a set of validation test before releasing it (as v10 or as v11? This may be a breaking change, but it fixes an existing issue at the same time.....

@flichtenheld thank you for the info as well, I think that the problem you are seeing is that run-cmake, before running cmake, is setting up the environment according to vcpkg env, which means whatever was defined in VCPKR_ROOT is going to be overridden if Visual Studio 2022 17.6 is going to be used (because it is setting the value of VCPKG_ROOT overriding any existing value).

There are many solutions, depending on what is your scenario:

* I could change `run-cmake` in a way that when it is setting up the MSVC environment, it wont override `VCPKG_ROOT` if it is already defined. This probably is the right fix, feedback is welcome.

I would definitely go for that. VCPKG_ROOT is a pretty standard variable name and this is a definite regression that breaks a lot of existing setups including some of the documented use-cases for run-vcpkg (due to a change outside of your control...). This solution would fix the existing setups to work again (AFAICT). Now if people actually want to use the built-in vcpkg they just don't set VCPKG_ROOT beforehand (e.g. don't use run-vcpkg). So this indeed sounds like the right solution to me.

bwrsandman commented 1 year ago

Moving the off topic discussion about configure + build (debug) + build (release) to another issue: #115

bwrsandman commented 1 year ago

Managed to get VCPKG_ROOT in CMakePresets.json working.

lukka commented 1 year ago

@bwrsandman @flichtenheld thanks again for all the useful info you provided to come up with a potential solution to the problems you have encountered. I have just pushed on run-cmake@main a version that should fix most of it, in particular:

Let me know and eventually I plan to publish it as the next v10 version.

flichtenheld commented 1 year ago

@bwrsandman @flichtenheld thanks again for all the useful info you provided to come up with a potential solution to the problems you have encountered. I have just pushed on run-cmake@main a version that should fix most of it, in particular:

* `run-cmake` won't override `VCPKG_ROOT` if already defined.

I tried to test this, but doesn't seem to work for me. See https://github.com/flichtenheld/openvpn-gui/actions/runs/5278823831/jobs/9548669301

Download action repository 'lukka/run-cmake@main' (SHA:0be4545f1ef26242f38471884b8bbb9faf59cd74)
[....]
Run lukka/run-cmake@main
  with:
    configurePreset: x64-release
    buildPreset: x64-release
    cmakeListsTxtPath: D:\a\openvpn-gui\openvpn-gui/CMakeLists.txt
    configurePresetAdditionalArgs: []
    buildPresetAdditionalArgs: []
    testPresetAdditionalArgs: []
    packagePresetAdditionalArgs: []
    useShell: true
    logCollectionRegExps: \s*"(.+CMakeOutput\.log)"\.\s*;\s*"(.+CMakeError\.log)"\.\s*;\s*(.+out\.log)\s*;\s+(.+err\.log)\s*;\s*(.+vcpkg.+\.log)\s*
    workflowPresetCmdString: [`--workflow`, `--preset`, `$[env.WORKFLOW_PRESET_NAME]`, `--fresh`]
    configurePresetCmdString: [`--preset`, `$[env.CONFIGURE_PRESET_NAME]`]
    buildPresetCmdString: [`--build`, `--preset`, `$[env.BUILD_PRESET_NAME]`]
    testPresetCmdString: [`--preset`, `$[env.TEST_PRESET_NAME]`]
    packagePresetCmdString: [`--preset`, `$[env.PACKAGE_PRESET_NAME]`]
    runVcpkgEnvFormatString: [`env`, `--bin`, `--include`, `--tools`, `--python`, `--triplet`, `$[env.VCPKG_DEFAULT_TRIPLET]`, `set`]
  env:
    VCPKG_DEFAULT_BINARY_CACHE: D:/a/openvpn-gui/b/vcpkg_cache
    VCPKG_BINARY_SOURCES: clear;x-gha,readwrite
    ACTIONS_CACHE_URL: https://artifactcache.actions.githubusercontent.com/eCNKhPUDWGfA9dRE0ONjnDhJT6Cox4Si9IDW6q4SzQPjweOW6i/
    ACTIONS_RUNTIME_TOKEN: ***
    RUNVCPKG_VCPKG_ROOT: D:\a\openvpn-gui\b\vcpkg
    VCPKG_ROOT: D:\a\openvpn-gui\b\vcpkg
[...]
Running command '"D:\a\_temp\740461635\cmake-3.26.4-windows-x86_64\bin\cmake.exe"' with args '^"--preset^",^"x64-release^"' in current directory 'D:\a\openvpn-gui\openvpn-gui'.
Preset CMake variables:

  CMAKE_BUILD_TYPE="Release"
  CMAKE_TOOLCHAIN_FILE:FILEPATH="C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\vcpkg/scripts/buildsystems/vcpkg.cmake"
lukka commented 1 year ago

@flichtenheld that is right, I spotted the issue and pushed again onto run-cmake@main, and also tested it so now it must be working. Let me know.

flichtenheld commented 1 year ago

@flichtenheld that is right, I spotted the issue and pushed again onto run-cmake@mgail, and also tested it so now it must be working. Let me know.

I assume you mean run-cmake@main. Can confirm that my build now succeeds. (e.g. https://github.com/flichtenheld/openvpn-gui/actions/runs/5278823831)

lukka commented 1 year ago

@bwrsandman @flichtenheld thank you again for helping solving this issue!

The problem is fixed in run-cmake@v10 with the release of run-cmake@v10.6.

flichtenheld commented 1 year ago

@lukka Note that the v10 branch doesn't seem to be updated, yet. So run-cmake@v10.6 works, but run-cmake@v10 doesn't, yet.