fix examples Makefile to use Makefile.Web when building for web (Brings core/core_loading_thread example closer to working)

[keithstellyes]

The examples Makefile wasn't properly using Makefile.Web which caused one of the bug in the core/core_loading_thread to not work (as mentioned in the wishlist 😊), as the compiler was not being passed the -s USE_PTHREADS=1 arg, consider the following terminal session, (edited for brevity)

raylib/examples: make -B PLATFORM=PLATFORM_WEB core/core_loading_thread

Output:

emcc -o core/core_loading_thread.html core/core_loading_thread.c [...] -s TOTAL_MEMORY=134217728 -s FORCE_FILESYSTEM=1 -s ASYNCIFY --preload-file core/resources@resources --shell-file ../src/minshell.html ../src/libraylib.a -DPLATFORM_WEB

raylib/examples: make -f Makefile.Web -B PLATFORM=PLATFORM_WEB core/core_loading_thread

Output:

emcc -o core/core_loading_thread.html core/core_loading_thread.c -[...] -s ASYNCIFY -s EXPORTED_RUNTIME_METHODS=ccall --shell-file ../src/shell.html ../src/libraylib.a -DPLATFORM_WEB -s USE_PTHREADS=1

Note that the Makefile.Web does produce an output where it's a bit more zoomed in, exploded, whatever you wanna call it. But, this seems to be a pre-existing issue that is exposed by this fix.

This was NOT tested with the Windows flow which probably needs testing (not sure if emmake is used there?)

Note that the example still does not ever finish joining

[ghost]

@keithstellyes

Makefile wasn't properly using Makefile.Web

1. That's an interesting find. I've been consistently using the current Makefile setup to build/test a lot on PLATFORM_WEB and it's been compiling alright, aside from those odd examples that may or may not necessarily be meant to run on PLATFORM_WEB or run without customizing the compilation.

2. @raysan5 This actually raises the question of if Makefile.Web is needed at all.

which caused one of the bug in the core/core_loading_thread

3. Upon review, looks like core/core_loading_thread probably shouldn't run on PLATFORM_WEB at all, as per L589. IMHO, it should probably be removed from Makefile.Web.

Note that the Makefile.Web does produce an output where it's a bit more zoomed in, exploded, whatever you wanna call it. But, this seems to be a pre-existing issue that is exposed by this fix.

4. This looks like an issue. If Makefile.Web is kept, this probably needs further inspection.

[keithstellyes]

1. That's an interesting find. I've been consistently using the current Makefile setup to build/test a lot on PLATFORM_WEB and it's been compiling alright, aside from those odd examples that may or may not necessarily be meant to run on PLATFORM_WEB or run without customizing the compilation.

2. raysan5 This actually raises the question of if Makefile.Web is needed at all.

Yeah, most of the targets are copy paste of the default calls, and none seem like they should really be a separate target

These modified calls being:

• Those that use TOTAL_MEMORY=67108864 (Seems unnecessary for this specific number versus the one already provided?)
• FORCE_FILESYSTEM=1 What if we just did that for all targets?
• USE_PTHREADS=1 (equivalent to -pthread referenced in comments, see emscripten docs) again, what if we just did that for all targets?

Seems it would be easy to just add those to the CC args in the base Makefile and delete it entirely?

3. Upon review, looks like core/core_loading_thread probably shouldn't run on PLATFORM_WEB at all, as per L589. IMHO, it should probably be removed from Makefile.Web.

I think the comment might not be completely accurate, I assume that comment is more in reference to the fact it's not by default?

Looking at the documentation:

The Web API for atomics does not allow blocking on the main thread [...]. Such blocking is necessary in APIs like pthread_join

To make them work, we use a busy-wait on the main browser thread, which can make the browser tab unresponsive, and also wastes power. (On a pthread, this isn’t a problem as it runs in a Web Worker, where we don’t need to busy-wait.)

Busy-waiting on the main browser thread in general will work despite the downsides just mentioned, for things like waiting on a lightly-contended mutex. However, things like pthread_join [...] are often intended to block for long periods of time, and if that happens on the main browser thread, and while other threads expect it to respond, it can cause a surprising deadlock. That can happen because of proxying[...]. If the main thread blocks while a worker attempts to proxy to it, a deadlock can occur.

To avoid these problems, you can use PROXY_TO_PTHREAD, which as mentioned earlier moves your main() function to a pthread, which leaves the main browser thread to focus only on receiving proxied events. This is recommended in general, but may take some porting work, if the application assumed main() was on the main browser thread.

Another option is to replace blocking calls with nonblocking ones. For example you can replace pthread_join with pthread_tryjoin_np. This may require your application to be refactored to use asynchronous events, perhaps through emscripten_set_main_loop() or Asyncify.

It seems the fix is to just do something like tryjoin or some other solution as mentioned. Let me attempt this to see if it's a complete fix. Should such a commit be added to this same PR?

[keithstellyes]

I am indeed able to get multiple threads running but I'm running into a very bizarre issue

    // We simulate data loading with a time counter for 5 seconds
    while (timeCounter < 5000)
    {
        clock_t currentTime = clock() - prevTime;
        timeCounter = currentTime*1000/CLOCKS_PER_SEC;

        // We accumulate time over a global variable to be used in
        // main thread as a progress bar
        atomic_store_explicit(&dataProgress, timeCounter/10, memory_order_relaxed);
    }

    atomic_store_explicit(&dataLoaded, true, memory_order_relaxed);

This loop never exits. Maybe the timeCounter never goes above 5000?

No, it definitely does. A logging message after the while loop never is hit either, which would mark the dataLoaded flag.

Also bizarrely enough, if I add a if(timeCounter > 4000) logging, it never gets logged at all:

    // We simulate data loading with a time counter for 5 seconds
    while (timeCounter < 5000)
    {
        if(timeCounter > 4000)
            TraceLog(LOG_ERROR, "time counter: %d", timeCounter);
        clock_t currentTime = clock() - prevTime;

It never actually outputs...

But change that 4000 to a 2000, and it will get logged:

I also tried to add logging to see if maybe the one thread was getting created multiple times, and some weird multithreading thing was happening, but nope:

I attempted all above with consistent hard refeshes, make -B, with and without the -s PTHREAD_POOL_SIZE=2 flag, and with replacing the atomic_store_explicit and atomic_load_explicit calls.

Quite bizarre, I'm guessing some weird thing is happening with multithreading

[ghost]

Yeah, most of the targets are copy paste of the default calls, and none seem like they should really be a separate target

5. Yeah, it looks redundant to keep the Makefile.Web at the moment.

• Those that use TOTAL_MEMORY=67108864 (Seems unnecessary for this specific number versus the one already provided?)

6. Agreed, I don't think that's necessary.

FORCE_FILESYSTEM=1 What if we just did that for all targets?

7. I'd vote againt that. As per "However, if your C/C++ code doesn't use files, but you want to use them from JavaScript, then you can build with -sFORCE_FILESYSTEM, which will make the compiler include file system support even though it doesn't see it being used." (doc). That would be a lot of extra weight (in an already footprint sensitive plataform) that, IMHO, shouldn't be on by default.

• USE_PTHREADS=1 (equivalent to -pthread referenced in comments, see emscripten docs) again, what if we just did that for all targets?

8. IMHO, that's a definitely no. As per "It is not possible to build one binary that would be able to leverage multithreading when available and fall back to single threaded when not. The best you can do is two separate builds, one with and one without threads, and pick between them at runtime." (doc). I really don't think that should be on by default, specially for web. IMHO, the users can enable it when they need (and want to deal with threads).

I think the comment might not be completely accurate, I assume that comment is more in reference to the fact it's not by default?

9. I think the warning is warranted. Check this and this. I don't think this feature is mature enough yet.

[keithstellyes]

Not assuming FORCE_FILESYSTEM and USE_PTHREADS makes sense, but this seems like it would mean Makefile.Web not redundant? Simplified maybe to just focus on the special cases needing params like that?

9. I think the warning is warranted. Check this and this. I don't think this feature is mature enough yet.

My issue is that I think it should be maybe reworded a bit, to me the warning sounds like threads are completely unsupported, when in reality they can definitely be created and used

[ghost]

Not assuming FORCE_FILESYSTEM and USE_PTHREADS makes sense, but this seems like it would mean Makefile.Web not redundant? Simplified maybe to just focus on the special cases needing params like that?

10. IMHO, they shouldn't be on by default. Exactly like the Makefile, where they already are turned off (L343-L344). The users could customize it as needed at L350.
    Edit 2: @keithstellyes I'm sorry, I was mixing the library src's Makefile compilation with the examples's Makefile. Just for the examples TOTAL_MEMORY and FORCE_FILESYSTEM there should be fine (L272), but definitely not for the library src (L350).

11. I don't think it's a problem some of the examples doesn't work out of the box, as long there's some comment, on those special cases, noting the required features that need to be enabled. That we could definitely improve where it's the case.

12. IMHO Makefile.Web could definitely be dropped.

My issue is that I think it should be maybe reworded a bit, to me the warning sounds like threads are completely unsupported, when in reality they can definitely be created and used

13. I see. Perhaps the wiki could be expanded to mention multithreading and what it entails. Although, personally, I still side with it being a potential security issue for PLATFORM_WEB until COOP/COEP/SharedArrayBuffer get more mileage.

Edit 1: editing.

Edit 3:

14. As a matter of fact, looks like we can completely remove this (L158-L160):

ifeq ($(PLATFORM),PLATFORM_WEB)
    MAKE = mingw32-make
endif

Tested deleting it here and the examples compiled fine.

[raysan5]

@ubkp I'm afraid Makefile.Web shouldn't be dropped because it compiles the examples defining the required resources per-example, the plain Makefile package the full resources directory for every example category, in some case many MB while just a few KB are required. Also, different examples have different requirements in terms of FILESYSTEM (that could be really resource hungry) or PTHREADS.

I couldn't find a better way to divide required resources per-example... Another solution would be using a separate directory and build system per example but I prefer to avoid that approach.

[raysan5]

@keithstellyes The improvement looks good to me, I didn't know about emmake, it's indeed a better option, independent of MinGW package.

@ubkp @keithstellyes One improvement on that build system (that I got on my list) would be customizing every generated example_name.html file from the input shell.html, to add the examples name, description and screenshots info in the header section, so examples could be shared on social networks on a fancy way (OpenGraph/Twitter metatags). But I don't know how it can be accomplished, probably an external tool would be required... basically it's a text replacement on output file.

[raysan5]

@keithstellyes I tried the new implementation and it does not work for me, how are you calling it? I try calling it from the predefined Notepad++ scripts:

mingw32-make core/core_basic_window PLATFORM=PLATFORM_WEB -B
Process started (PID=17472) >>>
emmake make -f Makefile.Web core/core_basic_window
make: make -f Makefile.Web core/core_basic_window
'make' is not recognized as an internal or external command,
operable program or batch file.
emmake: error: 'make -f Makefile.Web core/core_basic_window' failed (returned 1)
make: *** [Makefile:537: core/core_basic_window] Error 1
<<< Process finished (PID=17472). (Exit code 2)

[ghost]

@raysan5 That could be related to #3454. Between the changes there, I also changed the MAKE = mingw32-make to MAKE = emmake make (L152-R152) to make it inline with #3449 (L159-R159). I tested it compiling with emscripten/emsdk on Linux (Ubuntu 22.04 64-bit) with ~/raylib-master/examples$ make PLATFORM=PLATFORM_WEB.

Perhaps the problem could be notepad++'s npes_saved_mingw.txt script forcing mingw32-make at L127.

Note: Unfortunately I don't have a machine with Windows, so I can't test it.

[keithstellyes]

This PR originally changed it to emmake from mingw, not sure how it ended up back at mingw you look at the diff that's one of the couple lines that's changed

[raysan5]

@ubkp On Windows it used to work ok calling mingw32-make -f Makefile.web core/core_basic_window PLATFORM=PLATFORM_WEB but now it fails with the message I shared...

@keithstellyes I think emmake is actually a set of python calls, maybe there is some problem with paths?

[keithstellyes]

@keithstellyes I think emmake is actually a set of python calls, maybe there is some problem with paths?

Maybe the Makefile should use $(MAKE) -f instead?