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.

Sign up for GitHub

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

Jump to bottom

ci: use submodules for freetype #1585

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

ci: use submodules for freetype #1585

wants to merge 4 commits into from

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Mar 8, 2025 •
edited
Loading

Our submodules may provide symbols not provided by system libraries.

Makes sure libraries that may be used by both the engine and the game are built once.

See on game side:

@illwieckz illwieckz mentioned this pull request Mar 8, 2025
Closed
illwieckz added 2 commits March 8, 2025 13:10
@illwieckz
ci: prefer submodule over system libs
23bc0d2
@illwieckz
cmake: do not prefer system libs by default
22e1afa 
Engine or game submodules maybe much recent than the one the system provides,
and define symbols not found in system libraries.

While it is probably unlikely to happen with freetype,
this is more likely to happen with some game libraries like Lua.
@illwieckz illwieckz force-pushed the illwieckz/ci-submodules/sync branch from 91d83d6 to 6d3f0a1 Compare March 8, 2025 14:11
slipher
slipher reviewed Mar 10, 2025
View reviewed changes
CMakeLists.txt
@@ -765,7 +765,7 @@ if (USE_BREAKPAD)
endif()
endif()

option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." ON)
option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." OFF)
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be changed because Unvanquished doesn't even build in the default configuration that way (it defines the Freetype lib twice).

Copy link
Member Author

@illwieckz illwieckz Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The Freetype submodule double build bug is now fixed.

When building both the engine and a native game, the native game now reuses the Freetype target already sets by the engine.

Copy link
Member

Choose a reason for hiding this comment

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

The Freetype submodule double build bug is now fixed.

Great!

What's the motivation for the change though? It makes builds longer and people building Daemon themselves can no longer benefit from distro security updates.

@illwieckz
cmake: make possible to build both game and engine against the engine…
9edd77d 
… Freetype submodule
@illwieckz
cmake: always use the -fPIC compiler flag
4de5657 
Some libraries may be built statically before being linked
against game dll, so -fPIC would be required, and then we
better compile everything with -fPIC.
@illwieckz illwieckz force-pushed the illwieckz/ci-submodules/sync branch from 6d3f0a1 to a73200a Compare March 17, 2025 16:15
@illwieckz illwieckz mentioned this pull request Mar 17, 2025
Draft
@illwieckz illwieckz force-pushed the illwieckz/ci-submodules/sync branch 2 times, most recently from 8b508fb to f5e9697 Compare March 18, 2025 14:37
slipher
slipher reviewed Mar 19, 2025
View reviewed changes
CMakeLists.txt Outdated
if (PREFER_EXTERNAL_LIBS AND NOT NACL)
find_package(${LIB_NAME})
if (NOT ${LIB_NAME}_FOUND)
if (PREFER_EXTERNAL_LIBS AND NOT NACL)
Copy link
Member

Choose a reason for hiding this comment

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

The rest of this file is using spaces, not tabs

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

CMakeLists.txt
@@ -765,7 +765,7 @@ if (USE_BREAKPAD)
endif()
endif()

option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." ON)
option(PREFER_EXTERNAL_LIBS "Tries to use system libs where possible." OFF)
Copy link
Member

Choose a reason for hiding this comment

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

The Freetype submodule double build bug is now fixed.

Great!

What's the motivation for the change though? It makes builds longer and people building Daemon themselves can no longer benefit from distro security updates.

@illwieckz
Copy link
Member Author

What's the motivation for the change though? It makes builds longer

The motivation is that we better better strictly control what we build against when doing our own official release builds. We already rebuild every deps using the external_deps script in the docker release build. So this changes nothing for the release build, either rebuilding the external_deps or rebuilding the submodule is the same (though submodule is less cumbersome).

And when a library is used in both game and engine, like freetype, we better use the exact same freetype library in engine so we don't deal with different bugs and have consistent version to track down.

Then we better build stuff in our CI the closest way we build the release, building the submodule in both cases is the easiest way to achieve that.

Note that for non-Linux systems where we usually ship many prebuilt libraries in external deps archives, we may modify the external_deps script to build from submodule so we ensure prebuilt libraries are exactly the same as submodule ones. This can be implemented above #1433:

and people building Daemon themselves can no longer benefit from distro security updates.

This is not true at all. The whole reason why PREFER_EXTERNAL_LIBS exists and it is actually implemented at first and all the extra code was written by myself is to keep the possibility for distro packages to build engines fully relying on system libraries, and to allow people to link against their system libraries for whatever reason (binary size, updates…).

If this prevent to benefit from distro security updates or just simply rely on system libraries, the whole PREFER_EXTERNAL_LIBS mechanism could be entirely NUKEd and would have not been implemented to begin with.

@illwieckz illwieckz force-pushed the illwieckz/ci-submodules/sync branch from f5e9697 to 5ee903c Compare March 24, 2025 15:36
@slipher
Copy link
Member

slipher commented Mar 24, 2025

SGTM to use the submodule in the release script and to make the CI match what is done with the release. I'm just disputing what should be the default value. To benefit players who build their own engine from source, the default should be to use the system libs. Release and CI can customize everything so the default doesn't matter for them.

@illwieckz illwieckz mentioned this pull request Mar 24, 2025
Merged
@illwieckz illwieckz force-pushed the illwieckz/ci-submodules/sync branch from 5ee903c to 23bc0d2 Compare March 24, 2025 15:43
@illwieckz
Copy link
Member Author

I extracted the non “default switching” changes there and there:

We better merge them in all case. I'm myself not entirely convinced about what's the best to do for the default, but we can already fix the existing bugs without deciding on the default.

@illwieckz illwieckz marked this pull request as draft March 24, 2025 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slipher slipher slipher left review comments

Assignees
No one assigned
Labels
A-Build
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@illwieckz @slipher