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

[Linux] Make SO wrapper usage optional. #73359

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Feb 15, 2023

Adds use_sowrap build option to disable SO wrappers and link system libraries directly.

See #69449 (comment)

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks pretty good!

@omar-polo
Copy link
Contributor

Thanks for working on this PR! It works fine on OpenBSD and I can see Godot being linked to libX11 & co.

I had to do a small change on top to fix the compilation:

--- platform/linuxbsd/x11/display_server_x11.cpp
+++ platform/linuxbsd/x11/display_server_x11.cpp
@@ -28,8 +28,6 @@
 /* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.                 */
 /**************************************************************************/

-#include "display_server_x11.h"
-
 #ifdef X11_ENABLED

 #include "core/config/project_settings.h"
@@ -41,6 +39,8 @@
 #include "main/main.h"
 #include "scene/resources/texture.h"

+#include "display_server_x11.h"
+
 #if defined(VULKAN_ENABLED)
 #include "servers/rendering/renderer_rd/renderer_compositor_rd.h"
 #endif

otherwise the compilation would fail with various errors (see below.) To be fair I'm building Godot with a few more patches on top (needed for thirdparty/ stuff mostly) so it may be due to that, although I doubt.

[ 34%] In file included from platform/linuxbsd/x11/display_server_x11.cpp:31:
platform/linuxbsd/x11/display_server_x11.h:351:2: error: unknown type name 'Thread'; did you mean 'pthread'?
        Thread events_thread;
        ^~~~~~
        pthread
/usr/include/pthread.h:95:8: note: 'pthread' declared here
struct pthread;
       ^
In file included from platform/linuxbsd/x11/display_server_x11.cpp:31:
platform/linuxbsd/x11/display_server_x11.h:351:9: error: field has incomplete type 'pthread'
        Thread events_thread;
               ^
/usr/include/pthread.h:95:8: note: forward declaration of 'pthread'
struct pthread;
       ^
platform/linuxbsd/x11/display_server_x11.cpp:4827:17: error: cannot initialize a variable of type 'DisplayServer *' with an rvalue of type 'DisplayServerX11 *'
        DisplayServer *ds = memnew(DisplayServerX11(p_rendering_driver, p_mode, p_vsync_mode, p_flags, p_position, p_resolution, p_screen, r_error));
                       ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
platform/linuxbsd/x11/display_server_x11.cpp:4926:18: error: cannot initialize object parameter of type 'const DisplayServer' with an expression of type 'DisplayServerX11'
        int rq_screen = get_screen_from_rect(p_rect);
                        ^~~~~~~~~~~~~~~~~~~~
4 errors generated.

@bruvzg bruvzg force-pushed the so_wrap_opt branch 2 times, most recently from b00ab62 to 529b61e Compare February 16, 2023 07:28
@@ -41,6 +39,8 @@
#include "main/main.h"
#include "scene/resources/texture.h"

#include "display_server_x11.h"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why that move is needed, sounds to me like we have some includes above that need to go to the header.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer we fix this properly, as someone might move this back up following our includes policy and trigger the same error.

I haven't tested but at least this might help:

diff --git a/platform/linuxbsd/x11/display_server_x11.cpp b/platform/linuxbsd/x11/display_server_x11.cpp
index 4d3eecd4fc..896b7b95eb 100644
--- a/platform/linuxbsd/x11/display_server_x11.cpp
+++ b/platform/linuxbsd/x11/display_server_x11.cpp
@@ -28,6 +28,8 @@
 /* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.                 */
 /**************************************************************************/
 
+#include "display_server_x11.h"
+
 #ifdef X11_ENABLED
 
 #include "core/config/project_settings.h"
@@ -39,8 +41,6 @@
 #include "main/main.h"
 #include "scene/resources/texture.h"
 
-#include "display_server_x11.h"
-
 #if defined(VULKAN_ENABLED)
 #include "servers/rendering/renderer_rd/renderer_compositor_rd.h"
 #endif
diff --git a/platform/linuxbsd/x11/display_server_x11.h b/platform/linuxbsd/x11/display_server_x11.h
index 1eba2f8a4c..dbe8a0ce2b 100644
--- a/platform/linuxbsd/x11/display_server_x11.h
+++ b/platform/linuxbsd/x11/display_server_x11.h
@@ -36,6 +36,8 @@
 #include "servers/display_server.h"
 
 #include "core/input/input.h"
+#include "core/os/mutex.h"
+#include "core/os/thread.h"
 #include "core/templates/local_vector.h"
 #include "drivers/alsa/audio_driver_alsa.h"
 #include "drivers/alsamidi/midi_driver_alsamidi.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW it still builds fine for me with this applied, thanks!

@bruvzg bruvzg marked this pull request as ready for review February 16, 2023 09:07
@bruvzg bruvzg requested review from a team as code owners February 16, 2023 09:07
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Confirmed working fine for a build with as many libraries unbundled (linked against system libs) as possible.

@akien-mga akien-mga modified the milestones: 4.x, 4.0 Feb 16, 2023
@akien-mga akien-mga merged commit 29f670b into godotengine:master Feb 16, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants