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

Fix working directory for archiving from git. #21166

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

mahge
Copy link
Contributor

@mahge mahge commented Nov 3, 2021

Describe the pull request
Fix the working directory where git archive is issued in (for vcpkg_from_git)

  • What does your PR fix?

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    Not applicable

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Not applicable

  - The directory specified no longer matches the directory used for
    fetching.

  - This was overlooked by PR microsoft#19338.
@@ -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

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

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.

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

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_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.

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

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.

@JackBoosY JackBoosY added requires:author-response category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:author-response labels Nov 4, 2021
@JackBoosY
Copy link
Contributor

@strega-nil-ms @vicroms can you please take a look?

Thanks.

@@ -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
Member

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.

@JackBoosY
Copy link
Contributor

Depends on #21613.

@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Nov 23, 2021
@JackBoosY
Copy link
Contributor

The qca regression was fixed by #21250.

@BillyONeal BillyONeal added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) and removed depends:different-pr This PR or Issue depends on a PR which has been filed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly labels Nov 23, 2021
@mahge mahge requested a review from JackBoosY November 24, 2021 19:19
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 25, 2021
@BillyONeal BillyONeal merged commit 446f0d9 into microsoft:master Nov 26, 2021
@BillyONeal
Copy link
Member

Thanks for the typo fix!

@mahge mahge deleted the fix_archive_working_dir branch November 28, 2021 17:03
@mahge
Copy link
Contributor Author

mahge commented Nov 28, 2021

No problem. Thanks for accepting it.

I think it might be good to add at least one test that uses vcpkg_from_git in --head mode, when you have time of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants