Vcpkg root fix

[daschuer]

Use the VCPKG_ROOT environment variable only on windows. This fixes a build error on the GitHub Ubuntu 22.4 workflow runner which has comes with VCPKG preinstalled.

See: actions/runner-images@07e2db9

[Holzhaus]

I think the proper fix would be to unset VCPKG_ROOT in the GitHub workflow file (or set it to empty string), not hack around in our cmakelists. If someone really wants to use vcpkg on Linux, I see no reason why we should disallow it.

[daschuer]

If you like to use VCPKG on Linux you can still do this by cmake -DVCPKG_ROOT=<path>.

This way it happens explicit and not randomly only because someone has the environment variable VCPKG_ROOT set for another project. Finding out the root cause in this case took me many hours and I want to avoid do this ever again.

[daschuer]

I have also considered to not at all depend on ENV{VCPKG_ROOT} but that will require every contributor to change their build scripts. Let's avoid that.

[Holzhaus]

I have also considered to not at all depend on ENV{VCPKG_ROOT} but that will require every contributor to change their build scripts. Let's avoid that.

I agree that this would be unfortunate. On the other hand, the current solution is pretty surprising, because it magically picks up the VCPKG_ROOT from the environment except on Linux.

To keep stuff consistent across all OSes while still avoiding changing build setups, I'd rather override the env var on CI and leave the CMakeLists.txt untouched (or maybe print the vcpkg root explicitly if picked from the environment to make this more obvious and easier to debug).

[Holzhaus]

Let's see if this would even work. I pushed a POC here: https://github.com/Holzhaus/mixxx/tree/vcpkg-ci-fix

[daschuer]

For me it is not clear which use case you are targeting with your proposal. Please explain.

Your solution unsettling VCPKG_ROOT works only on the GitHub workflow runner, but not at home.

I think a default cmake .. call should always either behave like the user expects from cmake or default to our supported configuration. We do currently the later. Part of it is to pick up VCPKG_ROOT on Windows and MacOs but not on Linux.

When I read this https://vcpkg.io/en/getting-started.html, it is obvious mandatory that the user has to set
-DCMAKE_TOOLCHAIN_FILE=[path to vcpkg]/scripts/buildsystems/vcpkg.cmake
and not the VCPKG_ROOT environment variable.

So it was probably wrong to make the whole block depending on VCPKG

I think we should make sure this works on Linux as well and we are in a good shape.
What do you think?

[daschuer]

This PR also fixes an issue when you switch from main to the 2.3 branch on macOS.
After running tools/macos_buildenv.sh on main the VCPK_ROOT variable is set, so after switching to 2.3 the main environment picked and it becomes a vcpkg build.

[daschuer]

After the latest commit, we can build with any vcpkg installation via
cmake -DCMAKE_TOOLCHAIN_FILE=../../vcpkg/scripts/buildsystems/vcpkg.cmake
as is is decribed here https://vcpkg.io/en/getting-started.html
setting VCPKG_ROOT is not required.

I have tested this here successfully with Ubuntu 20.4 and libmad and libopus installed with vcpkg see:

-- Looking for X509_check_host - not found
-- Found Opus: /home/daniel/workspace/vcpkg/installed/x64-linux/debug/lib/libopus.a  
-- Found MAD: /home/daniel/workspace/vcpkg/installed/x64-linux/debug/lib/libmad.a  
-- Found ID3Tag: /usr/lib/x86_64-linux-gnu/libid3tag.so  
-- Found Modplug: /usr/lib/x86_64-linux-gnu/libmodplug.so  

[Holzhaus]

My main issue with your PR is that cmake picks up VCPKG_ROOT from the environment on Windows, but not on the OSes.

From the user's perspective this is surprising and there is no obvious reason for this - in fact the only reason for this is that our CI setup uses vcpkg on only Windows, which is pure coincidence.

I'd really like to avoid accumulating non-obvious special cases on the different OS which our users/contributors have to remember, especially when there is no actual reason for this.

If we want to keep this consistent, which is what I'm advocating for, we have 2 options:

1. Remove configuration from env vars entirely. This requires all users to pass the cmake toolchain file explicitly, which requires users to change the way how they build Mixxx at the moment.
2. Use the same env vars for configuration on all OSes, not just windows. This would retain backwards compatibility.

As you pointed out, breaking the build setups for our contributors would be unfortunate, so I'd suggest we use the latter approach.

This PR also fixes an issue when you switch from main to the 2.3 branch on macOS.
After running tools/macos_buildenv.sh on main the VCPK_ROOT variable is set, so after switching to 2.3 the main environment picked and it becomes a vcpkg build.

Good point, we should unset that variable in the 2.3 buildenv script.

[daschuer]

Since we have not yet a real use case the discussion about whether picking up is surprising or not, it is kind of virtual. It is obvious that we must not pick up the environment variable of the system installed vcpkg of GitHub.

That this exists, proves the assumption that the purpose of the VCPKG_ROOT variable is to find the system wide installation.

It can be surprising if we pick it up on Linux since it is not supported in the same way as if we pick it not up because we do on Windows. The original intention matters. We have the following imaginative users:

• mixxxdj/vcpkg on Linux: They might be surprised that VCPKG_ROOT is not picked up.
  Workaround: cmake -DCMAKE_TOOLCHAIN_FILE...
• System wide VCPKG_ROOT on Linux: They might be surprised that VCPKG_ROOT is picked up.
  Workaround: env -u VCPKG_ROOT cmake ...

Interestingly our non standard approach dealing with ENV{VCPKG_ROOT} is only required for manifest mode which does not support overlays. microsoft/vcpkg#12289

This gives me the idea how to help both. We can check whether the overlay folders exist. If not it is likely not our vcpkg tree and using VCPKG_ROOT surprising and unnecessary anyway.

@Holzhaus Does this work for you?

[daschuer]

It works. Now a plain cmake .. has again the right defaults without surprises.

[daschuer]

@Holzhaus can you have another look.

[Holzhaus]

Thanks for looking into this, unfortunately the proposed solution still suffers from the same underlying issue. Now setting VCPKG_ROOT from env works if and only if the VCPK_ROOT directory coincidentally has an overlay/ports directory - if it hasn't then it stops working. Thus these parts from my previous comment still apply:

From the user's perspective this is surprising and there is no obvious reason for this - in fact the only reason for this is that our CI setup uses vcpkg on only Windows has an overlay/ports directory, which is pure coincidence.

I'd really like to avoid accumulating non-obvious special cases on the different OS which our users/contributors have to remember, especially when there is no actual reason for this.

Let's take a step back and reassess what the actual root problem that we are trying to solve is:

• We set the VCPKG_ROOT env var in out buildenv script and read it in CMakeLists.txt to pass a path between those two contexts
• GitHub's default virtual environments now also set this variable, which conflics with our usage of that variable

We could unset the variable in the GitHub virtual environments (I proposed this here: #10904 (comment)), but you correctly pointed out that this would not fix if someone has the variable set outside of the CI context.

The only reason why this happens is that we use the VCPKG_ROOT variable for something that it was not originally intended for. We cannot get rid of the env variable configurtation altogether because it would break user's build setups and would also require OS-specific build instructions ("if you're using windows, you also need to pass -DCMAKE_TOOLCHAIN_FILE=... to CMake...") which is something we'd like to avoid.

As I explained above, I also don't think we should attempt to do heuristics to detect if the variable is set to an "expected" value (i.e. points to *our custom vcpkg root instead of a different one) as you proposed.

I thought some more about this, and since we control both setting the variable from our buildenv script and reading the variable from CMake, we can just use a different environment variable (e.g., MIXXX_VCPKG_ROOT or MIXXX_CMAKE_TOOLCHAIN_FILE or something like that). This would solve the issue on both GitHub Actions and on our contributor's machines, because it's quite unlikely that this variable is set by someone else than us, and also don't need some heuristcs or special cases anymore.

What do you think?

[daschuer]

Good idea

[Holzhaus]

Pre-commit fails due to trailing whitespace.

[Holzhaus]

Pre-commit still complains about trailing whitespace.

[Holzhaus]

this may have slipped in by mistake, or is this intentional?

[Holzhaus]

LGTM except for the minor issues I pointed out in my review. Thanks!

[daschuer]

Thank you for review. The remaining issues are now fixed and squashed into the last commit.

[Holzhaus]

LGTM, thank you.

[Holzhaus]

@daschuer can you take care of merging to main? If not, I'll have a look tomorrow.

[daschuer]

Done, thank you.