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

[vcpkg] use VCPKG_ROOT_DIR instead of DOWNLOADS for WORKING_DIRECTORY in do_version_check #15299

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

lebdron
Copy link
Contributor

@lebdron lebdron commented Dec 25, 2020

Add check for downloads directory existence in do_version_check to prevent No such file or directory error in execute_process.

@JackBoosY JackBoosY self-assigned this Dec 28, 2020
@JackBoosY JackBoosY added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Dec 28, 2020
vcpkg_execute_in_download_mode(
COMMAND ${${VAR}} ${VERSION_CMD}
WORKING_DIRECTORY ${DOWNLOADS}
${WORKING_DIRECTORY_ARG}
Copy link
Member

Choose a reason for hiding this comment

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

The correct response to "the downloads directory doesn't exist" is "create the downloads directory", not "dump the files in some random place wherever". Do you have a repro for this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. On arm64-osx, I install all the required tools - cmake and ninja, prior to any operations, run bootstrap with useSystemBinaries and allowAppleClang, then try to build some packages, and when it tries to test the version of system-installed ninja, it fails on this step, as nothing was downloaded, and downloads directory does not exist.

Copy link
Member

Choose a reason for hiding this comment

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

I see, the repro is arm64-osx.

To be clear, I'm not saying "it's impossible for this to happen", I'm saying "this is not a correct fix for the problem".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reproduced this on x64-linux as well. The thing is that it silently fails:

[DEBUG] -- Found ninja('') but at least version 1.10.1 is required! Trying to use internal version if possible!                                                                                                     
[DEBUG] -- Downloading https://github.com/ninja-build/ninja/releases/download/v1.10.1/ninja-linux.zip...

But because it downloads the binary for correct architecture, further build does not fail. This is not the case with arm64, however.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think that "dump the files in some random place wherever" is not the exact description of the issue. do_version_check simply runs /usr/bin/ninja --version, for example, and the directory it is running this command from does not seem to be a big issue.

Copy link
Member

Choose a reason for hiding this comment

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

The presence of the existing working directory setting strongly indicate that it was relevant for at least one program.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a high probability that the vcpkg_execute_in_download_mode command was just copied from msiexec call and having the call be inside a vcpkg dir seemed to be safe. Using VCPKG_ROOT or BUILDTREE_DIR seems also be reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let it be VCPKG_ROOT then

@lebdron
Copy link
Contributor Author

lebdron commented Jan 5, 2021

Ping @BillyONeal . Are you sure that working directory is required at all? CMake uses absolute paths for executables here.

@lebdron
Copy link
Contributor Author

lebdron commented Jan 8, 2021

I would also ping @Neumann-A , as the macro was introduced here #12895 and not changed much since then.

… in do_version_check

Signed-off-by: Andrei Lebedev <lebdron@gmail.com>
@lebdron lebdron force-pushed the fix-find-acquire-program branch from 1033f40 to bd5ac3f Compare January 11, 2021 15:24
@lebdron lebdron changed the title [vcpkg] Check if downloads directory exists in do_version_check [vcpkg] use VCPKG_ROOT_DIR instead of DOWNLOADS for WORKING_DIRECTORY in do_version_check Jan 11, 2021
@lebdron lebdron requested a review from BillyONeal January 11, 2021 15:24
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I did some testing to make sure this didn't edit anything in the root dir.

@BillyONeal BillyONeal added the info:reviewed Pull Request changes follow basic guidelines label Jan 11, 2021
@dan-shaw dan-shaw merged commit 557ecbe into microsoft:master Jan 13, 2021
@lebdron lebdron deleted the fix-find-acquire-program branch January 13, 2021 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants