-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Move download, SHA256 check and unzip of Windows buildenv from windows_buildenv.bat to CMakelists.txt #13306
base: main
Are you sure you want to change the base?
Move download, SHA256 check and unzip of Windows buildenv from windows_buildenv.bat to CMakelists.txt #13306
Conversation
22b1df5
to
ecc462a
Compare
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 for adopting this.
9cea1ee
to
b09ace0
Compare
@daschuer Friendly Ping |
@daschuer Can we merge this? |
@@ -47,6 +47,68 @@ if(POLICY CMP0135) | |||
cmake_policy(SET CMP0135 NEW) | |||
endif() | |||
|
|||
|
|||
if(WIN32) |
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.
I know you mentioned it as out-of-scope, but I agree that sharing this logic with macOS would be great, given that the vcpkg envs are structured practically identically as far as I understand.
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.
It would be great if you could create a follow-up PR for macOS.
@daschuer Can we merge this now? |
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 only works if we make sure
"MIXXX_VCPKG_ROOT = BUILDENV_BASEPATH/BUILDENV_NAME"
What are the use cases if this is not the case?
If we tray to use an alternative build environment "MIXXX_VCPKG_ROOT" needs to be set.
If that exists, I think we can skip the whole evaluation of "BUILDENV_BASEPATH/BUILDENV_NAME"
In case it does not exists, I think there is no point in downloading to a diffrent folder than "MIXXX_VCPKG_ROOT".
So we may fail fatal in that case. However if we are in a shell where "BUILDENV_BASEPATH/BUILDENV_NAME" is set it will allways fail.
Conclusion we may only use "MIXXX_VCPKG_ROOT" and may dispose "BUILDENV_BASEPATH/BUILDENV_NAME"
What do you think?
b09ace0
to
efa7b67
Compare
/softfix:squash
|
f1ac722
to
7db1d4f
Compare
/softfix
|
7db1d4f
to
8779ff3
Compare
@daschuer were your change requests addressed? Merge? |
Sorry for the early fixup @JoergAtGithub , I missred that PR and thought it was ready to go |
8779ff3
to
f1ac722
Compare
Restored original commits |
If all settings inside |
Did you test it? For my understanding building an old build directory after applying this patch will fail because BUILDENV_NAME and BUILDENV_URL are not set. |
4acb7a6
to
163f0bc
Compare
…XX_VCPKG_ROOT from windows_buildenv.bat to the place of it's first usage in CMakeLists.txt
163f0bc
to
ccc613b
Compare
…will happen after running windows_buildenv.bat
If you want me to do some more test ... let me know |
Oh yes, now it's a good time to test! As all is addressed now! |
It tested now with switching from this PR to main. Deleted the CMake cache and configured without calling windows_buildenv.bat again. As the buildenv was still there and the toolchain file remains the same everything worked. I just got these warnings at the end:
Going the other direction it fails now with the following clear message:
|
I did the test |
But isn't this unnecessary? To make it work was my idea if using MIXXX_VCPKG_ROOT primary. |
No it's not, as we have to set And this addresses also your request, that multiple worktrees can share the same buildenv. This is possible, because you can overwrite BUILDENV_BASEPATH by env variable. This wouldn't be possible by MIXXX_VCPKG_ROOT. |
I can confirm that disposing MIXXX_VCPKG_ROOT is a good idea. It has never been documented as a user variable. However in a context of an IDE this PR still introduces an issue.
I have not tested this because I am not on Windows, but the for my understanding the batch file is not re-run if you use an IDE like QtCreator. In case of Visual studio, the settings ares stored in CMakeSettings.json. Other IDEAs have other ways to configure the CMakeCache.txt. In Eclipse (I use on Linux) for example I can imagine to configure the batch file one time, to configure the project form the command line and than use only Eclipse without touching the command line ever again. So I think instead of MIXXX_VCPKG_ROOT, CMAKE_TOOLCHAIN_FILE is the real source of truth. This is also widely known because it is a CMake standard variable. Can we check this to skip the prompt for rerunning the batch? I will propose the change inline. |
Since MIXXX_VCPKG_ROOT is now internal, we need to adjust the error messages mentions that. |
I have just noticed that there is still the logic of configuring VCPKG_OVERLAY_PORTS and VCPKG_OVERLAY_TRIPLETS. |
I could, but this variable is still generated by
To my understanding these are not of any use with the prebuilt buildenv. These apply only if you link to a local working copy of mixxxdj/VCPKG where you build the dependencies locally. |
Nikhil Bisht did some extra for us on windows and discovers some details / requirements:
He struggles to reconfigure a configured build directory. From this we can generate these requirements:
Does that make sense? |
Yes, this is because we set
Therefore adding
Yes, but CMake detects these cases and tells you very clear, when you have to delete the cache first. But it doesn't do this automatic.
Sure
If the correct one is specified yes. But the most important thing is, that a user never gets the wrong vcpkg environment unless he explicitly overides the one set in windows_buildenv.bat . Otherwise switching between branches would no longer work.
Sure, this is using only Ninja not CMake
Yes
This warning makes no sense to me, because if the user runs windows_buildenv.bat, he explicitly set the new buildenv by this action. This is the main purpose of this batch file.
Sure |
…nv when correct CMAKE_TOOLCHAIN_FILE is already cached
Ah, so we mirror (probably unnecessarily) the default bahviour of CMake without VCPKG. See: At least "Z_VCPKG_ROOT_DIR" is reliably used and updated without our extra adjustments.
I Have just tested to change the CMAKE_TOOLCHAIN_FILE. I receive this unclear warning:
When I try to use a diffrent generator I get such a waring:
But this is not our case if we update to a new environment. What did you do to get a better warning?
I am not sure if I get you right. Please verify. Let's asume we have updated the main branch to our new 2.6 environment:
I use a 2.5 and a "main" branch to avaoid to much rebuilds. This is also a good receipt to use the right environment. For convenience we should always keep te 2.5 branch building with the 2.6 environment. (We need probably anyway because of Linux) Do you agree or do I miss something?
Ok, Then we should fail. Works for me as well. |
It looks like with your latest changes we are already close. Thank you. I have not tested, but I think the only missng piece is the question around manual setting the environment. And outdated commenst around
Thay may want to do this (or equivalent, I don't mind)
Officially it is:
https://learn.microsoft.com/en-us/vcpkg/users/buildsystems/cmake-integration#cmake_toolchain_file For my underanding both should skip a download if configured as well in the enviroment variables. |
The CMAKE_TOOLCHAIN_FILE ends as Internally a similar command like this is used:
Since we cannot reconfigure cmake safely, we need to fail if the BUILD_ENV variables are set but don't match. |
No, the buildenv variables must always win. If they differ the toolchain file path is invalid. |
If the project is configured and the BUILD_ENV, we can't safely reconfigure. That's why the only chance we have that the buildenv variables can win is to ask for deleting the cache. |
Yes, that is the behavior we currently have and works very well for me. I will not change this in this PR! |
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 works good on Linux as far as I can tell. I am pretty sure that clanging the environment without clearing the Cache does not work. But I can't prove it yet.
if(WIN32) | ||
string(REPLACE "\\" "/" BUILDENV_BASEPATH "${BUILDENV_BASEPATH}") | ||
string(REPLACE "\\" "/" CMAKE_TOOLCHAIN_FILE "${CMAKE_TOOLCHAIN_FILE}") | ||
endif() |
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 mirrors the CMAKE_TOOLCHAIN_FILE path handling, but uses a new variable to not change any cached value.
if(WIN32) | |
string(REPLACE "\\" "/" BUILDENV_BASEPATH "${BUILDENV_BASEPATH}") | |
string(REPLACE "\\" "/" CMAKE_TOOLCHAIN_FILE "${CMAKE_TOOLCHAIN_FILE}") | |
endif() | |
file( | |
REAL_PATH | |
"${CMAKE_TOOLCHAIN_FILE}" | |
CLEAN_CMAKE_TOOLCHAIN_FILE | |
BASE_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" | |
EXPAND_TILDE | |
) | |
if(DEFINED BUILDENV_NAME) | |
file(TO_CMAKE_PATH ${BUILDENV_BASEPATH} BUILDENV_BASEPATH) | |
set(MIXXX_VCPKG_ROOT "${BUILDENV_BASEPATH}/${BUILDENV_NAME}") | |
endif() |
if( | ||
WIN32 | ||
AND | ||
( | ||
NOT DEFINED CMAKE_TOOLCHAIN_FILE | ||
OR | ||
( | ||
DEFINED BUILDENV_BASEPATH | ||
AND DEFINED BUILDENV_NAME | ||
AND | ||
NOT | ||
CMAKE_TOOLCHAIN_FILE | ||
MATCHES | ||
"${BUILDENV_BASEPATH}/${BUILDENV_NAME}/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.
if( | |
WIN32 | |
AND | |
( | |
NOT DEFINED CMAKE_TOOLCHAIN_FILE | |
OR | |
( | |
DEFINED BUILDENV_BASEPATH | |
AND DEFINED BUILDENV_NAME | |
AND | |
NOT | |
CMAKE_TOOLCHAIN_FILE | |
MATCHES | |
"${BUILDENV_BASEPATH}/${BUILDENV_NAME}/scripts/buildsystems/vcpkg.cmake" | |
) | |
) | |
) | |
if( | |
DEFINED CMAKE_TOOLCHAIN_FILE | |
AND DEFINED MIXXX_VCPKG_ROOT | |
AND | |
NOT | |
CLEAN_CMAKE_TOOLCHAIN_FILE | |
MATCHES | |
"${MIXXX_VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake" | |
) | |
message( | |
FATAL_ERROR | |
"Build environment: '${MIXXX_VCPKG_ROOT}' does not match the environment used previously: '${CMAKE_TOOLCHAIN_FILE}'. Either remove the CMakeCache.txt file and CMakeFiles directory or choose a different binary directory." | |
) | |
endif() | |
if(WIN32 AND NOT DEFINED CMAKE_TOOLCHAIN_FILE) |
Here is a CI run that demonstrates the issue It will work, if the original environment still exists and you download the new in addition, However this leads to a dubious situation where some parts of the old and some parts of the new environment are used. I consider this as maximal confusing and therefore the early error message with a clear instruction is helpful. |
This move the VCPKG buildenv download, SHA256 verification and unzipping into CMakelists.txt. This removes the need for third party tools like 7zip and Powershell (see #11524 for reference).
For the developer everything should behave as before. No change in the build instructions is neccessary - just the tasks mentioned above are executed later.
Out of scope of this PR is: