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

[Windows] Add VCPKG_ROOT variable #6192

Merged
merged 2 commits into from
Sep 8, 2022

Conversation

erik-bershel
Copy link
Contributor

@erik-bershel erik-bershel commented Sep 7, 2022

Description

Add VCPKG_ROOT variable in additional to VCPKG_INSTALLATION_ROOT + simple style fix

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 changed the title [Windows] Added VCPKG_ROOT variable [Windows] Add VCPKG_ROOT variable Sep 7, 2022
@erik-bershel erik-bershel requested review from miketimofeev and a team September 7, 2022 14:24
@nightlark
Copy link

nightlark commented Sep 16, 2022

This change broke several of our CI builds on GitHub Actions and Azure Pipelines (we use the presence of VCPKG_ROOT being set to indicate that a user wants to opt-in to building dependencies with vcpkg rather than ones installed on the system). Some form of announcement about this change would have been nice.

  • Windows builds on Azure Pipelines started timing out: installing the Boost packages takes an excessively long time, given we only use the headers (last I checked there wasn't a vcpkg for grabbing just the headers and not compile anything), adding about 16 minutes to the overall build time
  • MSYS2 builds for our MINGW package started failing (conflict between the vcpkg version of Boost and the mingw copy? not sure)

To get things working again our solution has been to liberally add unset VCPKG_ROOT to our CI workflows and a new option for our CMake builds that forces it to not use vcpkg even if VCPKG_ROOT is set.

@ddobranic
Copy link
Contributor

@nightlark thank you for the feedback and workaround! we will investigate the issue.

@miketimofeev
Copy link
Contributor

This change broke several of our CI builds on GitHub Actions and Azure Pipelines (we use the presence of VCPKG_ROOT being set to indicate that a user wants to opt-in to building dependencies with vcpkg rather than ones installed on the system). Some form of announcement about this change would have been nice.

  • Windows builds on Azure Pipelines started timing out: installing the Boost packages takes an excessively long time, given we only use the headers (last I checked there wasn't a vcpkg for grabbing just the headers and not compile anything), adding about 16 minutes to the overall build time
  • MSYS2 builds for our MINGW package started failing (conflict between the vcpkg version of Boost and the mingw copy? not sure)

To get things working again our solution has been to liberally add unset VCPKG_ROOT to our CI workflows and a new option for our CMake builds that forces it to not use vcpkg even if VCPKG_ROOT is set.

@michaelbprice could you please comment on this? It seems it's not so safe to add that variable

@michaelbprice
Copy link

I've left a more detailed comment over at #6196 (comment).

We'll see how this shakes out, but it seems to me that if tools expect to use VCPKG_ROOT as an indication of where vcpkg is installed, it should not also serve the purpose of an opt-in, unless that opt-in is strictly predicated on the tool being installed in such an identifiable way.

My recommendation would be to change that opt-in behavior to depend on an environment variable more clearly within the control of the users' environment, by doing something like prepending an org identifier to the environment variable, such as MY_AWESOME_PROJECT_USING_VCPKG_ROOT.

@michaelbprice
Copy link

@michaelbprice could you please comment on this? It seems it's not so safe to add that variable

In the sense that it's never safe to add any environment variable, as you cannot predict if someone downstream might already be using it for some other purpose.

MehdiChinoune added a commit to MehdiChinoune/MINGW-packages that referenced this pull request Oct 2, 2022
Recently GHA started defining VCPKG_ROOT environment variable (actions/runner-images#6192).
So unset it to not let it autodetected and interfere with our build jobs.
MehdiChinoune added a commit to msys2/MINGW-packages that referenced this pull request Oct 2, 2022
Recently GHA started defining VCPKG_ROOT environment variable (actions/runner-images#6192).
So unset it to not let it autodetected and interfere with our build jobs.
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.

6 participants