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

[Ubuntu] Add VCPKG_ROOT variable #6196

Merged
merged 3 commits into from
Sep 12, 2022

Conversation

erik-bershel
Copy link
Contributor

Description

Add VCPKG_ROOT variable in additional to VCPKG_INSTALLATION_ROOT

Related issue:

#4241

Check list

  • Related issue / work item is attached
  • Tests are written (if applicable)
  • Documentation is updated (if applicable)
  • Changes are tested and related VM images are successfully generated

@erik-bershel erik-bershel requested a review from a team September 7, 2022 14:32
ddobranic
ddobranic previously approved these changes Sep 7, 2022
@ddobranic
Copy link
Contributor

ddobranic commented Sep 7, 2022

/azp run ubuntu2004, ubuntu2204

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@BillyONeal
Copy link

This breaks vcpkg customers who use vcpkg as a submodule. Can you explain why this variable is being set?

BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Sep 23, 2022
@al-cheb
Copy link
Contributor

al-cheb commented Sep 23, 2022

@BillyONeal, we got a request to add this variable #6146. @michaelbprice, Could you please provide more context?

@michaelbprice
Copy link

There are at least 3 techniques being used in the wild to acquire vcpkg.

  1. Included as a git submodule, which is then acquired/updated using git's submodule commands.
  2. Downloaded via a one-line shell command (as shown in https://devblogs.microsoft.com/cppblog/vcpkg-environment-activation-in-visual-studio/)
  3. Pre-installed on the system via some tool (e.g. container image).

Under all of these scenarios, how should we expect tools to invoke vcpkg? The options as I see them:

  1. vcpkg is on the PATH environment variable OR
  2. Invocation should be through a rooted, non-relative path that is provided to the tool.
  3. There exists some conventional environment variable, such as VCPKG_ROOT, at which vcpkg could be located.

For option 3), it would be unwise to have that conventional environment variable differ according to the technique that was used to obtain the vcpkg tool, which was the current case in the GitHub Actions containers, where VCPKG_INSTALLATION_ROOT was used instead of VCPKG_ROOT. Furthermore, any users who are techniques 1) or 2) to obtain vcpkg should be prepared to deal with systems where it already exists via technique 3), as it will be a very common way of obtaining vcpkg as soon as it begins being shipped more widely by other tools.

Another issue was identified where a user was using the presence of VCPKG_ROOT as an opt-in signal (see #6192 (comment)), which is the sort of situation of not having an agreed-upon convention has caused. Users should have a single environment variable whose sole purpose is to provide the location of the tool and should not be relied upon for other purposes (such as the aforementioned opt-in, unless it really is tied to the availbility of the tool).


If we decide that we do not want to support option 3) for identifying the location of the tool, then we should make it clear that no one should rely upon such an environment variable being defined and that they should instead use options 1) or 2).

@BillyONeal
Copy link

VCPKG_ROOT is the location of:

(1) the location of the built in registry, and
(2) the default root location for some of the other values, like the installed package root.

It defaults to the directory "above" the vcpkg binary with the file .vcpkg-root.

It was added because once upon a time, the canonical way to use vcpkg required building the vcpkg binary, and people asked for some way to avoid needing to rebuild the executable over and over, as that took significant time. So you could build / download the executable once, and reuse it with different "instances" of vcpkg.

In classic mode, it, in effect, controls the versions of all of the packages that one can install. A CI system setting this is, in effect, saying "to hell with the versions the project being built asked for, you are getting the system versions".

There are at least 3 techniques being used in the wild to acquire vcpkg.

  1. Included as a git submodule, which is then acquired/updated using git's submodule commands.

In this case, the default VCPKG_ROOT will already be chosen correctly. VCPKG_ROOT being set by a CI system is unexpected and breaks these customers because they get incorrect versions of packages, and the packages are installed in the wrong place.

  1. Downloaded via a one-line shell command (as shown in https://devblogs.microsoft.com/cppblog/vcpkg-environment-activation-in-visual-studio/)

In this case, vcpkg is being installed into the shell in experimental support of the "artifacts" feature, where vcpkg needs to be able to reach into the outer shell and set environment variables. VCPKG_ROOT will be set by that installation script to the extracted vcpkg binary, but this way of deploying vcpkg doesn't work in classic mode, so the problem of that variable controlling versions of everything is not a problem.

  1. Pre-installed on the system via some tool (e.g. container image).

This looks like case 1.

In all 3 cases, the expected way the user calls us is via PATH (or by equivalent shell function in case 2).

Under all of these scenarios, how should we expect tools to invoke vcpkg? The options as I see them:

  1. vcpkg is on the PATH environment variable OR
  2. Invocation should be through a rooted, non-relative path that is provided to the tool.
  3. There exists some conventional environment variable, such as VCPKG_ROOT, at which vcpkg could be located.

The vast vast majority of the time, it should be (1).

For option 3), it would be unwise to have that conventional environment variable differ according to the technique that was used to obtain the vcpkg tool, which was the current case in the GitHub Actions containers, where VCPKG_INSTALLATION_ROOT was used instead of VCPKG_ROOT. Furthermore, any users who are techniques 1) or 2) to obtain vcpkg should be prepared to deal with systems where it already exists via technique 3), as it will be a very common way of obtaining vcpkg as soon as it begins being shipped more widely by other tools.

There is no such "conventional environment variable". I don't know if one is even possible because the different deployment mechanisms have intentionally different behavior; the "one liner" installer was added as an experiment and we need to take a long hard look at all of our configuration before taking the experimental tag off. The other methods reduce to PATH.

BillyONeal added a commit to microsoft/vcpkg that referenced this pull request Sep 23, 2022
* Version lock the github actions actions.

* Work around actions/runner-images#6196
@erik-bershel
Copy link
Contributor Author

@BillyONeal @michaelbprice
So, I would like to draw a line three weeks after the breaking changes. What is the reaction in the vcpkg community? May we continue the deprecation process for the VCPKG_INSTALLATION_ROOT variable, or do you recommend reverting to using it?

@BillyONeal
Copy link

We are still getting constant reports of people broken by this setting and still do not understand why GitHub Actions is setting it. We are also considering product changes to explicitly ignore the setting with a warning because CI systems keep doing this to us.

BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Oct 5, 2022
This resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1634907

In actions/runner-images#6196 we have yet another CI system that is trying to helpfully set VCPKG_ROOT for customers to find their vcpkg instance which yet again breaks classic-mode-as-submodule customers. This change mitigates the issue by ignoring the environment variable when we detect a valid VCPKG_ROOT, which the user can override by using the command line option.
@michaelbprice
Copy link

Mea culpa

After some deliberation, my understanding of the role of the VCPKG_ROOT environment variable was incorrect. That incorrect understanding arose from difficulties I was seeing when trying to directly run the vcpkg tool that is pre-installed on the Actions systems within my workflow, which seemed to require that VCPKG_ROOT be defined. I can work on putting together an example of the issue that I had seen, but it'll take me a while to get around to it. It's important to me that if the systems have a pre-installed vcpkg, that it should work out-of-the-box.

@erik-bershel - I think that due to the disruptions the change has caused, the addition of VCPKG_ROOT environment variable to the Actions systems should be backed out.

BillyONeal added a commit to microsoft/vcpkg-tool that referenced this pull request Oct 11, 2022
…t. (#725)

* Remove unique_ptr from VcpkgCmdArguments.

* Ignore the VCPKG_ROOT environment variable when we detect a vcpkg_root.

This resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1634907

In actions/runner-images#6196 we have yet another CI system that is trying to helpfully set VCPKG_ROOT for customers to find their vcpkg instance which yet again breaks classic-mode-as-submodule customers. This change mitigates the issue by ignoring the environment variable when we detect a valid VCPKG_ROOT, which the user can override by using the command line option.

* Fix disable-metrics test and echo tool output.

* Fix imprecise env-passthrough test.
@BillyONeal
Copy link

Our workaround for this in the vcpkg product just landed today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants