cmdline: Allow overriding -jsD directives#26639
cmdline: Allow overriding -jsD directives#26639subhaushsingh wants to merge 1 commit intoemscripten-core:mainfrom
Conversation
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
from
aff073f to
02426d8
Compare
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
Thanks!
lgtm % test comments
test/test_jslib.py
Outdated
| self.cflags += ['--js-library', test_file('jslib/test_jslib_custom_settings.js'), '-jsDCUSTOM_JS_OPTION=1'] | ||
| self.do_run_in_out_file_test('jslib/test_jslib_custom_settings.c') | ||
|
|
||
| self.run_process([EMCC, test_file('hello_world.c'), '-jsDCUSTOM_JS_OPTION=1', '-jsDCUSTOM_JS_OPTION=2']) |
There was a problem hiding this comment.
Can you replace these two lines with:
self.do_runf('jslib/test_jslib_custom_settings.c', '1\n')
# verify that the settings can be specified more then once, and that the last one wins.
self.do_runf('jslib/test_jslib_custom_settings.c', '2\n', cflags=['-jsDCUSTOM_JS_OPTION=2'])
Then you can also delete the test/jslib/test_jslib_custom_settings.out file
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
from
73088d2 to
9855548
Compare
|
@sbc100 It looks like the Codesize Checks are failing due to a pre-existing stale baseline on main (specifically test_codesize_cxx_lto.json showing a +206 byte drift). This seems unrelated to my -jsD logic. Could you please run the rebaseline-tests workflow on main to unblock the CI? Thanks! |
test/test_jslib.py
Outdated
| self.do_runf('jslib/test_jslib_custom_settings.c', '1\n') | ||
|
|
||
| # verify that the settings can be specified more than once, and that the last one wins. | ||
| self.do_runf('jslib/test_jslib_custom_settings.c', '2\n', cflags=['-jsDCUSTOM_JS_OPTION=2']) |
There was a problem hiding this comment.
Maybe the oder of the cflags is not correct there.
How about adding cflags=['-jsDCUSTOM_JS_OPTION=1'] to the first run then cflags=['-jsDCUSTOM_JS_OPTION=1', '-jsDCUSTOM_JS_OPTION=2'] to the second run.
Then the order will be obvious.
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
from
9855548 to
5da5234
Compare
|
Looks like the override test is still failing for some reason? |
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
2 times, most recently
from
eb5e07a to
ad32fb1
Compare
tools/cmdline.py
Outdated
|
|
||
| # Apply -jsD defines last so they win over -s args | ||
| for key, value in options.user_js_defines.items(): | ||
| settings[key] = value |
There was a problem hiding this comment.
Why can't think line be up on line 581? User defined settings cannot overlap with normal -s settings so I don't see why we would care about competing with them, right?
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
2 times, most recently
from
70faeb6 to
1c13494
Compare
tools/cmdline.py
Outdated
| should_exit = False | ||
| skip = False | ||
| builtin_settings = set(settings.keys()) | ||
| user_js_defines = set() |
There was a problem hiding this comment.
I think maybe we no longer require user_js_defines now?
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
3 times, most recently
from
d0e21ba to
2a9cf37
Compare
tools/cmdline.py
Outdated
| exit_with_error(f'{arg}: cannot change built-in settings values with a -jsD directive. Pass -s{key}={value} instead!') | ||
| # Apply user -jsD settings | ||
| if key in builtin_settings and key in settings: | ||
| if str(settings[key]) != value: |
There was a problem hiding this comment.
I don't think we need to compare the value here.
I think just if key in builtin_settings: should be enough. i.e. it should always be not allowed to do -jsDASSERTIONS=value regardless of the value passed
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
2 times, most recently
from
28591cd to
059ee71
Compare
sbc100
changed the title
Head branch was pushed to by a user without write access
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
2 times, most recently
from
83dde76 to
7bd8181
Compare
subhaushsingh
force-pushed
the
fix-jsd-override-26576
branch
from
7bd8181 to
4c47d6a
Compare
|
Sorry for the messy commit history, still getting familiar with this codebase. Thanks for the patience @sbc100. The fix captures builtin_settings before processing any -jsD flags to allow duplicates, and the JS test fixture now uses {{{ CUSTOM_JS_OPTION }}} to return the actual value. |
2 participants
The Problem
-jsDFOO=1 -jsDFOO=2triggers an error:cannot change built-in settings values with a -jsD directive.FOOto the internalsettingsdictionary. When the second flag appears, the compiler mistakenly thinks a protected system setting (likeWASM) is being overwritten.The Fix
user_js_defines: A set intools/cmdline.pyto track which keys were introduced by the user during the current run.-jsDin this session.Verification
test/test_jslib.pythat specifically tests duplicate/overridden-jsDflags.-jsDWASM=0) still correctly fails as intended.Fixes: #26576