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

[SCons] Add option to build without threads #1451

Merged

Conversation

Faless
Copy link
Contributor

@Faless Faless commented Apr 30, 2024

This is relevant for the Web platform, where builds with and without threads are incompatible.

@Faless Faless added enhancement This is an enhancement on the current functionality platform:web topic:buildsystem Related to the buildsystem or CI setup labels Apr 30, 2024
@Faless Faless requested a review from a team as a code owner April 30, 2024 17:11
@Faless
Copy link
Contributor Author

Faless commented Apr 30, 2024

@adamscott in godotengine/godot#85939 we added the threads feature flag.

I think for max compatibility we should also add a nothreads feature flag when threads are not available. To make it less error prone when defining the tags in the .gdextension files (right now the order matters I think). WDYT?

This is relevant for the Web platform, where builds with and without
threads are incompatible.
@Faless Faless force-pushed the build/to_threads_or_not_to_threads branch from a17134c to b0296bb Compare April 30, 2024 17:19
@adamscott
Copy link
Member

I think for max compatibility we should also add a nothreads feature flag when threads are not available.

In what cases would not OS.has_feature("threads") != OS.has_feature("nothreads")?

@Faless
Copy link
Contributor Author

Faless commented Apr 30, 2024

In what cases would not OS.has_feature("threads") != OS.has_feature("nothreads")?

If you look at the changes in the .gdextension file:

https://github.com/godotengine/godot-cpp/pull/1451/files#diff-d24941d2910274e6d843d090ee0af8f2f32fbcc4c37112c21d394a36d183d201R32-R35

I can filter the "thread" version, but not the "nothread" version (and I'm relying on the fact that tags are matched in order).

So if I provide only a "nothread" version, I don't think I can't let godot know to not try to load that extension in threaded builds via the .gdextension file.

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

The changes look good to me. I did a little manual testing, and everything seemed to work fine. :-)

@dsnopek dsnopek added this to the 4.x milestone May 16, 2024
@dsnopek dsnopek merged commit 340dde3 into godotengine:master May 16, 2024
12 checks passed
@Faless Faless deleted the build/to_threads_or_not_to_threads branch May 17, 2024 15:50
@DmitriySalnikov
Copy link
Contributor

@dsnopek can these changes be cherry picked to 4.1 and 4.2?

I understand that there is no support for threads=yes/no in Godot 4.1 and Godot 4.2, but this is necessary for compatibility with Godot 4.3. As I pointed out here, I managed to build my library compatible with Godot 4.2+ (actually 4.1.4, but so far there is a problem with shaders). To do this, I needed a patch with this PR.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jul 22, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 22, 2024

I don't think it'd do any harm to cherry-pick it!

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 3, 2024

Cherry-picked for 4.2 in PR #1570

@dsnopek
Copy link
Collaborator

dsnopek commented Sep 4, 2024

Cherry-picked for 4.1 in PR #1572

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is an enhancement on the current functionality platform:web topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants