Remove unused internal setting: EMSCRIPTEN_VERSION. NFC #25661
Conversation
sbc100
force-pushed
the
remove_unused_settings
branch
from
54ff057 to
13176b9
Compare
|
Hmm, does(n't) removing this prevent this user code from working? If not, then LGTM. Though I suppose that's what this was for. (It would be nice to get a better construct for downstream users to #ifdef check the version. I think we had an open bug about that somewhere) |
|
I see. Its kind of shame that internal settings like this are exposed to user libraries. I was kind of hoping that internal settings we internal in the sense that we were free to add/remove/change them with impunity. But I guess it turns out that they are visible in this way to use JS libraries. |
|
I guess we should add test to |
I agree it would be good for settings_internal.js to be internal. FWIW, back in 2023 it seems like you were agreeing with manually parsing EMSCRIPTEN_VERSION as a good solution, #19469 (comment) to avoid depending on changes to Emscripten. Not sure if EMSCRIPTEN_VERSION was back then located in settings.js though. |
I proposed removing this internal setting in emscripten-core#25661 but it turns out it was useful for externally managed JS library files. However, I don't think it should be tested as part of emscripten_get_compiler_setting.c since C/C++ can already get the compiler version using VERSION macros, no need for the runtime check. Indeed I hope we can stop exposing any internal settings to emscripten_get_compiler_setting: emscripten-core#25663
I proposed removing this internal setting in emscripten-core#25661 but it turns out it was useful for externally managed JS library files. However, I don't think it should be tested as part of emscripten_get_compiler_setting.c since C/C++ can already get the compiler version using VERSION macros, no need for the runtime check. Indeed I hope we can stop exposing any internal settings to emscripten_get_compiler_setting: emscripten-core#25663
I proposed removing this internal setting in emscripten-core#25661 but it turns out it was useful for externally managed JS library files. However, I don't think it should be tested as part of emscripten_get_compiler_setting.c since C/C++ can already get the compiler version using VERSION macros, no need for the runtime check. Indeed I hope we can stop exposing any internal settings to emscripten_get_compiler_setting: emscripten-core#25663
I proposed removing this internal setting in #25661 but it turns out it was useful for externally managed JS library files. However, I don't think it should be tested as part of `emscripten_get_compiler_setting.c` since C/C++ can already get the compiler version using `VERSION` macros. . There should be no need for the runtime checks. Indeed I hope we can stop exposing any internal settings to emscripten_get_compiler_setting: #25663 Also, add test for @juj's specific use case for `EMSCRIPTEN_VERSION` in a JS library file.
|
That setting was intentionally added for this purpose, to be used as in that test. Someone asked for a way to get the emscripten version at runtime, and we had other requests for such things, so a general "get compiler setting" option seemed simplest, plus adding that option with the version in it. |
But the runtime version of emscripten and build time version are always the same right? By definition? At the build time version is available much more simply via I did a global github search and could only find one use of this.. which could easily be replaced by a build time check. |
|
Oh, are you saying there is an easy way to capture the build-time value and make it available at runtime? Yeah, I guess it could be saved as a string with a C preprocessor |
|
Seems reasonable. In that case I'm ok to remove this, but perhaps we could add to the changelog or docs an example of the other way. |
|
I decided to abandon this for now. Since its needed for @juj's external JS library use case. |
3 participants
I did have to update the test for
emscripten_get_compiler_settingwhich was using this internal settingsto test for compiler settings that return strings. I changed to a different (non-inernal) setting:
CLOSURE_WARNINGS.I think we should probably followup by making
emscripten_get_compiler_settingnot report internal setting since thewhole point of internal settings is that we should be free to remove/change them at any time without breaking any
user code. See #25663