Move download, SHA256 check and unzip of Windows buildenv from windows_buildenv.bat to CMakelists.txt#13306
Conversation
22b1df5 to
ecc462a
Compare
9cea1ee to
b09ace0
Compare
|
@daschuer Friendly Ping |
|
@daschuer Can we merge this? |
| endif() | ||
|
|
||
|
|
||
| if(WIN32) |
There was a problem hiding this comment.
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.
It would be great if you could create a follow-up PR for macOS.
|
@daschuer Can we merge this now? |
daschuer
left a comment
There was a problem hiding this comment.
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 |
|
All done! |
Hi Jörg, |
|
@Eve00000 The code with the message 'did you execute the windows_buildenv.bat' is not touched by this PR. This is therefore out of scope. The final goal is of cause to get rid of this batch file at all, but this is up to future PRs. There is no need to modify the CMakelists.txt code when you use a zip file from other sources (a GitHub PR build artifact). Just manual download the ZIP file in the directory specified as BUILDENV_BASEPATH, set BUILDENV_NAME to the name of these manual downloaded file(without the file extension .zip). |
|
Cool! |
|
@Eve00000 while testing, can you keep an eye in the documentation? I can imagine that some wiki docks needs to be adjusted, error messages rephrased. |
|
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! |
daschuer
left a comment
There was a problem hiding this comment.
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.
|
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. |
04db0c1 to
cf364ea
Compare
…s_buildenv.bat to CMakelists.txt
cf364ea to
c6066d0
Compare
|
The requested fata error message is added, this is ready now! |
|
@daschuer A friendling ping, that this is ready to merge! |
This additional condition doesn't harm my use cases Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
Co-authored-by: Daniel Schürmann <daschuer@mixxx.org>
Fix wrong variable use syntax in condition Unified directory seperator between BUILDENV_BASEPATH and BUILDENV_NAME Inhibited warnings about unused external specified variables if the buildenv is already there
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: