-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[update_cc_test_checks] Use lit's shell to run commands #65333
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.
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
base: main
Are you sure you want to change the base?
Conversation
While update_test_checks and update_llc_test_checks handle these correctly, it does not currently work in update_cc_test_checks.
I was trying to update a test that redirected stderr to a file and hit errors because the 2>%t.out was being passed as a positional argument to clang. Instead of re-implementing basic shell parsing in this script, we can reuse lit's internal shell to run these commands and handle redirection and pipelines. A more minimal fix could have been to use the shell=True parameter in python's subprocess modules, but this would not work e.g. on Windows. Additionally, I am planning to simplify the common.py infrastructure by reusing the shell helpers from lit and this is the first step in that direction.
Looks like only squash and merge is possible here, so I'll commit the baseline test separately before landing this. |
ret = collections.defaultdict(list) | ||
# Use clang's JSON AST dump to get the mangled name | ||
json_dump_args = [args.clang] + clang_args + ["-fsyntax-only", "-o", "-"] | ||
json_dump_args = clang_cmd.args + ["-fsyntax-only", "-o", "/dev/null"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the change from stdout to /dev/null output. How can you get an output from that command when doing execute_pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is not required, it seems JSON is always printed to stdout. This is just to ensure to other output is produced (although probably -fsyntax-only) is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
cmd.redirects[i] = redirect[0], common.applySubstitutions( | ||
redirect[1], subs | ||
) | ||
|
||
# Parse executable args. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be removed.
] | ||
clang_args += ti.args.clang_args | ||
clang_cmd.args[0:1] = SUBST[clang_cmd.args[0]] | ||
print(clang_cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a forgotten debug print statement?
# Remove all -verify arguments since they could cause the IR generation to fail | ||
clang_cmd.args = [x for x in clang_cmd.args if not x.startswith("-verify")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't you want the tool to fail then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch that is a local diff that should not have been added to this change.
My use-case for that change was updating tests after a merge as a two-stage process, first fix the CHECK lines then the -verify ones, but you're right this can be dropped.
I was trying to update a test that redirected stderr to a file and hit
errors because the
2>%t.out
was being passed as a positional argument toclang. Instead of re-implementing basic shell parsing in this script, we
can reuse lit's internal shell to run these commands and handle
redirection and pipelines.
A more minimal fix could have been to use the shell=True parameter in
python's subprocess modules, but this would not work e.g. on Windows.
Additionally, I am planning to simplify the common.py infrastructure by
reusing the shell helpers from lit and this is the first step in that
direction.