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.

Sign up for GitHub

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

Jump to bottom

cmake(install): include vcpkg dlls #2971

Merged
merged 2 commits into from
Jan 21, 2021
Merged

cmake(install): include vcpkg dlls #2971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
897dd0f
cmake(install): include vcpkg dlls
dennisameling Jan 8, 2021
9d4e28c
GitHub Actions: Add vcpkg initialization step
dennisameling Jan 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .github/workflows/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ jobs:
vs-build:
needs: ci-config
if: needs.ci-config.outputs.enabled == 'yes'
timeout-minutes: 15
env:
MSYSTEM: MINGW64
NO_PERL: 1
Expand All @@ -188,6 +189,11 @@ jobs:
## Unzip and remove the artifact
unzip artifacts.zip
rm artifacts.zip
- name: initialize vcpkg
uses: actions/checkout@v2
with:
repository: 'microsoft/vcpkg'
path: 'compat/vcbuild/vcpkg'
- name: download vcpkg artifacts
shell: powershell
run: |
Expand Down
4 changes: 4 additions & 0 deletions contrib/buildsystems/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ if(WIN32)

# In the vcpkg edition, we need this to be able to link to libcurl
set(CURL_NO_CURL_CMAKE ON)

# Copy the necessary vcpkg DLLs (like iconv) to the install dir
set(X_VCPKG_APPLOCAL_DEPS_INSTALL ON)
set(CMAKE_TOOLCHAIN_FILE ${VCPKG_DIR}/scripts/buildsystems/vcpkg.cmake CACHE STRING "Vcpkg toolchain file")
Copy link
Member

Choose a reason for hiding this comment

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

This causes the build to fail:

 CMake Error at C:/Program Files/CMake/share/cmake-3.19/Modules/CMakeDetermineSystem.cmake:110 (message):
  Could not find toolchain file:
  D:/a/git/git/contrib/buildsystems/../../compat/vcbuild/vcpkg/scripts/buildsystems/vcpkg.cmake

Copy link
Author

@dennisameling dennisameling Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It works locally, so something is missing in your vcpkg artifacts in GH Actions. I just checked:

Downloaded https://dev.azure.com/git/4c888a9b-b30a-4c62-9301-8dfa18a71976/_apis/build/builds/2101/artifacts?artifactName=compat&api-version=6.0&%24format=zip

The scripts folder is missing, which causes the error:

image

Could you please include vcpkg/scripts/buildsystems/vcpkg.cmake in your vckpg artifacts, @dscho? That should resolve the issue 👍🏼

The alternative is that we could checkout vcpkg in GH Actions and then download your artifact with the prebuilt DLLs.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm. I did that, and now the vs-build jobs take longer than even the linux-gcc build (which builds Git on Linux and then runs the test suite twice). To test the hypothesis that this is triggered by including scripts, I started https://github.com/dscho/git/actions/runs/474627271. Let's see how that fares. If its vs-build jobs also take excruciatingly long, I will have to roll back that scripts change.

Copy link
Member

Choose a reason for hiding this comment

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

Nope, seems that https://github.com/dscho/git/runs/1674724825?check_suite_focus=true was done with the generate Visual Studio solution step within 1.5 minutes, but https://github.com/git-for-windows/git/pull/2971/checks?check_run_id=1674658393 has now been running for more than 30 minutes.

Copy link
Author

Choose a reason for hiding this comment

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

That's weird. Looks like it just wasn't doing anything for 6 (!) hours. It's probably better if we first clone vcpkg in CI and then download your artifacts over it. I'll try that tomorrow evening if I can make it work time-wise. To be continued 👍🏼

Copy link
Member

Choose a reason for hiding this comment

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

I don't even know what purpose this toolchain file serves.

Copy link
Author

Choose a reason for hiding this comment

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

I tried without the CMAKE_TOOLCHAIN_FILE but then it doesn't copy over the DLLs. So it is needed. But I was able to fix CI in d8cf02e, how does that look @dscho? 😊

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't that take super long because it rebuilds vcpkg.exe every single time? I'd rather have a manual copy step, using constants defined by find_package(), if that is at all possible.

Copy link
Author

Choose a reason for hiding this comment

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

1m51, that is: https://github.com/git-for-windows/git/pull/2971/checks?check_run_id=1687684942. But I see that vcpkg.exe is also included in your artifact, so let me try to exclude the vcpkg.exe build step and see if that works

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 09d1573. Now the new step initialize vcpkg only takes 6s 👍🏼

endif()

find_program(SH_EXE sh PATHS "C:/Program Files/Git/bin")
Expand Down