Skip to content

Conversation

@db-tech
Copy link
Contributor

@db-tech db-tech commented Dec 6, 2024

I added support for multi threading (-pthread) to the sdl2_mixer port.
For me this was kind of new but I did try to follow the prinziples of the sdl2_image adn sdl2_ttf (#22946) changes.

I also changed the "_" to "-" for consistency.

Let me know if there is something wrong or missing.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 6, 2024

Seems reasonable to me. Does SDL2_mixer actually need thread support?

@db-tech
Copy link
Contributor Author

db-tech commented Dec 7, 2024

Should fix some dependency issues discussed here: #23093

To summerize:
Without the changes I get errors like:

AssertionError: attempt to lock the cache while a parent process is holding the lock (sysroot/lib/wasm32-emscripten/libSDL2.a)

So It might be a dependency problem because mixer depends on another SDL2 version (-mt) then the other SDL2 libs and would differ from the SDL2 lib that is used when -pthread is enabled.

Copy link

@ololoken ololoken left a comment

Choose a reason for hiding this comment

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

lgtm
tested and fix works

'sdl2_mixer_none': {'SDL2_MIXER_FORMATS': []},
'sdl2_mixer-mp3': {'SDL2_MIXER_FORMATS': ["mp3"]},
'sdl2_mixer-none': {'SDL2_MIXER_FORMATS': []},
'sdl2_mixer-mp3-mt': {'SDL2_MIXER_FORMATS': ["mp3"], 'PTHREADS': 1},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use single quote here and above.

@sbc100
Copy link
Collaborator

sbc100 commented Dec 27, 2024

It would be good to a test here that depends on this new variant. What is the set of command line flags you used to cause the AssertionError: attempt to lock the cache while a parent process is holding the lock (sysroot/lib/wasm32-emscripten/libSDL2.a) failure?

@sbc100
Copy link
Collaborator

sbc100 commented Dec 27, 2024

Also can you rebase or merge so you are up-to-date with main?

@oleg-derevenetz
Copy link
Contributor

Hi @sbc100

What is the set of command line flags you used to cause the AssertionError: attempt to lock the cache while a parent process is holding the lock (sysroot/lib/wasm32-emscripten/libSDL2.a) failure?

Here is a reproducible example of the build failure when building the project that depends on sdl2-mixer with -pthread:

https://github.com/oleg-derevenetz/fheroes2/actions/runs/12609145320/job/35142382904#step:5:40

and here is the corresponding test PR:

oleg-derevenetz/fheroes2#218

If there are any issues with maintaining this PR, I can create my own (based on this one with credits to @db-tech) to speed up the process, because I'm interested in solving this build issue, since I would like to build my project for WASM with threads... eventually.

@db-tech
Copy link
Contributor Author

db-tech commented Jan 4, 2025

Hi, sorry for not answering.
I am currently on vacation and usually off the grid.

Tomorrow, I might have time to address the comments.

Otherwise, I am back next week.

@sbc100 sbc100 enabled auto-merge (squash) January 6, 2025 17:28
@db-tech
Copy link
Contributor Author

db-tech commented Jan 6, 2025

As @oleg-derevenetz said "-pthread" is the option which results in issues.
I changed the double quotes to single ones and, fetched upstream and rebased, I hope that was correct.
Otherwise, let me know.

@sbc100 sbc100 merged commit 21d9dd8 into emscripten-core:main Jan 6, 2025
29 checks passed
Totto16 added a commit to OpenBrickProtocolFoundation/oopetris that referenced this pull request Jan 15, 2025
- since upstream emscripten-core/emscripten#23094 was merged, the local patch is now unnecessary
- fix the rest to incorporate these changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants