search

DescentDevelopers / Descent3

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

Use vcpkg on all presets #469

Open tophyr opened 1 week ago

tophyr commented 1 week ago

Pull Request Type

Description

Uses vcpkg for all platforms and removes platform-specific installation methods for ZLib, SDL and GTest. Still allows system-provided libraries.

Related Issues

Effectively a rebased and simplified reboot of @Arcnor's #242

Checklist

tophyr commented 1 week ago

I added it into the main CMakeLists right at the top. That accomplishes two things: it removes the burden of specifying it on the invocation (or on specifying a preset) and it lets us tie in to vcpkg's toolchain-chaining capability, which will be critical for Android.

On Wed, Jun 26, 2024 at 12:51 PM Louis Gombert @.***> wrote:

@.**** commented on this pull request.

In CMakePresets.json https://github.com/DescentDevelopers/Descent3/pull/469#discussion_r1655297212 :

@@ -39,8 +21,7 @@ "architecture": { "strategy": "external", "value": "x64"

  • },
  • "toolchainFile": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake"

I think you still need that to load the toolchain, especially locally

— Reply to this email directly, view it on GitHub https://github.com/DescentDevelopers/Descent3/pull/469#pullrequestreview-2142585308, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RKC76GU7MYXKHKS5RLZJL5S7AVCNFSM6AAAAABJ5B7LOKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBSGU4DKMZQHA . You are receiving this because you authored the thread.Message ID: @.***>

Lgt2x commented 1 week ago

I added it into the main CMakeLists right at the top. That accomplishes two things: it removes the burden of specifying it on the invocation (or on specifying a preset) and it lets us tie in to vcpkg's toolchain-chaining capability, which will be critical for Android. … On Wed, Jun 26, 2024 at 12:51 PM Louis Gombert @.> wrote: @*.*** commented on this pull request. ------------------------------ In CMakePresets.json <#469 (comment)> : > @@ -39,8 +21,7 @@ "architecture": { "strategy": "external", "value": "x64" - }, - "toolchainFile": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake" I think you still need that to load the toolchain, especially locally — Reply to this email directly, view it on GitHub <#469 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPB6RKC76GU7MYXKHKS5RLZJL5S7AVCNFSM6AAAAABJ5B7LOKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNBSGU4DKMZQHA . You are receiving this because you authored the thread.Message ID: @.>

Oh right, I did not notice, that's fine

tophyr commented 1 week ago

@winterheart I'd be happy with a solution that accepted either system-installed libraries or vcpkg ones. I would assume that a system-packaged library should take precedence over a vcpkg-provided one? Is there a pre-existing convention to use for buildscript parameters (-DCMAKE_FOO etc) to force using one or the other? (For example, in the case that I do have a system SDL installed but I want the build to use the vcpkg one anyway.)

Jayman2000 commented 1 week ago

@winterheart I'd be happy with a solution that accepted either system-installed libraries or vcpkg ones.

What about this?

I would assume that a system-packaged library should take precedence over a vcpkg-provided one?

I don’t think that the system-packaged library should take precedence over a vcpkg-provided one. I want to be able to easily test two scenarios:

  1. Compiling Descent 3 with all dependencies provided by vcpkg. Testing this scenario helps ensure that there are no undeclared dependencies.
  2. Compiling Descent 3 with all dependencies provided by the system package manager. Testing this scenario helps ensure that packages for Descent 3 be built in a way that distributions will approve of.

If system packages took precedence over vcpkg packages, then it would be more difficult to test that first scenario.

Is there a pre-existing convention to use for buildscript parameters (-DCMAKE_FOO etc) to force using one or the other? (For example, in the case that I do have a system SDL installed but I want the build to use the vcpkg one anyway.)

I don’t know if there’s a pre-existing convention, but I do know that GZDoom has options like FORCE_INTERNAL_BZIP2 that you might want to take a look at.

winterheart commented 1 week ago

@winterheart I'd be happy with a solution that accepted either system-installed libraries or vcpkg ones. I would assume that a system-packaged library should take precedence over a vcpkg-provided one? Is there a pre-existing convention to use for buildscript parameters (-DCMAKE_FOO etc) to force using one or the other? (For example, in the case that I do have a system SDL installed but I want the build to use the vcpkg one anyway.)

I think -DUSE_VCPKG (with default OFF) option would be OK. I don't think, that we'll need to install different dependencies from different sources.

Lgt2x commented 1 week ago

Why not try to use VCPKG if available (ie VCPKG_ROOT env variable set) and fallback to system libs if not found? With a clear message during configuration that tells you what's being used. I agree though that mixing VCPKG and systems libs is a bad idea

tophyr commented 1 week ago

Went with a combination of @Jayman2000's and @Lgt2x's ideas: The system will use vcpkg by default if VCPKG_ROOT is set in the environment, and if USE_VCPKG is not set to OFF.

Something I've noticed is that if the build has already been configured, it will continue using that configuration's settings even if different ones are specified. Is that normal CMake behavior? This feels like either a bug or a misunderstanding on my part.

Lgt2x commented 1 week ago

Went with a combination of @Jayman2000's and @Lgt2x's ideas: The system will use vcpkg by default if VCPKG_ROOT is set in the environment, and if USE_VCPKG is not set to OFF.

Something I've noticed is that if the build has already been configured, it will continue using that configuration's settings even if different ones are specified. Is that normal CMake behavior? This feels like either a bug or a misunderstanding on my part.

Can you give an example of the commands you've tried?

Needs a warning or error message when USE_VCPKG is ON and VCPKG_ROOT cannot be found

tophyr commented 1 week ago

Can you give an example of the commands you've tried?

https://gist.github.com/tophyr/2f8df17c6a46c3a9157ac73f69de1939#file-gistfile1-txt-L104-L110

I think this is intended behavior due to CMake's CMakeCache.txt file. Doing this:

cmake --preset mac

ends up storing USE_VCPKG=ON and CMAKE_TOOLCHAIN_FILE=/Users/csarbora/Projects/vcpkg/ (for me) in the CMakeCache.txt file, and then those values are already set (as if they were passed in on the command line) for subsequent CMake runs. This is intentional, so that this works:

cmake -B builds/mac -DUSE_VCPKG=OFF -DSOME_OTHER_OPTION=FOO
cmake --build builds/mac # USE_VCPKG and SOME_OTHER_OPTION will be set even tho they are unspecified

.... in short, i hate cmake

tophyr commented 5 days ago

Rewrote the logic to enable warning if USE_VCPKG && !VCPKG_ROOT per @Lgt2x's feedback. Behavior is now:

USE_VCPKG ENV{VCPKG_ROOT} Behavior
\<unset> \<unset> System Libraries
\<unset> \<set> vcpkg
ON \<unset> Warning + System Libraries
ON \<set> vcpkg
OFF \<unset> System Libraries
OFF \<set> System Libraries

Rewrote the Build Instructions in the README file as well; feedback welcome.