Skip to content

Cache the fact that sanity checks were performed for subprocesses #6124

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.

Sign up for GitHub

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

Closed
atrosinenko wants to merge 3 commits into from
Closed

Cache the fact that sanity checks were performed for subprocesses #6124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1dbc26b
Cache sanity check results and skip checks when possible
atrosinenko Jul 24, 2017
c79e4ed
More work on sanity check result caching
atrosinenko Jan 21, 2018
571e4d2
Simplify PR as suggested by kripken & add some force=True args
atrosinenko Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions embuilder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ def build(src, result_libs, args=[]):
def build_port(port_name, lib_name, params):
build(C_BARE, [os.path.join('ports-builds', port_name, lib_name)] if lib_name else None, params)

# Check once and cache results
shared.check_sanity()

operation = sys.argv[1]

Expand Down
2 changes: 1 addition & 1 deletion src/struct_info.compiled.json
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions tests/runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,9 @@ def setUp(self):
if temp_file.endswith('.ll'):
self.has_prev_ll = True

# Cache check results

@kripken kripken Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this for each test is more than we need, I think. We can just do it once at the very beginning, at the top level?

@atrosinenko atrosinenko Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I probably put it in the wrong place. I thought to place it to the global setup function.

check_sanity(force=True)

def tearDown(self):
if not self.save_dir:
# rmtree() fails on Windows if the current working directory is inside the tree.
Expand Down
5 changes: 5 additions & 0 deletions tests/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7926,6 +7926,11 @@ def test_wasm_backend(self):
if old == '1': return # already the default
try:
os.environ['EMCC_WASM_BACKEND'] = '1'
if EMCC_SKIP_SANITY_CHECK_NAME in os.environ:
# Cached sanity check result was for different configuration
del os.environ[EMCC_SKIP_SANITY_CHECK_NAME]

@kripken kripken Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's usually better to skip the test with a message than to modify the environment.

in any case, though, I don't think we need to check sanity here?

@atrosinenko atrosinenko Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand, we already skip this test if no environment modification (that was in place before this PR) is required. :) As you already noted, we don't need to perform checks before every test case. I redo sanity check for this particular test case because here we artificially changed the condition on which sanity check control flow depends (in fact, I don't handle very well another input: the force function argument here and in the other places).

check_sanity(force=True)

for args in [[], ['-O1'], ['-O2'], ['-O3'], ['-Os'], ['-Oz']]:
print(args)
run_process([PYTHON, EMCC, path_from_root('tests', 'hello_world.cpp')] + args)
Expand Down
4 changes: 4 additions & 0 deletions tests/test_sanity.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,17 @@ def setUpClass(self):
print('WARNING: This will modify %s, and in theory can break it although it should be restored properly. A backup will be saved in %s_backup' % (EM_CONFIG, EM_CONFIG))
print()

os.environ[EMCC_SKIP_SANITY_CHECK_NAME] = '-1'

@kripken kripken Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1?

@atrosinenko atrosinenko Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this -1 looks just like a typo, but in fact, it (and every other string) has opposite meaning to 1. It would be better to replace it with os.environ[EMCC_SKIP_SANITY_CHECK_NAME] = 'dontskip'

assert os.path.exists(CONFIG_FILE), 'To run these tests, we need a (working!) %s file to already exist' % EM_CONFIG
assert not os.environ.get('EMCC_DEBUG'), 'do not run sanity checks in debug mode!'
assert not os.environ.get('EMCC_WASM_BACKEND'), 'do not force wasm backend either way in sanity checks!'

@classmethod
def tearDownClass(self):
super(RunnerCore, self).tearDownClass()
if EMCC_SKIP_SANITY_CHECK_NAME in os.environ:
del os.environ[EMCC_SKIP_SANITY_CHECK_NAME]
check_sanity(force=True)

def setUp(self):
wipe()
Expand Down
13 changes: 12 additions & 1 deletion tools/shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,11 +483,16 @@ def get_emscripten_version(path):
def generate_sanity():
return EMSCRIPTEN_VERSION + '|' + LLVM_ROOT + '|' + get_clang_version() + ('_wasm' if Settings.WASM_BACKEND else '')

EMCC_SKIP_SANITY_CHECK_NAME = 'EMCC_SKIP_SANITY_CHECK'

def check_sanity(force=False):
ToolchainProfiler.enter_block('sanity')
try:
if os.environ.get('EMCC_SKIP_SANITY_CHECK') == '1':
if os.environ.get(EMCC_SKIP_SANITY_CHECK_NAME) == '1':
return
if EMCC_SKIP_SANITY_CHECK_NAME not in os.environ:
os.environ[EMCC_SKIP_SANITY_CHECK_NAME] = '1'

reason = None
if not CONFIG_FILE:
return # config stored directly in EM_CONFIG => skip sanity checks
Expand Down Expand Up @@ -1497,6 +1502,9 @@ def has_substr(array, substr):

@staticmethod
def configure(args, stdout=None, stderr=None, env=None):
# Check once and cache results
check_sanity()

@kripken kripken Feb 12, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these also seem unnecessary?

@atrosinenko atrosinenko Feb 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, according to comments, emconfigure (this configure function is its main function, yes?) performs builds with regular clang... Maybe it is really unnecessary here. On the other hand, would it take significant amount of time to check sanity once in case some day emcc will eventually be used instead of clang (meanwhile, is native clang invoked directly or proxyed by emcc.py now?).


if not args:
return
if env is None:
Expand All @@ -1523,6 +1531,9 @@ def configure(args, stdout=None, stderr=None, env=None):

@staticmethod
def make(args, stdout=None, stderr=None, env=None):
# Check once and cache results
check_sanity()

if env is None:
env = Building.get_building_env()
if not args:
Expand Down