Skip to content

Fix paths with spaces in lit check updater - #9375

Closed
Rodri5449 wants to merge 1 commit into
WebAssembly:mainfrom
Rodri5449:fix/lit-checks-paths-with-spaces
Closed

Rodri5449 wants to merge 1 commit into
WebAssembly:mainfrom
Rodri5449:fix/lit-checks-paths-with-spaces

Conversation

@Rodri5449

Copy link
Copy Markdown

No description provided.

@Rodri5449
Rodri5449 requested a review from a team as a code owner November 9, 2026 16:15
@Rodri5449 Rodri5449 closed this Nov 9, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates scripts/update_lit_checks.py to properly handle and escape paths containing spaces when running commands, and adds a corresponding unit test suite in test/unit/test_update_lit_checks_paths.py. The review feedback points out a potential bug in the path replacement logic where sequential .replace() calls could lead to recursive replacements if the paths themselves contain placeholder substrings, and suggests a single-pass regular expression replacement as a safer alternative.

Comment on lines +114 to +115
def replace_paths(text):
return text.replace('%s', test).replace('%S', os.path.dirname(test)).replace('%t', tmp)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using sequential .replace() calls can lead to unexpected behavior if one of the replacement values (such as test or tmp paths) contains another placeholder substring (e.g., if the test path contains %t or %S). In such cases, the subsequent .replace() calls will perform recursive replacements on the already-replaced path, corrupting the final command.

Using a single-pass regular expression replacement ensures that each placeholder is replaced exactly once and prevents recursive replacement bugs.

Suggested change
def replace_paths(text):
return text.replace('%s', test).replace('%S', os.path.dirname(test)).replace('%t', tmp)
def replace_paths(text):
replacements = {
'%s': test,
'%S': os.path.dirname(test),
'%t': tmp
}
return re.sub(r'%[sSt]', lambda m: replacements[m.group()], text)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant