[ports] upgrade SDL to 2.28.4

[ericoporto]

This is just to update the SDL2 port, last time it was a long time in #18026 .

There are only two files added from what I can tell, events/SDL_keysym_to_scancode.c and events/SDL_scancode_tables.c, and no files removed, when comparing with the previous 2.24.2.

The removed TODO has been done in a PR on SDL2 by @sbc100 , and the commit is included from 2.28.4 and forward.

[sbc100]

That is odd.. normally the circleci job run on the emscripten account when you send us a PR...

[ericoporto]

@sbc100 I don't know why it happens with me, but it happened the other time too, and when it happens the build-linux and test-mac-arm64 can't run because they use paid features. :/

Btw, I deleted my Emscripten fork and recreated, and the circle CI project too, before making this PR. (I thought this would make it work this time)

I also see a message "This branch is out-of-date with the base branch", but this is fairly isolated, just let me know if there's a need to rebase any time and I will adjust.

[ericoporto]

Hey, it would be great to have this before we get to 3.1.48 release :)

[sbc100]

I think the only thing missing is the CI.

Can you try delete the emscripten project from your personal circleci account and then rebasing this PR to trigger a re-run?

[sbc100]

the browser test failure here look real. e.g. : test_sdl2_pumpevents

[ericoporto]

Hi @sbc100 , turns out I can't recall how to use my fork locally, so I can actually try the tests (I thought I did, but turns out I was running the main from here and didn't pay attention). I opened an issue to document this emscripten-core/emsdk#1291

[ericoporto]

I noticed a few sdl2 really old headers floating around

emscripten/system/include/SDL/SDL_touch.h

Line 24 in 3319a31

This header taken from SDL2

Doing a git grep will find more. Are these a typo or are these used in any way here?

[sbc100]

I noticed a few sdl2 really old headers floating around

emscripten/system/include/SDL/SDL_touch.h

Line 24 in 3319a31

This header taken from SDL2

The headers in that directory are for SDL1, not SDL2. They should not effect the SDL2 port at all (I hope).

[sbc100]

There has been some recent progress here. Last week I landed #21337 which updated SDL2 port to 2.26.0. As part of that I fixed a few issues related to scancodes and keycodes.

I have another PR up to bump the version further but its not currently passing all tests #20268. If you have time to continue working on this that would be most welcome. Otherwise I will probably pick up that change next week some time.

[ericoporto]

Hey @sbc100 , all tests passed :)

[sbc100]

When I was working on #20268 I noticed that some of the sdl audio or sdl mixer tests were failing locally for me. The audio tests don't run as part of the CI sadly so you have to run them locally. Can you try running them?

[ericoporto]

I can't, I still can't figure how to run my Emscripten fork with my own cloned Emsdk. The emsdk install will put a upstream/emscripten dir that contains something that was unzipped, and that is not a repository and it can't be checked as described here. Also it appears it requires to be able to build llvm from source (???).

I still find it weird that I can't just symlink and run something local instead of having to push things to a git repository somewhere...

[sbc100]

I can't, I still can't figure how to run my Emscripten fork with my own cloned Emsdk.

The simplest way to do this is to export EM_CONFIG=/path/to/emsdk/.emscripten and then just run ./emcc to ./test/runner from your emscripten chekcout.

This will cause your emscripten checkout to use all the prebuilt tools from emsdk.

We should really document this better. I thought it was...

[sbc100]

Ah yes here it is: https://emscripten.org/docs/contributing/developers_guide.html

[ericoporto]

I am sorry, but I didn't fork the emsdk, I forked emscripten... Is this a typo in ALL the docs? When it writes

export EM_CONFIG=/path/to/emsdk/.emscripten

THIS REALLY should be export EM_CONFIG=/path/to/emscripten/.emscripten ??

Uhm. It appears it is, but then I get and error.

runner: error: BINARYEN_ROOT is set to empty value in C:\Users\user\git\ericoporto\emscripten\.emscripten

In the .emscripten file I see a part where it's like so

...
EMSCRIPTEN_ROOT = 'C:\\Users\\user\\git\\ericoporto\\emscripten' # directory

LLVM_ROOT = '/usr/bin' # directory
BINARYEN_ROOT = '' # directory
...

Retrying using the below with the env var set.

./emsdk install latest-upstream
./emsdk activate latest-upstream

well... Nope, still empty...

just run ./emcc to ./test/runner

both the left command and the right command will fail because binaryen root is empty. I tried running through all the issues and just found emscripten-core/emsdk#418 where reading the second comment feels like https://xkcd.com/979/ .

Well apparently after rebasing now something failed?? It was just one commit difference in the rebase...

[sbc100]

Right, when you fork emscripten you can use the emsdk config file to easily get access the binaryen and llvm from emsdk while still using your fork of emscripten (that can be anywhere in the filesystem).

So export EM_CONFIG=/path/to/emsdk/.emscripten is correct. That is the file that emsdk creates when you run ./emsdk activate.

If you use the emsdk config file you don't need a .emscripten file in your emscripten checkout at all.

Then you can run tests in your emscripten checkout such as ./test/runner browser.test_sdl2_image. You can also use wildcards such as ./test/runner browser.test_sdl2_*.

[sbc100]

That test failure looks like a flake. I re-ran it.

[ericoporto]

So export EM_CONFIG=/path/to/emsdk/.emscripten is correct.

I don't understand this, why my emsdk needs an environment variable to find a file in it's own path?

Anyway, this means the truth is I should edit something in the file .emscripten where I cloned emsdk repo to point to where I cloned emscripten repo? Can you tell me what is the variable in this file that requires such value?

[sbc100]

So export EM_CONFIG=/path/to/emsdk/.emscripten is correct.

I don't understand this, why my emsdk needs an environment variable to find a file in it's own path?

The purpose of the EM_CONFIG environment variable is to tell your fork of emscripten to use the emsdk-generated config file.

If you use emsdk normally you don't need to do this.

By setting this environment variable you are saying "no matter which version of emscripen I use, always use llvm and binaryen from emsdk".

Anyway, this means the truth is I should edit something in the file .emscripten where I cloned emsdk repo to point to where I cloned emscripten repo? Can you tell me what is the variable in this file that requires such value?

emsdk is in charge or creating its own config file. It does this when you run the activate command. You should never need to edit this.

Neither emsdk not its config file need to know how many different copies of emscripten your have or where they live.

You can just use your cloned copy of emscripten directly via ./emcc or via ./test/runner.py. When you run those commands with ./ (or the full path to them) they will (as you would imagine) run the the local version emscripten.. but when they need to use tools like LLVM they will look in the config file (which you specified by setting EM_CONFIG).

You can also put your cloned copy of emscripten first in your PATH if you want to have it run when you type just emcc (without the pathname). I tend not to do this myself and instead just run ./emcc or ./test/runner.py. This avoids having to worry about what is in PATH.

Apologies if this is non-intuitive. I've tried to make it better over time but perhaps there are more ways was can improve this. In any case this setup works well once you understand what is going on (I think).

[ericoporto]

ok, I think I understood now and did the following. First navigate to emsdk directory where I cloned it.

./emsdk install latest
./emsdk activate latest

Now I just go where I cloned emscripten using cd ../emscripten

export EM_CONFIG=../emsdk/.emscripten
./test/runner browser.test_sdl2_*

Now I finally get a different error

$ ./test/runner browser.test_sdl2_*
Test suites:
['test_browser']
Running test_browser: (48 tests)
(checking sanity from test runner)
shared:INFO: old sanity: 3.1.25-git|C:/Users/user/git/ericoporto/ags/Emscripten/emscripten/emsdk/upstream/bin|16.0|0x79932be2
shared:INFO: new sanity: 3.1.55-git|../emsdk/upstream/bin
shared:INFO: (Emscripten: config changed, clearing cache)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 15252]
common:INFO: No EMTEST_BROWSER set. Defaulting to `google-chrome`
common:INFO: Launching browser: ['google-chrome']
ERROR

======================================================================
ERROR: setUpClass (test_browser.browser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Users\user\git\ericoporto\emscripten\test\test_browser.py", line 211, in setUpClass
    super().setUpClass()
  File "C:\Users\user\git\ericoporto\emscripten\test\common.py", line 1955, in setUpClass
    cls.browser_open(cls.harness_url)
  File "C:\Users\user\git\ericoporto\emscripten\test\common.py", line 1940, in browser_open
    cls.browser_proc = subprocess.Popen(browser_args + [url])
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.9_3.9.3568.0_x64__qbz5n2kfra8p0\lib\subprocess.py", line 951, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.9_3.9.3568.0_x64__qbz5n2kfra8p0\lib\subprocess.py", line 1420, in _execute_child
    hp, ht, pid, tid = _winapi.CreateProcess(executable, args,
FileNotFoundError: [WinError 2] The system cannot find the file specified

I do have google-chrome, and firefox, and microsoft edge installed.

I got an error when I tried to set my google chrome location, but also I can't tell if it understood where chrome was from the log - note the argument broken for whatever reason there. It did launch a tab with http://localhost:8888/run_harness but it stopped.

 EMTEST_BROWSER="/c/Program Files/Google/Chrome/Application/chrome.exe" ./test/runner browser.test_sdl2_*
Test suites:
['test_browser']
Running test_browser: (48 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 15272]
common:INFO: Launching browser: ['C:/Program', 'Files/Google/Chrome/Application/chrome.exe']

Running the browser tests. Make sure the browser allows popups from localhost.

test_sdl2_canvas_blank (test_browser.browser) ... emcc: warning: config file not found: ../emsdk/.emscripten.  You can create one by hand or run `emcc --generate-config`
emcc: error: LLVM_ROOT not set in config (../emsdk/.emscripten), and `clang` not found in PATH

Using EMTEST_BROWSER='"/c/Program Files/Google/Chrome/Application/chrome.exe"' cause it to not find google chrome instead...

[ericoporto]

Ok, it appears the problem is everything here doesn't support using BASH in MSYS2 shell. I switched to using cmd.exe instead. Now all tests failed, it appears is some problem with npm packages?

https://gist.github.com/ericoporto/6ac10f72f8787439cde0c4482d8b5e84

Running .\bootstrap.... Ok, it appears it's working now.

@sbc100 I reproduced!!!

https://gist.github.com/ericoporto/437749961e72a477a6fb87643bb33162

The last test called test_sdl2_mixer_music_mod just stops working in the middle and doesn't produce any sound. Also the test_sdl2_mixer_music_mp3 doesn't work either. I can't find a file that has such name, not so sure what code is the test runner running exactly...

[sbc100]

Great news that you got it working.

It would be good to know if these tests failures you are seeing are new or pre-existing. I suspect they are new since that is what I was seeing in my other PR but it would be good to verify that they work against main.

For tests that don't correspond to a specific filename just look for the test name in test_browser.py to see what it does. See

emscripten/test/test_browser.py

Lines 3428 to 3442 in e6be4f5

	def test_sdl2_mixer_music(self, formats, flags, music_name):
	shutil.copyfile(test_file('sounds', music_name), music_name)
	args = [
	'--preload-file', music_name,
	'-DSOUND_PATH="%s"' % music_name,
	'-DFLAGS=' + flags,
	'-sUSE_SDL=2',
	'-sUSE_SDL_MIXER=2',
	'-sSDL2_MIXER_FORMATS=' + ','.join(formats),
	]
	# libmodplug is written in C++ so we need to link in C++
	# libraries when using it.
	if 'mod' in formats:
	args += ['-lc++', '-lc++abi']
	self.btest_exit('test_sdl2_mixer_music.c', args=args)

In this case this test has 4 different variants via @parameterize which give you the _wav, _ogg, _mp3 and _mod suffixes.

(Regarding windows + bash + msys2 .. its not suprising to me that stuff doesn't just work there/ (at least that has been our experience). Its pretty tricky to get right since you have to also worry about some folks using windows python and some folks using msys python. It gets really complicated really quickly, and we don't have any windows users on the core emscripten team sadly. So all that stuff with MSYS and cygwin is mostly best effort supported by users who use those environments).

[ericoporto]

The bash MSYS2 shell I use is the one that installs when you install git-bash in the git for Windows. I have python installed by running the installer (I am aware of the Windows Store python). My experience running this bash in Windows Terminal is that everything usually works.

Most of the problems in Emscripten is that the command line code is hand-rolled instead of using some standard command line handling like argparse and paths are not normalized with pathlib. This is usually simply a reflection of the machine of the developers that are mainly contributing to the repository and it's fine - probably people aren't using Windows machines and instead using macOS or Linux - or they learned to do it in Windows at some point but because they do it everyday there was no need to document. It's just there are lot of assumptions that the Windows user will be using cmd.exe, but at the same time the documentation is missing for it. Anyway, that's why I wrote every step I took so at least I can look into to reproduce it later for myself. :)

Anyway, I will take a look later more in depth to try to figure it out (today is Sunday!), my first approach will be just to cut the main.c test code to a new project and build this sdl2 release using Emscripten+CMake in CLion to verify that the issue is not something that works with sdl2 in upstream and doesn't here for some difference in the build process.

cd emsdk
.\emsdk install latest
.\emsdk activate latest
cd ..\emscripten
setx EM_CONFIG C:\Users\path\to\emsdk\.emscripten
set EM_CONFIG=C:\Users\path\to\emsdk\.emscripten
set EMTEST_BROWSER="C:\Program Files\Google\Chrome\Application\chrome.exe"

[ericoporto]

@sbc100 I figured! The problem is very erh, simple, if you click the black area in Emscripten, the sound plays and the test pass. I think it's just that the browser requires user input.

.\test\runner browser.test_sdl2_mixer_music_mp3
Test suites:
['test_browser']
Running test_browser: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 1468]
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']

Running the browser tests. Make sure the browser allows popups from localhost.

test_sdl2_mixer_music_mp3 (test_browser.browser) ... [unresponsive tests: 1]
common:INFO: Restarting browser process
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']
[test error (see below), automatically retrying]
Expected to find '/report_result?exit:0
' in '[no http server activity]
', diff:

--- expected
+++ actual
@@ -1 +1 @@
-/report_result?exit:0
+[no http server activity]


ok
[Browser harness server terminated]

----------------------------------------------------------------------
Ran 1 test in 71.359s

OK

Btw, this behavior is something I observed previous with the Emscripten OpenAL js implementation, it seems it magically works without clicking, but it requires clicking in macOS Safari before playing sound and on my iPhone, using Safari, I can't get sound from the Emscripten OpenAL js.

[ericoporto]

OK, managed to reproduce with the current 2.26.0 in main. Here, the first run I accidentally clicked and then it run ok, lol.

.\test\runner browser.test_sdl2_mixer_music_mp3
Test suites:
['test_browser']
Running test_browser: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 20712]
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']

Running the browser tests. Make sure the browser allows popups from localhost.

test_sdl2_mixer_music_mp3 (test_browser.browser) ... ports:INFO: retrieving port: sdl2 from https://github.com/libsdl-org/SDL/archive/release-2.26.0.zip
ports:INFO: unpacking port: sdl2
cache:INFO: deleting cached file: C:\Users\user\git\ericoporto\emscripten\cache\sysroot\lib\wasm32-emscripten\libSDL2.a
cache:INFO: generating port: sysroot\lib\wasm32-emscripten\libSDL2.a... (this will be cached in "C:\Users\user\git\ericoporto\emscripten\cache\sysroot\lib\wasm32-emscripten\libSDL2.a" for subsequent builds)
system_libs:INFO: compiled 115 inputs in 4.59s
cache:INFO:  - ok
cache:INFO: generating system asset: symbol_lists/41d823f5b680041a7bf9eee4e174a9d89fa411fd.json... (this will be cached in "C:\Users\user\git\ericoporto\emscripten\cache\symbol_lists\41d823f5b680041a7bf9eee4e174a9d89fa411fd.json" for subsequent builds)
cache:INFO:  - ok
ok
[Browser harness server terminated]

----------------------------------------------------------------------
Ran 1 test in 40.254s

OK

But then, I reran and this time hold my mouse. Once it retries, then I click and managed to make the test pass.

.\test\runner browser.test_sdl2_mixer_music_mp3
Test suites:
['test_browser']
Running test_browser: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 14312]
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']

Running the browser tests. Make sure the browser allows popups from localhost.

test_sdl2_mixer_music_mp3 (test_browser.browser) ... [unresponsive tests: 1]
common:INFO: Restarting browser process
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']
[test error (see below), automatically retrying]
Expected to find '/report_result?exit:0
' in '[no http server activity]
', diff:

--- expected
+++ actual
@@ -1 +1 @@
-/report_result?exit:0
+[no http server activity]


ok
[Browser harness server terminated]

----------------------------------------------------------------------
Ran 1 test in 118.820s

OK

[sbc100]

Do all test_sdl2_* test pass? I'm particularly interested in the ones that are tagged with @requires_sound_hardware which don't run in CI

[sbc100]

lgtm assuming all the sound tests pass

[ericoporto]

Here are my logs for browser.test_sdl2_mixer_music_{mp3,mod,ogg,wavl}: https://gist.github.com/ericoporto/e02548f630f1142354021526e2173176

The standalong browser.test_sdl2_mixer_wav worked too

.\test\runner browser.test_sdl2_mixer_wav
Test suites:
['test_browser']
Running test_browser: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 20152]
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']

Running the browser tests. Make sure the browser allows popups from localhost.

test_sdl2_mixer_wav (test_browser.browser) ... ok
[Browser harness server terminated]

----------------------------------------------------------------------
Ran 1 test in 5.950s

OK

I also ran ok the browser.test_sdl_audio_beep_sleep (but I think this is sdl1?). This one you have to click the black rectangle right away or it fails.

.\test\runner browser.test_sdl_audio_beep_sleep
Test suites:
['test_browser']
Running test_browser: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
[Browser harness server on process 16240]
common:INFO: Launching browser: ['C:\\Program Files\\Google\\Chrome\\Application\\chrome.exe']

Running the browser tests. Make sure the browser allows popups from localhost.

test_sdl_audio_beep_sleep (test_browser.browser) ... ok
[Browser harness server terminated]

----------------------------------------------------------------------
Ran 1 test in 57.522s

OK

I think these are all requires_sound_hardware.

You can try during the week too and see what you think, there is no rush.

[sbc100]

Can you revert this line? (We don't update copyright years)

[ericoporto]

I wasn't sure if it was something here too, so I had as a separate commit. I just dropped it!

Ah, if you guys ever want to upgrade, perhaps a script similar to this could be useful: https://github.com/adventuregamestudio/ags/blob/master/Script/push_codeheader.py