Fix running emrun tests on Linux against the Linux system browser#26810
Conversation
…x emrun to work around Firefox https://bugzil.la/1996614
| if 'firefox' in browser_exe: | ||
| browser += ['-url', url] | ||
| else: | ||
| browser += [url] |
There was a problem hiding this comment.
With the old code the url was only added in the non-android case.
| if WINDOWS: | ||
| exe_file += '.exe' | ||
| if os.path.isfile(exe_file): | ||
| return exe_file |
There was a problem hiding this comment.
Can you just use shutil.which here?
If not, can you move this utility out the top level?
There was a problem hiding this comment.
Thanks, I wasn't aware of that - that's much better.
…ved to its own file.
|
|
||
| from tools.shared import EMCC, PIPE | ||
|
|
||
|
|
There was a problem hiding this comment.
The emrun class below is moved without changes.
|
|
||
| if not EMTEST_BROWSER: | ||
| EMTEST_BROWSER = 'google-chrome' | ||
| EMTEST_BROWSER = shutil.which('google-chrome') |
There was a problem hiding this comment.
Is this needed? IIRC this works as expected on my desktop without this change.
There was a problem hiding this comment.
Yes, because if I don't do that search, I don't know if I should search for firefox. I imagine we do want to favor google-chrome as the first default, if both are installed on the system?
There was a problem hiding this comment.
I think just having single default is fine. We have --browser=xxxx in addition to EMTEST_BROWSER=xxx that folks can use to specifiy non-default values.
There was a problem hiding this comment.
It would be convenient to have this automatically look up like this. This way I can run e.g. test/runner emrun on Raspberry Pi, finding Chrome as default, and the same command on Debian Linux, finding firefox as default.
This makes it simpler for me to not need to encode on the CI runner master script side the knowledge of "what browser do you have". I'd imagine this can help other users as well, who have a certain browser on their system. (and not have a FTUE that errors)
|
This is now ready to land, though there is an odd error from a fp16 test, that should not be related to this PR. I wonder how that failed. |
| if not EMTEST_BROWSER: | ||
| EMTEST_BROWSER = shutil.which('firefox') | ||
| if not EMTEST_BROWSER: | ||
| EMTEST_BROWSER = 'default-browser-not-found' |
There was a problem hiding this comment.
How about making this into a hard error?
There was a problem hiding this comment.
This code is run to detect a browser in the general test runner config, even if a browser is not actually run. So it was more straightforward to keep it as-is.
There was a problem hiding this comment.
I think we should fix that at some point. We shouldn't need to go through browser config unless we are running browser tests. I know there are some subtle issues with multiprocessing and startup here, so its maybe non-trival though.
There was a problem hiding this comment.
Ok, about about:
EMTEST_BROWSER = 'google-chrome'
if not shutil.which(EMTEST_BROWSER):
EMTEST_BROWSER = 'firefox'
if not shutil.which(EMTEST_BROWSER):
# FIXME: This should really be and error, but this code currently also runs for non-browser tests.
EMTEST_BROWSER = 'default-browser-not-found'
This way the value of EMTEST_BROWSER doesn't become and absolute path as part of this change.
| self.assertContained('Hello, world!', stdout) | ||
| self.assertContained('Testing ASCII characters: !"$%&\'()*+,-./:;<=>?@[\\]^_`{|}~', stdout) | ||
| self.assertContained('Testing char sequences: %20%21 ä', stdout) | ||
| self.assertContained('hello, error stream!', stderr) |
There was a problem hiding this comment.
Can you make the "move emrun into its own file" into separate PR?
There was a problem hiding this comment.
(Seems reasonable to combine with adding it to list of non-parall suites, since that is the motivation for moving it).
There was a problem hiding this comment.
I'd prefer not, that would cause more churn at this point.
| ini_path = find_system_firefox_platform_ini() | ||
| if not ini_path: | ||
| logger.warning(f'Firefox browser detected in {EMTEST_BROWSER}, but could not find Firefox platform.ini to detect Firefox version. Assuming OLDEST_SUPPORTED_FIREFOX={OLDEST_SUPPORTED_FIREFOX}') | ||
| return OLDEST_SUPPORTED_FIREFOX |
There was a problem hiding this comment.
Have you ever run into this issue?
Maybe we should just assert os.path.exists(init_path), 'unable to find firefox ini file' below before reading it?
There was a problem hiding this comment.
Yes, the CircleCI build-linux bot just ran into it.. that is why I added that.
It has /usr/bin/firefox but no platform.ini anywhere in sight.
On my own systems I do have platform.ini in /usr/lib/firefox-esr/platform.ini or /usr/lib/firefox/platform.ini
There was a problem hiding this comment.
I debugged CircleCI with a search from /usr/ to find if there's a platform.ini somewhere, but that turned up nothing.
8b43aae#diff-eb5e6400ad6f92fec8556d07ec1d8d14915c79814e87f354c5ab65e6163f6ad1R176-R177
There was a problem hiding this comment.
The firefox we use in circleci installed from tar archive in EMTEST_BROWSER="$HOME/firefox/firefox"? Is that the one you mean?
There was a problem hiding this comment.
has /usr/bin/firefox (shutil.which('firefox') returned /usr/bin/firefox), but does not have /usr/.../platform.ini anywhere.
EMTEST_BROWSER: /usr/bin/firefox -new-instance -wait-for-browser
Searching for platform.ini
/usr/local/lib/python3.10/dist-packages/numpy/typing/tests/data/mypy.ini
/usr/local/lib/python3.10/dist-packages/numpy/_core/lib/npy-pkg-config/npymath.ini
/usr/local/lib/python3.10/dist-packages/numpy/_core/lib/npy-pkg-config/mlib.ini
Searching for platform.ini over
Traceback (most recent call last):
File "/root/project/./test/runner.py", line 775, in <module>
sys.exit(main())
File "/root/project/./test/runner.py", line 732, in main
log_test_environment()
File "/root/project/./test/runner.py", line 655, in log_test_environment
print(f'Firefox version: {browser_common.get_firefox_version()}')
File "/root/project/test/browser_common.py", line 189, in get_firefox_version
m = re.search(r"^Milestone=(.*)$", read_file(ini_path), re.MULTILINE)
File "/root/project/tools/utils.py", line 157, in read_file
with open(file_path, encoding='utf-8') as fh:
FileNotFoundError: [Errno 2] No such file or directory: '/usr/bin/platform.ini'
There was a problem hiding this comment.
Now I see.. this task gives a clue: https://app.circleci.com/pipelines/github/emscripten-core/emscripten/50916/workflows/c9a01534-7402-4de2-aac7-028abe35de8e/jobs/1184421
It prints
Command '/usr/bin/firefox' requires the firefox snap to be installed.
Please install it with:
snap install firefox
so I wonder if there's a stub executable in /usr/bin/firefox present that prints out that message. So it is expected that there wouldn't exist any platform.ini on the system.
But now I'm puzzled a bit as to why this is detecting /usr/bin/firefox in the first place...
Oh that's because I wrote a silly typo.. fixed now.
There was a problem hiding this comment.
That CI job should not have EMTEST_BROWSER set at all.. its not trying to run any browser tests.
I guess your new code is causing that error because its now finding the /usr/bin/firefox even though it doesn't needed to for this CI step.
My guess is that /usr/bin/firefox is a symlink to somewhere like /snap/..., so maybe following the symlinks would fix it.
Ideally we could fix that TODO since then it would be moot point, but I know it wasn't easy last time I tried to move that code around.
I'm happy to iterate on this stuff in followups if you want to land this change now.
There was a problem hiding this comment.
Thanks, yeah hopefully after this lands, the instability from emrun suite will be gone.
Fix running emrun tests on Linux against the Linux system browser. This has a few fixes in one:
-urlto last on cmdline.test_emrun, so that it can be migrated to run single-threaded. This is because emrun may tear down the test browser in a non-parallel-friendly manner:emscripten/emrun.py
Lines 423 to 443 in 92ef17c
/usr/lib/, or fall back to assuming oldest supported browser if not found.