-
Notifications
You must be signed in to change notification settings - Fork 760
Firefox support #1606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Firefox support #1606
Conversation
emsdk_manifest.json
Outdated
"version": "78.15.0esr", | ||
"bitness": 64, | ||
"url": "downloaded via mozdownload script, but a dummy directive is placed here so emsdk understands this Tool to be downloaded from the web", | ||
"activated_cfg": "EMTEST_BROWSER=%installation_dir%/firefox%.exe%", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EMTEST_BROWSER
is not a config file setting so I would hope this would not be needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately that is the only way currently to distinguish whether the tool is set to be active or not.
Without this directive, there is no record of whether user has set, say, emsdk activate firefox-78.15.0esr
or emsdk activate firefox-latest-esr-64bit
.
I'll rename this cfg directive to EMSDK_ACTIVATED_TEST_BROWSER
to help distinguish that is not actually used to set up EMTEST_BROWSER
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this directive, there is no record of whether user has set, say,
emsdk activate firefox-78.15.0esr
oremsdk activate firefox-latest-esr-64bit
.
It is important to track that? What purpose does it serve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise if you run emsdk_env
, it will activate all Firefoxes, so the very last one in emsdk_manifest.json will win.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess mean all the ff versions you have installed?
Its a shame we don't have any other way to track activated status, other than writing to the config file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess mean all the ff versions you have installed?
Err yeah, that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to add a lot of extra complexity, but I'm not sure I see the benefit outside of your specific internal CI setup.
Can we at least try to reduce the extra lines of code somehow? For example do we really need all those FF versions?
emsdk_manifest.json
Outdated
"activated_env": "EMTEST_BROWSER=%installation_dir%/firefox%.exe%", | ||
"mac_activated_cfg": "EMTEST_BROWSER=%installation_dir%/Contents/MacOS/firefox%.exe%", | ||
"mac_activated_cfg": "EMSDK_ACTIVATED_TEST_BROWSER=%installation_dir%/Contents/MacOS/firefox%.exe%", | ||
"mac_activated_env": "EMTEST_BROWSER=%installation_dir%/Contents/MacOS/firefox%.exe%", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we modify the extraction process to strip the Contents/MacOS
prefix, and avoid adding this new mac_activated_cfg
and mac_activated_env
concepts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are files in the other directories, e.g. Contents/Resources/
. I could maybe remove the Contents/
part, but that would be very non-Apple'esque, and any Apple native devs reading the code would likely read it to be a silly hack.
Users can
to test in any Firefox?
Yes, otherwise we blind our eyes from bugs, and saying we would support codegen down to Firefox 65 would be lip service. |
I'm not saying that testing old versions of FF is not good, I'm just not sure if emsdk is needs to be the way firefox gets installed. Up until now we have not installed browser, or stuff needed for testing only as part of emsdk. Would it possible perhaps to write a script that downloads FF, without complicating emsdk? |
This way I can spin up Emscripten testing the same way in Node, vs in Firefox.
Yes, but then I will have to go through hoops to install some other git repository in tandem with installing emsdk, which would be a massive hassle. With this I can install Firefox at the same line where I install rest of Emscripten tools. It can be of big value in user bug reports as well. |
Ping, further thoughts here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm since I don't want to keep blocking your progress.
But I do have some concerns:
I'm generally against adding a more untested code to emsdk (I do get it is being testing on your end but that doesn't stop future changes breaking it)
I don't love adding this many new entries to the manifest... maybe there is some way to templatize so we don't need to repeat ourselves so much? I get that its just a bunch of json, but I also find myself paging through it fairly often by hand and lots of mostly-unused (i.e. used only on your CI bots) entries are somewhat annoying here.
I would love to:
- Find a way to avoid the new
mac_activated_env
andmac_activated_cfg
which duplicate the non-man versions. Could magically also look inContents/MacOS
without needing the include this in the json? - Find some way to avoid the duplication in the many new json entries?
- Find a way another way to signal activation other than presence in the config file? We have a lot of tools now that don't ever want to be the
.emscripten
config file since emscripten itself doesn't use them. Perhaps a separatelyactivated.json
file that gets written separately to the config file?
Perhaps a secondary firefox_manifest.json
, perhaps even allowing your to maintain that externally?
This PR adds downloadable firefox packages into emsdk.
The downloading of firefox will be done via the
mozdownload
pip package, which simplifies the installation a lot.Installing and activating a Firefox package will set up the
EMTEST_BROWSER
variable, so running browser tests will then automatically use the activated browser.The oldest supported Firefox version by Emscripten is 65. So add that version, and all ESR versions from 65 upwards, and the moving channels. This allows people to go back and forth testing different versions of interest.