Skip to content

Fix working directory for archiving from git. #21166

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

Merged
BillyONeal merged 2 commits into from
Nov 26, 2021
Merged

Fix working directory for archiving from git. #21166

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8997c89
Fix working directory for archiving from git.
mahge Nov 3, 2021
488aa93
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Nov 23, 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
4 changes: 2 additions & 2 deletions scripts/cmake/vcpkg_from_git.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function(vcpkg_from_git)
if(DEFINED arg_TAG)
message(WARNING "The TAG argument to vcpkg_from_git has been deprecated and has no effect.")
endif()


if(NOT DEFINED arg_OUT_SOURCE_PATH)
message(FATAL_ERROR "OUT_SOURCE_PATH must be specified")
Expand Down Expand Up @@ -164,7 +164,7 @@ function(vcpkg_from_git)
vcpkg_execute_required_process(
ALLOW_IN_DOWNLOAD_MODE
COMMAND "${GIT}" archive "${rev_parse_ref}" -o "${temp_archive}"
WORKING_DIRECTORY "${DOWNLOADS}/git-tmp"
WORKING_DIRECTORY "${git_working_directory}"
Copy link
Contributor

@JackBoosY JackBoosY Nov 4, 2021

Choose a reason for hiding this comment

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

Can you tell me which issue you solved here?

Copy link
Contributor Author

@mahge mahge Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Of course. This issue is with head (--head) mode operation.

It starts here. The directory where the script wants to operate in, i.e git_working_directory, is set to ${DOWNLOADS}/git-tmp.

vcpkg/scripts/cmake/vcpkg_from_git.cmake

Line 82 in 40a0fb2

set(git_working_directory "${DOWNLOADS}/git-tmp")

However, if you have specified head mode (--head) then the code changes that directory to "${CURRENT_BUILDTREES_DIR}/src/git-tmp" here

vcpkg/scripts/cmake/vcpkg_from_git.cmake

Lines 83 to 88 in 40a0fb2

if(VCPKG_USE_HEAD_VERSION)
if(DEFINED arg_HEAD_REF)
vcpkg_list(SET working_directory_param "WORKING_DIRECTORY" "${CURRENT_BUILDTREES_DIR}/src/head")
vcpkg_list(SET git_fetch_shallow_param --depth 1)
set(ref_to_use "${arg_HEAD_REF}")
set(git_working_directory "${CURRENT_BUILDTREES_DIR}/src/git-tmp")

Which means the port will be fetched to that directory here

vcpkg/scripts/cmake/vcpkg_from_git.cmake

Lines 117 to 128 in 40a0fb2

vcpkg_execute_required_process(
ALLOW_IN_DOWNLOAD_MODE
COMMAND "${GIT}" init "${git_working_directory}"
WORKING_DIRECTORY "${CURRENT_BUILDTREES_DIR}"
LOGNAME "git-init-${TARGET_TRIPLET}"
)
vcpkg_execute_required_process(
ALLOW_IN_DOWNLOAD_MODE
COMMAND "${GIT}" fetch "${arg_URL}" "${ref_to_use}" ${git_fetch_shallow_param} -n
WORKING_DIRECTORY "${git_working_directory}"
LOGNAME "git-fetch-${TARGET_TRIPLET}"
)

The script then tries to archive the fetched code (for caching). However, the archiving custom command was still assuming the fetch was always done in ${DOWNLOADS}/git-tmp.

vcpkg/scripts/cmake/vcpkg_from_git.cmake

Lines 163 to 169 in 40a0fb2

file(MAKE_DIRECTORY "${DOWNLOADS}/temp")
vcpkg_execute_required_process(
ALLOW_IN_DOWNLOAD_MODE
COMMAND "${GIT}" archive "${rev_parse_ref}" -o "${temp_archive}"
WORKING_DIRECTORY "${DOWNLOADS}/git-tmp"
LOGNAME git-archive
)

This was failing because there is nothing in ${DOWNLOADS}/git-tmp. My guess is that it was just overlooked among the changes in #19338. I changed it to use the git_working_directory variable instead. This way it works for both modes.

I am not sure why the two operation modes (normal vs head) use two different temporary directories. Maybe that was overlooked. But I thought this fix covers both cases without big changes.

Copy link
Member

@BillyONeal BillyONeal Nov 22, 2021

Choose a reason for hiding this comment

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

I agree with @mahge -- this is once again a case the GitHub code review tool makes terrible. Consider existing uses on 126 and 149 that the terrible code review tool isn't showing as contest.

LOGNAME git-archive
)
file(RENAME "${temp_archive}" "${archive}")
Expand Down