-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
CMake: fail early when MIXXX_VCPKG_ROOT is invalid #15899
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
base: main
Are you sure you want to change the base?
CMake: fail early when MIXXX_VCPKG_ROOT is invalid #15899
Conversation
|
Welcome at Mixxx! |
daschuer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. This looks already helpful.
Some tweaking is required to make it more robust. Se my comments.
CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| # Validate MIXXX_VCPKG_ROOT points to a vcpkg root to avoid confusing follow-up errors. | ||
| set(_vcpkg_toolchain "${MIXXX_VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MIXXX_VCPKG_ROOT is already used above. Why does it not fail? Why does the error above not kick in? I think this first sanity check need to be moved above.
CMakeLists.txt
Outdated
| set(_vcpkg_toolchain "${MIXXX_VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake") | ||
| if(NOT EXISTS "${_vcpkg_toolchain}") | ||
| message(FATAL_ERROR | ||
| "MIXXX_VCPKG_ROOT was set to \"${MIXXX_VCPKG_ROOT}\", but this is not a valid vcpkg root\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message in the PR description uses different path separators / . CMake uses internally the unix / separator. I am not on windows. Does it work with both? At least the output looks visually wrong. Please verify where this happens.
CMakeLists.txt
Outdated
| message(FATAL_ERROR | ||
| "MIXXX_VCPKG_ROOT was set to \"${MIXXX_VCPKG_ROOT}\", but this is not a valid vcpkg root\n" | ||
| "Expected to find: ${_vcpkg_toolchain}\n" | ||
| "Please set MIXXX_VCPKG_ROOT to the vcpkg root directory (the folder containing vcpkg.exe)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vcpkg.exe is the windows file, on macOs it is just vcpkg without .exe.
Maybe something like this:
| "Please set MIXXX_VCPKG_ROOT to the vcpkg root directory (the folder containing vcpkg.exe)" | |
| "Please set MIXXX_VCPKG_ROOT to the vcpkg root directory, the folder containing the vcpkg tool (vcpkg.exe on Windows)." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the end you may also call "fatal_error_missing_env" for extra info like above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed updates addressing review comments:
- moved the toolchain existence check to the top of the MIXXX_VCPKG_ROOT block
- normalized the displayed expected path on Windows via file(TO_NATIVE_PATH ...)
- made wording cross-platform in status message
- used message(STATUS ...) and fatal_error_missing_env()
Yes, I have agreed to the Mixxx Contributor Agreement. Thanks! |
CMakeLists.txt
Outdated
| if(DEFINED MIXXX_VCPKG_ROOT) | ||
| # Validate MIXXX_VCPKG_ROOT points to a vcpkg root to avoid confusing follow-up errors. | ||
| set(_vcpkg_toolchain "${MIXXX_VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake") | ||
| file(TO_NATIVE_PATH "${_vcpkg_toolchain}" _vcpkg_toolchain_native) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement can be moved below into the if scope. Saves a bit CPU in the good case.
|
Hi @daschuer, |
Fixes #15016
Summary
Add an early CMake validation for
MIXXX_VCPKG_ROOTto avoid confusing follow-up errors when contributors select an incorrect vcpkg root directory.When
MIXXX_VCPKG_ROOTis set but${MIXXX_VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmakeis missing, configuration now fails immediately with a clear message explaining what is wrong and how to fix it.Testing
cmake -S . -B build15016 -DMIXXX_VCPKG_ROOT="C:\Windows"Output