Skip to content

Commit eb36294

Browse files
authored
[empath-split] Fix --preserve-manifest option (#25637)
`--preserve-manifest` was doing the opposite of what it meant to do: deleting the manifest file when `--preserve-manifest` was given and not deleting it when it was not given. But simply changing ```py with tempfile.NamedTemporaryFile(suffix=".manifest", mode='w+', delete=args.preserve_manifest) as f: ``` ```py with tempfile.NamedTemporaryFile(suffix=".manifest", mode='w+', delete=not args.preserve_manifest) as f: ``` doesn't work, because of Windows' peculiar behavior. 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: > - `delete` is false > - additional open shares delete access (e.g. by calling [os.open()](https://docs.python.org/3/library/os.html#os.open) with the flag `O_TEMPORARY`) > - `delete` is true but `delete_on_close` is false. Note, that in this case the additional opens that do not share delete access (e.g. created via builtin [open()](https://docs.python.org/3/library/functions.html#open)) must be closed before exiting the context manager, else the [os.unlink()](https://docs.python.org/3/library/os.html#os.unlink) call on context manager exit will fail with a [PermissionError](https://docs.python.org/3/library/exceptions.html#PermissionError). And we are trying to reopen a temporary file within the `with` context. 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 can't help but use `try`-`finally`.
1 parent d574f69 commit eb36294

File tree

2 files changed

+21
-4
lines changed

2 files changed

+21
-4
lines changed

test/test_other.py

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15183,7 +15183,8 @@ def test_empath_split(self):
1518315183
''')
1518415184

1518515185
self.run_process([EMCC, 'main.cpp', 'foo.cpp', '-gsource-map', '-g2', '-o', 'test.js'])
15186-
self.run_process([empath_split, 'test.wasm', 'path_list.txt', '-g', '-o', 'test_primary.wasm', '--out-prefix=test_'])
15186+
empath_split_cmd = [empath_split, 'test.wasm', 'path_list.txt', '-g', '-o', 'test_primary.wasm', '--out-prefix=test_', '-v']
15187+
out = self.run_process(empath_split_cmd, stdout=PIPE).stdout
1518715188

1518815189
# Check if functions are correctly assigned and split with the specified
1518915190
# paths. When one path contains another, the inner path should take its
@@ -15207,6 +15208,18 @@ def has_defined_function(file, func):
1520715208
self.assertTrue(has_defined_function('test_lib2.wasm', r'std::__2::ios_base::getloc\\28\\29\\20const'))
1520815209
self.assertTrue(has_defined_function('test_lib2.wasm', r'std::uncaught_exceptions\\28\\29'))
1520915210

15211+
# When --preserve-manifest is NOT given, the files should be deleted
15212+
match = re.search(r'wasm-split(?:\.exe)?\s+.*--manifest\s+(\S+)', out)
15213+
manifest = match.group(1)
15214+
self.assertNotExists(manifest)
15215+
15216+
# When --preserve-manifest is given, the files should be preserved
15217+
out = self.run_process(empath_split_cmd + ['--preserve-manifest'], stdout=PIPE, stderr=subprocess.DEVNULL).stdout
15218+
match = re.search(r'wasm-split(?:\.exe)?\s+.*--manifest\s+(\S+)', out)
15219+
manifest = match.group(1)
15220+
self.assertExists(manifest)
15221+
delete_file(manifest)
15222+
1521015223
# Check --print-sources option
1521115224
out = self.run_process([empath_split, 'test.wasm', '--print-sources'], stdout=PIPE).stdout
1521215225
self.assertIn('main.cpp', out)

tools/empath-split.py

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -314,8 +314,9 @@ def main():
314314
path_to_funcs = get_path_to_functions_map(args.wasm, sourcemap, all_paths)
315315

316316
# Write .manifest file
317-
with tempfile.NamedTemporaryFile(suffix=".manifest", mode='w+', delete=args.preserve_manifest) as f:
318-
manifest = f.name
317+
f = tempfile.NamedTemporaryFile(suffix=".manifest", mode='w+', delete=False)
318+
manifest = f.name
319+
try:
319320
for i, (module, paths) in enumerate(module_to_paths.items()):
320321
if i != 0: # Unless we are the first entry add a newline separator
321322
f.write('\n')
@@ -339,7 +340,7 @@ def main():
339340
f.write(f'{module}\n')
340341
for func in funcs:
341342
f.write(func + '\n')
342-
f.flush()
343+
f.close()
343344

344345
cmd = [args.wasm_split, '--multi-split', args.wasm, '--manifest', manifest]
345346
if args.verbose:
@@ -349,6 +350,9 @@ def main():
349350
if args.verbose:
350351
print('\n' + ' '.join(cmd))
351352
shared.run_process(cmd)
353+
finally:
354+
if not args.preserve_manifest:
355+
os.remove(manifest)
352356

353357

354358
if __name__ == '__main__':

0 commit comments

Comments
 (0)