[empath-split] Support multi-paths modules #25577
Conversation
This changes the format of the user input "paths" file so that multiple paths can be split into a single module. The new paths file structure is now very similar to wasm-split's manifest file, with functions replaced with paths. For example, the format will be like ``` module1 path/to/a path/to/b module2 path/to/c ``` Where `module1` and `module2` are module names.
tools/empath-split.py
Outdated
|
|
||
| # Write .manifest file | ||
| with tempfile.NamedTemporaryFile(suffix=".manifest", mode='w+', delete=args.preserve_manifest) as f: | ||
| with tempfile.NamedTemporaryFile(suffix=".manifest", mode='w+', delete=not args.preserve_manifest) as f: |
There was a problem hiding this comment.
Drive-by fix. --preserve-manifest was doing the opposite of what it should do...
There was a problem hiding this comment.
Apparently this drive-by fix is seems to be a reason that Windows CI is failing: https://app.circleci.com/pipelines/github/emscripten-core/emscripten/46843/workflows/78cfd5c9-b267-45b7-ac41-6669c0b2adb6/jobs/1058317
It looks in Windows it is not allowed to re-open an already opened file unless some conditions are met: https://docs.python.org/3/library/tempfile.html
Opening the temporary file again by its name while it is still open works as follows:
- On POSIX the file can always be opened again.
- On Windows, make sure that at least one of the following conditions are fulfilled:
deleteis false- additional open shares delete access (e.g. by calling os.open() with the flag
O_TEMPORARY)deleteis true butdelete_on_closeis false. Note, that in this case the additional opens that do not share delete access (e.g. created via builtin open()) must be closed before exiting the context manager, else the os.unlink() call on context manager exit will fail with a PermissionError.
The Windows CI didn't error out before this PR because the first condition (delete is false) was accidentally satisfied due to this bug. To fix this I think we should use try-finally. Will do this in another PR, and will revert this drive-by fix here.
test/test_other.py
Outdated
| @@ -15760,9 +15760,14 @@ def test_empath_split(self): | |||
| void foo() { std::cout << "foo" << std::endl; } | |||
| ''') | |||
| create_file('path_list', r''' | |||
There was a problem hiding this comment.
Should we call this path_list.txt ?
tools/empath-split.py
Outdated
| f.write(func + '\n') | ||
| if i < len(paths) - 1: | ||
| if i < len(module_to_paths) - 1: | ||
| f.write('\n') |
There was a problem hiding this comment.
Maybe instead do this at the top with:
if i != 0: // Unless we are the first entry add a newline separator
f.write('\n')
| for func in path_to_funcs[path]: | ||
|
|
||
| f.write(f'{module}\n') | ||
| for func in funcs: |
There was a problem hiding this comment.
Annoyingly If you want this to be deterministic then I think you need to make this a dict rather than set.
The dict in python became deterministic in 3.7 but the set remains unordered. Annoying..
There was a problem hiding this comment.
Changed it and also one more place from set to list: 7eedf6d
Apparently they didn't even need to be sets after all...
3 participants
This changes the format of the user input "paths" file so that multiple paths can be split into a single module. The new paths file structure is now very similar to wasm-split's manifest file, with functions replaced with paths. For example, the format will be like
Where
module1andmodule2are module names.