[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...
|
I don't understand why @sbc100 Do you think other things look fine? |
| ''') | ||
| create_file('path_list', r''' | ||
| create_file('path_list.txt', r''' | ||
| myapp |
There was a problem hiding this comment.
nit: Can you end these lines with : maybe? I feel like it would make them easier to read if I could clearly see the different between module names and filenames.
There was a problem hiding this comment.
I'd like to make the file format similar to that of wasm-split's multi split manifest, which does not have colons: https://github.com/WebAssembly/binaryen/blob/main/test/lit/wasm-split/multi-split.wast.manifest
There was a problem hiding this comment.
Fair enough, maybe we could update them both one day?
There was a problem hiding this comment.
I don't mind much, but wasm-split's manifest file already has some users. If we decide to do it, I think we can give some internal clients a heads-up though. WDYT? @tlively
There was a problem hiding this comment.
Will land this PR for now then.
There was a problem hiding this comment.
I don't mind much, but wasm-split's manifest file already has some users. If we decide to do it, I think we can give some internal clients a heads-up though. WDYT? @tlively
Making improvements to the manifest file format sounds good to me. I imagine we might want to make more changes in the future as well, for example to include information about dependencies between modules.
Suggested by by @sbc100 (emscripten-core/emscripten#25577 (comment)). Currently this supports both module names with a `:` and without it for backwards compatibility and also to pass the CI.
This makes better distinction of module names and function lists. Suggested by by @sbc100 (emscripten-core#25577 (comment)). This has to land after WebAssembly/binaryen#8003.
4 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.