search

DescentDevelopers / Descent3

Descent 3 by Outrage Entertainment
GNU General Public License v3.0
2.74k stars 231 forks source link

Windows CMake preset: use ninja generator #422

Closed Lgt2x closed 3 weeks ago

Lgt2x commented 1 month ago

Pull Request Type

Description

Use the Ninja CMake generator on Windows instead of the Visual Studio one. Ninja provides better build output, and handles multi-process builds much better than the previous build system. This also makes Windows on par with the other OSes already using it.

This PR also renames the Github Windows jobs, that had the same name for x86 and x64

Related Issues

421

Lgt2x commented 1 month ago

@tophyr can you give this a try? :)

tophyr commented 1 month ago

@Lgt2x this fails to link win32 for me: https://gist.github.com/tophyr/edd91f5a48aec64dee2c79545dddc7b4

Looks like it's trying to use the x64 version of zlib, despite specifying VCPKG_TARGET_TRIPLET in CMakePresets.json - interesting.

Also interesting: Developer PowerShell uses MSVC's built-in vcpkg, but normal PowerShell uses the one I installed manually (and cleaned/re-bootstrapped in the first part of the paste). Wonder if that's part of things.

tophyr commented 1 month ago

Ah, came back to this and found it. CMake is, for some reason, passing /machine:x64 to link.exe when doing a win32 build.

I wonder if this is what you meant by

Ninja generator for CMake does not support the -A flag to support setting build architecture

in chat?

tophyr commented 1 month ago

See https://github.com/DescentDevelopers/Descent3/pull/423

Lgt2x commented 1 month ago

Given the external VS script to set arch, let's do away with the win32/win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment.

I'd still like to have 2 presets for developers to be able to compile x64 and x86 in different folders, I'll see what I can do

tophyr commented 1 month ago

Isn't that what the -B option to cmake is for?

On Fri, Jun 7, 2024 at 11:21 AM Louis Gombert @.***> wrote:

Given the external VS script to set arch, let's do away with the win32/ win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment.

I'd still like to have 2 presets for developers to be able to compile x64 and x86 in different folders, I'll see what I can do

— Reply to this email directly, view it on GitHub https://github.com/DescentDevelopers/Descent3/pull/422#issuecomment-2155153259, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RIJJIKIXHNZOICY3E3ZGHMYPAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGE2TGMRVHE . You are receiving this because you were mentioned.Message ID: @.***>

Lgt2x commented 1 month ago

Isn't that what the -B option to cmake is for? … On Fri, Jun 7, 2024 at 11:21 AM Louis Gombert @.> wrote: Given the external VS script to set arch, let's do away with the win32/ win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment. I'd still like to have 2 presets for developers to be able to compile x64 and x86 in different folders, I'll see what I can do — Reply to this email directly, view it on GitHub <#422 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RIJJIKIXHNZOICY3E3ZGHMYPAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGE2TGMRVHE . You are receiving this because you were mentioned.Message ID: @.>

When using presets you're not supposed to use -B because we specify buildPresets that choose the build folder for you. I think I found what I was looking for, I updated the PR. It's now using "architecture" set as "external", which means "don't try to load the toolchain through the generator", but IDEs recognize it

Lgt2x commented 1 month ago

@jcoby @Arcnor can one of you review that?

tophyr commented 1 month ago

Nice. I don't have too strong an opinion anyway, just don't love the idea of forgetting settings and producing a 64 build into a win32 directory.

On Fri, Jun 7, 2024 at 12:21 PM Louis Gombert @.***> wrote:

Isn't that what the -B option to cmake is for? … <#m-4602107359020641232> On Fri, Jun 7, 2024 at 11:21 AM Louis Gombert @.> wrote: Given the external VS script to set arch, let's do away with the win32/ win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment. I'd still like to have 2 presets for developers to be able to compile x64 and x86 in different folders, I'll see what I can do — Reply to this email directly, view it on GitHub <#422 (comment) https://github.com/DescentDevelopers/Descent3/pull/422#issuecomment-2155153259>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RIJJIKIXHNZOICY3E3ZGHMYPAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGE2TGMRVHE https://github.com/notifications/unsubscribe-auth/AAPB6RIJJIKIXHNZOICY3E3ZGHMYPAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGE2TGMRVHE . You are receiving this because you were mentioned.Message ID: @.>

When using presets you're not supposed to use -B because we specify buildPresets that choose the build folder for you. I think I found what I was looking for, I updated the PR. It's now using "architecture" set as "external", which means "don't try to load the toolchain through the generator", but IDEs recognize it

— Reply to this email directly, view it on GitHub https://github.com/DescentDevelopers/Descent3/pull/422#issuecomment-2155235685, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RJH57OUPLA2Z3NK323ZGHT2TAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGIZTKNRYGU . You are receiving this because you were mentioned.Message ID: @.***>

Lgt2x commented 1 month ago

Nice. I don't have too strong an opinion anyway, just don't love the idea of forgetting settings and producing a 64 build into a win32 directory. On Fri, Jun 7, 2024 at 12:21 PM Louis Gombert @.> wrote: … Isn't that what the -B option to cmake is for? … <#m-4602107359020641232> On Fri, Jun 7, 2024 at 11:21 AM Louis Gombert @.> wrote: Given the external VS script to set arch, let's do away with the win32/ win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment. I'd still like to have 2 presets for developers to be able to compile x64 and x86 in different folders, I'll see what I can do — Reply to this email directly, view it on GitHub <#422 (comment) <#422 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RIJJIKIXHNZOICY3E3ZGHMYPAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGE2TGMRVHE https://github.com/notifications/unsubscribe-auth/AAPB6RIJJIKIXHNZOICY3E3ZGHMYPAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGE2TGMRVHE . You are receiving this because you were mentioned.Message ID: @.> When using presets you're not supposed to use -B because we specify buildPresets that choose the build folder for you. I think I found what I was looking for, I updated the PR. It's now using "architecture" set as "external", which means "don't try to load the toolchain through the generator", but IDEs recognize it — Reply to this email directly, view it on GitHub <#422 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RJH57OUPLA2Z3NK323ZGHT2TAVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGIZTKNRYGU . You are receiving this because you were mentioned.Message ID: @.>

the only difference this PR does is not force using the right toolchain but "advise" it. That's the best we can do AFAIK

Arcnor commented 1 month ago

Main issue I see with this is that the compiler is now not guaranteed to be the same on Windows (the MSVC generator always picks the VC compiler unless otherwise forced, but Ninja can go either way). This might or might not be a problem, depending on whoever builds this. As for the CI, nothing will change obviously.

tophyr commented 1 month ago

Main issue I see with this is that the compiler is now not guaranteed to be the same on Windows (the MSVC generator always picks the VC compiler unless otherwise forced, but Ninja can go either way). This might or might not be a problem, depending on whoever builds this. As for the CI, nothing will change obviously.

Iirc there is a toolset parameter similar to architecture that should tell MSVC to use the cmake-specified generator/toolchain

Lgt2x commented 1 month ago

Main issue I see with this is that the compiler is now not guaranteed to be the same on Windows (the MSVC generator always picks the VC compiler unless otherwise forced, but Ninja can go either way). This might or might not be a problem, depending on whoever builds this. As for the CI, nothing will change obviously. :

When launching CMake from the x86 command line prompt, which is the method we recommend, the right compiler is selected. I don't think this is a huge problem in other situations

tophyr commented 1 month ago

Main issue I see with this is that the compiler is now not guaranteed to be the same on Windows (the MSVC generator always picks the VC compiler unless otherwise forced, but Ninja can go either way). This might or might not be a problem, depending on whoever builds this. As for the CI, nothing will change obviously.

Iirc there is a toolset parameter similar to architecture that should tell MSVC to use the cmake-specified generator/toolchain

Actually hold on, @Arcnor i don't think I understood the concern properly. My CLI and MSVC builds use identical cl.exe invocations (checked by introducing a cpp error) and MSVC is definitely using Ninja to build (based on build speed and text output)

In what situations do you foresee MSVC and CLI diverging?

tophyr commented 1 month ago

Given the external VS script to set arch, let's do away with the win32/win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment.

See https://github.com/Lgt2x/Descent3/pull/6 for what I mean here - I think the configs are a lot "cleaner". Although I admit that I haven't figured out how, in the MSVC UI, to specify the target architecture. "Cleaner configs" wouldn't be a strong enough argument to eliminate that capability if MSVC doesn't have a way to specify arch.

Lgt2x commented 4 weeks ago

Given the external VS script to set arch, let's do away with the win32/win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment.

See Lgt2x#6 for what I mean here - I think the configs are a lot "cleaner". Although I admit that I haven't figured out how, in the MSVC UI, to specify the target architecture. "Cleaner configs" wouldn't be a strong enough argument to eliminate that capability if MSVC doesn't have a way to specify arch.

I understand what you want to do; yet I still prefer my solution. It solves the problem of having 2 builds (x86, x64) with 2 presets, which is not automated using presets in your solution. We cannot force selecting arch anymore, but it's not a big issue if the dev either know what they're doing, uses an IDE that selects arch using the "external" preset instruction, or follows instructions in the README.

tophyr commented 4 weeks ago

Yeah I'm not married to it

On Sun, Jun 9, 2024 at 1:24 PM Louis Gombert @.***> wrote:

Given the external VS script to set arch, let's do away with the win32/ win64 distinction entirely and just have only a win preset. vcpkg arch will be picked up from the build environment.

See Lgt2x#6 https://github.com/Lgt2x/Descent3/pull/6 for what I mean here - I think the configs are a lot "cleaner". Although I admit that I haven't figured out how, in the MSVC UI, to specify the target architecture. "Cleaner configs" wouldn't be a strong enough argument to eliminate that capability if MSVC doesn't have a way to specify arch.

I understand what you want to do; yet I still prefer my solution. It solves the problem of having 2 builds (x86, x64) with 2 presets, which is not automated using presets in your solution. We cannot force selecting arch anymore, but it's not a big issue if the dev either know what they're doing, uses an IDE that selects arch using the "external" preset instruction, or follows instructions in the README.

— Reply to this email directly, view it on GitHub https://github.com/DescentDevelopers/Descent3/pull/422#issuecomment-2156728105, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RPKLQNLX5FRDUNDER3ZGSMV3AVCNFSM6AAAAABI3JYEKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJWG4ZDQMJQGU . You are receiving this because you were mentioned.Message ID: @.***>