Add multi-threaded versions of sdl2_image and sdl2_ttf #22946
Conversation
sbc100
force-pushed
the
sdl2_image_fix
branch
from
f956130 to
b5e25a0
Compare
Because some possible dependencies of these libraries have `-mt` variants (specifically png and harfbuzz) we also needs to declare `-mt` variant of these libraries. The reason for this is a little complex: When we find the set of transitive dependencies of given library we select the correct variant of each library. This means that if we are building with `-pthread` then we select and include the `-mt` variants of all dependencies. However if libsdl2_image or libsdl2_ttf themselves then need to be built we end up building them without `-pthread` which means that emcc subprocesses will try to select and build the single threaded variants of the dependencies. This is mostly a serious problem because we don't allow for nested calls to emcc (we assume all dependencies have been already built before we try to build a given library and the we error out with `EM_CACHE_IS_LOCKED` if that is not the case). Testing this fix requires the cache to be setup just right so I'm not sure its worth it. Fixes: emscripten-core#22941, emscripten-core#20204
sbc100
force-pushed
the
sdl2_image_fix
branch
from
b5e25a0 to
610dcf9
Compare
| @@ -752,7 +752,7 @@ def test_embuilder_wildcards(self): | |||
| def test_embuilder_with_use_port_syntax(self): | |||
| restore_and_set_up() | |||
| self.run_process([EMBUILDER, 'build', 'sdl2_image:formats=png,jpg', '--force']) | |||
| self.assertExists(os.path.join(config.CACHE, 'sysroot', 'lib', 'wasm32-emscripten', 'libSDL2_image_jpg-png.a')) | |||
| self.assertExists(os.path.join(config.CACHE, 'sysroot', 'lib', 'wasm32-emscripten', 'libSDL2_image-jpg-png.a')) | |||
There was a problem hiding this comment.
Is the _ => - change needed, or for aesthetics? If it isn't needed then maybe it isn't worth it, as users might have hardcoded CI commands about it.
There was a problem hiding this comment.
Tend to use - for all our variants (not _) so this is just for consistency.
The precise names of these variants should not be hardcoded anywhere.. if it were I think that would be a bug.
There was a problem hiding this comment.
I just worry about a user that has
cp libSDL2_image_jpg-png.a $OUT_DIR
in their CI workflow. This change would make them need to rename.
But if you are confident that is unlikely, and we haven't had issues with such changes in the past, sgtm as is.
There was a problem hiding this comment.
I think that would be very unlikely yes. The consider those derived names (not the basename, but the extended name containing the variants) to be an internal detail that should not be relied on.
Because some possible dependencies of these libraries have
-mtvariants (specifically png and harfbuzz) we also needs to declare-mtvariant of these libraries.The reason for this is a little complex: When we find the set of transitive dependencies of given library we select the correct variant of each library. This means that if we are building with
-pthreadthen we select and include the-mtvariants of all dependencies. However if libsdl2_image or libsdl2_ttf themselves then need to be built we end up building them without-pthreadwhich means that emcc subprocesses will try to select and build the single threaded variants of the dependencies. This is mostly a serious problem because we don't allow for nested calls to emcc (we assume all dependencies have been already built before we try to build a given library and the we error out withEM_CACHE_IS_LOCKEDif that is not the case).Testing this fix requires the cache to be setup just right so I'm not sure its worth it.
Fixes: #22941, #20204