Cleaning loading of settings from .js file. #7499
Conversation
| @@ -7953,7 +7953,6 @@ def setUp(self): | |||
| os.chdir(self.get_dir()) # Ensure the directory exists and go there | |||
|
|
|||
| self.emcc_args = emcc_args[:] | |||
| Settings.load(self.emcc_args) | |||
There was a problem hiding this comment.
Without this, we won't notice pre-existing changes to self.emcc_args, I think? E.g. in asm2g we set ASSERTIONS on via emcc_args.
(to make sure I'm not missing something, I diffed the Settings serialization before and after this call, and it does look different)
There was a problem hiding this comment.
I believe the ever since #6801, the contents of the Settings object in the test runner is not used. If the tests themselves use get_setting and set_setting and then only those settings get serialized onto the emcc command line. So anything you do the Settings object in the test process is not relevant.
There was a problem hiding this comment.
Actually you were right.. its little more subtle. get_settings() has the return the right thing. I update the test suites to be explicit about their settings.
There was a problem hiding this comment.
Hmm, I think there might be even more here - -O1 and above will set ASSERTIONS=0. I think with the current code here, that would not be set?
There was a problem hiding this comment.
OK I think I ironed out all the issues. I think this should be good to land now.
sbc100
force-pushed
the
cleanup_settings_loading
branch
3 times, most recently
from
eba8b27
to
b3bd95f
Compare
- Don't load settings from emcc_args in test runnter. We use set_setting/set_setting/serailize_settings to apply test settings. - Use exec with second argument to avoid accidental side effects - remove Settings.load method and merge it with init
sbc100
force-pushed
the
cleanup_settings_loading
branch
from
b3bd95f
to
e245595
Compare
…mscripten-core#8382)" This reverts commit b56a8c7.
…mscripten-core#8382)" This reverts commit b56a8c7.
We use set_setting/set_setting/serailize_settings to apply test
settings.