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

feat: Building on MSVC #366

Merged
merged 20 commits into from Nov 4, 2022
Merged

Conversation

TheLastRar
Copy link
Contributor

This PR contains the work I've done getting PrismLauncher to build with MSVC
I'm willing to split this into smaller PRs if desired.

On a cache hit GitHub actions takes 2-3 minutes to build and 15 seconds to package, bringing it more inline with the other builds see run on my branch

You will need vc redist 2022/latest and, because of Qt5's OpenSSL dlls, vc redist 2010

The debug builds produced by the CI link to the release runtime, so Visual Studio is not required to run those builds
If you produce debug builds without -DCMAKE_MSVC_RUNTIME_LIBRARY="MultiThreadedDLL" then prism links to the debug runtime, giving better debugging capability. (Release builds without CMAKE_MSVC_RUNTIME_LIBRARY set still link with the release runtime)

zlib is bundled, but that only used if Launcher_FORCE_BUNDLED_LIBS is set or system zlib is not found by CMake.
ECM is also bundled, and is only used if the system ECM is not found.

I dropped the CMP0020 OLD policy to make my life a bit easier (and for Clang-cl), looking at git blame it was added to suppress cmake warnings,
I believe qtmain just has a calls our main() function anyway

The CI action I've added used the msbuild generators, as at the time I couldn't get Ninja to use ccache.
I did later have an idea on the issue and volunteered someone to test it (it worked), at that point, however, I had already switched to msbuild, but I should be able to switch back and keep ccache working if desired.

Fully supporting msbuild did require modifying the CMake file to support multi config generators, it still maintains support for single config generators like Ninja
An interesting experiment might be to see if the Ninja Multi-Config generator works as expected.

Code changes at the start of the series are fixing build issues with MSVC, code fixes at the end of the series is for Clang-cl
Clang-cl issues where spotted by getchoo in the discord, although I did not use their patch as-is

@TheLastRar
Copy link
Contributor Author

I'll also take the opportunity to document what I've learned about cmake's install step with fixup_bundle on windows

If it's run in an environment that's been prepared by vcvars*.bat (such as the one you get when you run 'Native Tools Command Prompt for VS' or with the ilammy/msvc-dev-cmd action it actually runs pretty quick

MSYS2 however doesn't inherit all of the environment variables (namely additions to PATH), this can be corrected by setting path-type to inherit (desktop MSYS2 has a config file for this).
however this could lead to interference between MSYS2 and other tools installed on the system, so I never tested this in actions.

Copy link
Member

@DioEgizio DioEgizio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, don't forget to update the release workflow to add the new files!

.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@DioEgizio
Copy link
Member

I'll also take the opportunity to document what I've learned about cmake's install step with fixup_bundle on windows

If it's run in an environment that's been prepared by vcvars*.bat (such as the one you get when you run 'Native Tools Command Prompt for VS' or with the ilammy/msvc-dev-cmd action it actually runs pretty quick

MSYS2 however doesn't inherit all of the environment variables (namely additions to PATH), this can be corrected by setting path-type to inherit (desktop MSYS2 has a config file for this). however this could lead to interference between MSYS2 and other tools installed on the system, so I never tested this in actions.

yeah, fixup_bundle/getprerequisites is weird in general, and it's also deprecated, but the alternative kinda sucks :p

@TheLastRar TheLastRar force-pushed the MSVC-Build branch 2 times, most recently from 95d3f91 to 18f9db4 Compare November 1, 2022 20:05
@TheLastRar
Copy link
Contributor Author

also, don't forget to update the release workflow to add the new files!

Dunno if I've modified the release workflow correctly, not sure how to test that

@TheLastRar
Copy link
Contributor Author

Release workflow seems to work in a test branch
image

@flowln flowln added enhancement New feature or request actions Issues and PRs related to GH actions and other CIs we might have Windows Issues and PRs related to WIndows specifically labels Nov 1, 2022
@DioEgizio DioEgizio added the packaging Issues and PRs related to packaging (CI builds or package managers) label Nov 2, 2022
@DioEgizio DioEgizio added this to the 6.0 milestone Nov 2, 2022
CMakeLists.txt Outdated Show resolved Hide resolved
@txtsd
Copy link
Member

txtsd commented Nov 3, 2022

Now rebase and you'll have the stack size fix.

EDIT: oh nvm. It's in here too.

@TheLastRar
Copy link
Contributor Author

I should actually note that the legacy builds are on MSVC are Qt 5.15.2 instead of Qt 5.15.6 as that is the latest version that aqtinstall can grab, hopefully that isn't an issue. Legacy Mac is using the same version anyway.

Also seems like aqtinstall tripped up on the openssl libs, seems Qt updated it's OpenSSL libs today so maybe not everything aqt needs has gotten the memo, hopefully that resolves itself

MSVC uses a different search mechanism that ends up picking the mete Version.h

Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
@txtsd
Copy link
Member

txtsd commented Nov 4, 2022

I should actually note that the legacy builds are on MSVC are Qt 5.15.2 instead of Qt 5.15.6 as that is the latest version that aqtinstall can grab, hopefully that isn't an issue. Legacy Mac is using the same version anyway.

Also seems like aqtinstall tripped up on the openssl libs, seems Qt updated it's OpenSSL libs today so maybe not everything aqt needs has gotten the memo, hopefully that resolves itself

We should probably pin all versions to known working ones 👀

@DioEgizio
Copy link
Member

I should actually note that the legacy builds are on MSVC are Qt 5.15.2 instead of Qt 5.15.6 as that is the latest version that aqtinstall can grab, hopefully that isn't an issue. Legacy Mac is using the same version anyway.

it's not a big issue, and it's also the only version we can pick without compiling Qt from source :/

@DioEgizio
Copy link
Member

I should actually note that the legacy builds are on MSVC are Qt 5.15.2 instead of Qt 5.15.6 as that is the latest version that aqtinstall can grab, hopefully that isn't an issue. Legacy Mac is using the same version anyway.
Also seems like aqtinstall tripped up on the openssl libs, seems Qt updated it's OpenSSL libs today so maybe not everything aqt needs has gotten the memo, hopefully that resolves itself

We should probably pin all versions to known working ones 👀

MSYS2 is rolling release, so we can't do that there

@Scrumplex
Copy link
Member

Scrumplex commented Nov 4, 2022

Would be cool if we had Nix on Windows to pin dependencies :trollface:

@DioEgizio
Copy link
Member

we has :trollface:

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Clang-cl fails to select the correct function and instead errors

Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
Signed-off-by: TheLastRar <TheLastRar@users.noreply.github.com>
@Scrumplex Scrumplex merged commit 5e9b26d into PrismLauncher:develop Nov 4, 2022
@Scrumplex
Copy link
Member

Thanks a lot!

@TheLastRar TheLastRar deleted the MSVC-Build branch November 5, 2022 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actions Issues and PRs related to GH actions and other CIs we might have enhancement New feature or request packaging Issues and PRs related to packaging (CI builds or package managers) Windows Issues and PRs related to WIndows specifically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants