-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
[lit] Fix some issues from --per-test-coverage #65242
Conversation
What is
We probably should, purely on the basis that people who develop on Windows might be using this setup. That being said, I'm fairly confident that Windows users by default use the internal shell. I have not verified that though. |
FWIW, I'm not a regular Windows user, I only run it in VMs and similar for testing things, but whenever I do and want to test things in a native Windows environment (as opposed to running in msys2 bash or similar), I do it with cmd. I've never really gotten familiar with powershell. And whenever tools like python etc, programmatically execute some command string in a way that it is parsed by a shell, that shell is cmd, not powershell afaik. |
I haven't been so much involved in the recent Lit changes, and it's been a while since I poked at these things, so it took me a little bit of fiddling around to get back on track about what the actual status quo is here. I'll try to summarize my findings, please doublecheck if I've missed something: Status quo
As additional context: The libcxx testsuite used to use the external shell a few years ago, but this was problematic for running the tests on Windows, it didn't really work right there (I don't have exact details). This was changed in 39bbfb7 - before that, running the libcxx tests required making sure that I.e., unless jumping through hoops, none of these testsuites really run things with the win32 cmd.exe. BreakageRunning things with win32 cmd.exe as external shell did use to work, but currently doesn't. This actually seems to have broken in 1041a96 already. If running a simple llvm shell test with
After that commit, it instead breaks like this:
It further breaks at 64d1954 as you've noticed, since the RUN lines essentially just become no-op goto labels.
(It prints this even if injecting an error in the test.) Furthermore, 09b6e45 changes this case further, making it just print this:
(Even when executing with Separate side note; running the LLVM testsuite with bash as external executor (if it happens to be available in path) also seems to fail:
(This seems to have been broken throughout this time period at least. However before 39bbfb7, when libcxx tests were using the external executor, they did work when executed with bash as external executor.) ConclusionIt certainly looks like this configuration is severely broken. I'm not aware of any parts of the configurations that use it by default, and even then, it's generally problematic, so I'm not sure if it's worth spending effort on fixing. (The suggestion that was made somewhere around these discussions, to move most things towards using the internal shell even on unix, sounds to me like a good direction in general .) That said; for echoing executed commands, when using the internal executor, it's probably very valuable if they're printed in a form that is easy to copypaste and execute in cmd.exe (where possible - at least for the majority of simple cases). |
FWIW, this PR doesn't entirely fix running the tests with win32 cmd.exe either; with this PR, it fails like this:
|
It's something like an internal substitution. When parsing a RUN line, lit inserts it before the RUN line's commands like this:
Later, lit expands it based on the shell (internal, external sh-like shell, external windows
|
Thanks for the nice summary. It looks right to me.
You're right, and that landed in April, 2022, so windows
It looks like the
Agreed. What if we raise a python exception on the windows
If RUN lines are written for lit's internal shell, I'm not sure if they are generally guaranteed to work in windows |
Yep, probably - I guess this goes the same for all the llvm tools that have substitutions set up.
I guess that sounds reasaonble. Although I'm not sure how often odd downstreams integrate from upstream, but things have been quite broken for quite some time anyway, so I guess it's pretty safe to say that this is dead code.
Not really, no - I'm mostly commenting on it from the point of view that I remember seeing somewhere else in these discussions (regarding formatting of the |
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.
LGTM
Ah, thanks for the explanation. Then it's used a lot more than I thought. |
Before <https://reviews.llvm.org/D154984> and <https://reviews.llvm.org/D156954>, lit reported full RUN lines in a `Script:` section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows `cmd`), they aren't reported at all. A fix was requested at the following: * <https://reviews.llvm.org/D154984#4627605> * <https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839/35?u=jdenny-ornl> This patch does not correctly address the case when the external shell is windows `cmd`. As discussed at <llvm#65242>, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.
Thanks for pointing that out. I noticed it too but made the bad assumption that something was wrong with the pre-commit check itself, and I ran out of time to investigate. This is the diagnostic I saw after a traceback:
I didn't see In contrast, when I run black on the command line on my local system, I see:
Is there any way to get the pre-commit check to mention the name of the problematic file? (By the way, I have noticed that many python files were formatted with >80 columns, probably the black default of 88. That doesn't fit in the 80-column editor/terminal windows I use for most of LLVM. Is there any good reason not to follow the LLVM coding standard for source code width in python files? I see no exception there for python, or did I misunderstand something? Black just needs a
Lit's tests use the |
I'll look into this, it also took me time to figure it out.
This was discussed when we wrote the coding style for python and then we thought it was just better to use black's default to avoid having people need to run black with arguments (since it won't read a config file). |
OK, but the LLVM coding standard appears to contradict that approach, as far as I can tell:
The LLVM coding standard's advice on using black is:
Combining all the above, I'm led to believe I should be passing options to black, but it's unclear which ones. I'm sorry I didn't participate in the original discussion. If it's too late to change things, can we clarify the LLVM coding standard?
For example, in
And it worked for me. |
I think that's pretty new then. We tried this back during the original discussion without luck. But feel free to either update the docs to reflect the reality or post to discourse suggesting a policy change. I have no strong feelings either way as long as we don't have to remember to manually change options. A change here would also mean a lot of reformatting which is a bit unnecessary churn. |
I'll plan to raise it in the original RFC before proposing any changes. Thanks for all your work in this area. It is much appreciated. |
21b4d57
to
e756b3a
Compare
Before <https://reviews.llvm.org/D154984> and <https://reviews.llvm.org/D156954>, lit reported full RUN lines in a `Script:` section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows `cmd`), they aren't reported at all. A fix was requested at the following: * <https://reviews.llvm.org/D154984#4627605> * <https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839/35?u=jdenny-ornl> This patch does not correctly address the case when the external shell is windows `cmd`. As discussed at <llvm#65242>, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.
PR has changed since the review.
I'm not trying to rush reviewers, but I want it to be clear that this patch is still in need of review due to recent changes. See #65242 (comment) for a summary. I'm afraid the recent discussion of black might have buried that comment. Maybe I'm just not used to LLVM PR reviews yet. |
I saw it but was waiting on the updated version fixing the python issue before giving it another look. I shall look at it today. |
keyword=keyword, line_number=line_number | ||
) | ||
assert re.match( | ||
kPdbgRegex + "$", pdbg |
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.
Why is the $ dropped in the buildPdbgCommand?
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 question.
Originally, my goal was for the assert to mimic how kPdbgRegex will actually be used later when expanding %dbg
substitutions. That's the usage that the assert is trying to verify will work correctly. Thus, this patch changes the assert in two ways: the searched string contains all of %dbg(...) cmd-line
instead of just %dbg(...)
, and it does not use $
.
However, now that you ask, I think the assert should be stricter. For example, D154987 proposes to extend kPdbgRegex to permit newlines, which might appear in lit.run(cmd)
in PYTHON directives, as in the example in that review's summary. Using $
in the assert here would have caught that existing deficiency of kPdbgRegex: its .*
doesn't match beyond the first newline.
Instead of $
, what do you think of using re.fullmatch
in the assert? That is, buildPdbgCommand expects kPdbgRegex to match the entire string it's building.
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 went ahead and applied that change and wrote a commit log to explain it.
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.
LGTM but does it mean you'll just rebase and push to the repo using pure git? If using the github tool AFAIK you can only merge and squash on the LLVM project so the commit message won't show up unless you modify the PR message. I might be wrong though, I haven't interacted much with github.
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.
At least from the web UI you have the chance to edit the final commit message. So you can combine them there if you're ok with it all being one commit.
(otherwise yes manually push some of it)
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.
Thanks for the review!
Yes, I performed most steps from the git command line: squash commits and commit logs together, rebase onto the latest main, and force push to the PR. I then clicked "Squash and merge" and copied and pasted my new commit log there. The commit log it initially offered was the original comment I posted for this PR, but the focus of the PR has shifted since then. It would be nicer if it offered the current commit logs, squashed in the same way a git merge --squash
would squash them.
I don't know how to accomplish the rebase onto main via the github web UI... even though it seems like that's almost always going to be required given how frequently new commits show up on main. That means I needed to work from the command line anyway, so it seemed more practical to handle squashing there too.
Please let me know if I've overlooked best practices for using the github web UI.
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.
From https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/about-merge-methods-on-github#squashing-your-merge-commits it seems squash and merge is actually a squash and rebase (it makes it clear it's a fast forward).
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.
The web UI squash-and-merge will automatically rebase to current HEAD.
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.
Thanks for the clarifications. For some reason, I thought it previously rejected one when I wasn't caught up to main, but perhaps I had conflicts then. I'll try again next time.
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.
Yep, it worked fine in another PR that had no conflicts.
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.
LGTM
D154280 (landed in 64d1954 in July, 2023) implements `--per-test-coverage` (which can also be specified via `lit_config.per_test_coverage`). However, it has a few issues, which the current patch addresses: 1. D154280 implements `--per-test-coverage` only for the case that lit is configured to use an external shell. The current patch extends the implementation to lit's internal shell. 2. In the case that lit is configured to use an external shell, regardless of whether `--per-test-coverage` is actually specified, D154280 causes `%dbg(RUN: at line N)` to be expanded in RUN lines early and in a manner that is specific to sh-like shells. As a result, later code in lit that expands it in a shell-specific manner is useless as there's nothing left to expand. The current patch cleans up the implementation to avoid useless code. 3. Because of issue 2, D154280 corrupts support for windows `cmd` as an external shell (effectively comments out all RUN lines with `:`). The current patch happens to fix that particular corruption by addressing issue 2. However, D122569 (landed in 1041a96 in April, 2022) had already broken support for windows `cmd` as an external shell (discards RUN lines when expanding `%dbg(RUN: at line N)`). The current patch does not attempt to fix that bug. For further details, see the PR discussion of the current patch. The current patch addresses the above issues by implementing `--per-test-coverage` before selecting the shell (internal or external) and by leaving `%dbg(RUN: at line N)` unexpanded there. Thus, it is expanded later in a shell-specific manner, as before D154280. This patch introduces `buildPdbgCommand` into lit's implementation to encapsulate the process of building (or rebuilding in the case of the `--per-test-coverage` implementation) a full `%dbg(RUN: at line N) cmd` line and asserting that the result matches `kPdbgRegex`. It also cleans up that and all other uses of `kPdbgRegex` to operate on the full line with `re.fullmatch` not `re.match`. This change better reflects the intention in every case, but it is expected to be NFC because `kPdbgRegex` ends in `.*` and thus avoids the difference between `re.fullmatch` and `re.match`. The only caveat is that `.*` does not match newlines, but RUN lines cannot contain newlines currently, so this caveat currently shouldn't matter in practice. The original `--per-test-coverage` implementation avoided accumulating `export LLVM_PROFILE_FILE={profile}` insertions across retries (due to `ALLOW_RETRIES`) by skipping the insertion if `%dbg(RUN: at line N)` was not present and thus had already been expanded. However, the current patch makes sure the insertions also happen for commands without `%dbg(RUN: at line N)`, such as preamble commands or some commands from other lit test formats. Thus, the current patch implements a different mechanism to avoid accumulating those insertions (see code comments).
e2c0d8c
to
c0fa7bc
Compare
Thank you @jdenny-ornl for fixing the issues. |
D154280 (landed in 64d1954 in July, 2023) implements `--per-test-coverage` (which can also be specified via `lit_config.per_test_coverage`). However, it has a few issues, which the current patch addresses: 1. D154280 implements `--per-test-coverage` only for the case that lit is configured to use an external shell. The current patch extends the implementation to lit's internal shell. 2. In the case that lit is configured to use an external shell, regardless of whether `--per-test-coverage` is actually specified, D154280 causes `%dbg(RUN: at line N)` to be expanded in RUN lines early and in a manner that is specific to sh-like shells. As a result, later code in lit that expands it in a shell-specific manner is useless as there's nothing left to expand. The current patch cleans up the implementation to avoid useless code. 3. Because of issue 2, D154280 corrupts support for windows `cmd` as an external shell (effectively comments out all RUN lines with `:`). The current patch happens to fix that particular corruption by addressing issue 2. However, D122569 (landed in 1041a96 in April, 2022) had already broken support for windows `cmd` as an external shell (discards RUN lines when expanding `%dbg(RUN: at line N)`). The current patch does not attempt to fix that bug. For further details, see the PR discussion of the current patch. The current patch addresses the above issues by implementing `--per-test-coverage` before selecting the shell (internal or external) and by leaving `%dbg(RUN: at line N)` unexpanded there. Thus, it is expanded later in a shell-specific manner, as before D154280. This patch introduces `buildPdbgCommand` into lit's implementation to encapsulate the process of building (or rebuilding in the case of the `--per-test-coverage` implementation) a full `%dbg(RUN: at line N) cmd` line and asserting that the result matches `kPdbgRegex`. It also cleans up that and all other uses of `kPdbgRegex` to operate on the full line with `re.fullmatch` not `re.match`. This change better reflects the intention in every case, but it is expected to be NFC because `kPdbgRegex` ends in `.*` and thus avoids the difference between `re.fullmatch` and `re.match`. The only caveat is that `.*` does not match newlines, but RUN lines cannot contain newlines currently, so this caveat currently shouldn't matter in practice. The original `--per-test-coverage` implementation avoided accumulating `export LLVM_PROFILE_FILE={profile}` insertions across retries (due to `ALLOW_RETRIES`) by skipping the insertion if `%dbg(RUN: at line N)` was not present and thus had already been expanded. However, the current patch makes sure the insertions also happen for commands without `%dbg(RUN: at line N)`, such as preamble commands or some commands from other lit test formats. Thus, the current patch implements a different mechanism to avoid accumulating those insertions (see code comments).
By the way, thanks for the feature! |
D154280 (landed in 64d1954 in July, 2023) implements `--per-test-coverage` (which can also be specified via `lit_config.per_test_coverage`). However, it has a few issues, which the current patch addresses: 1. D154280 implements `--per-test-coverage` only for the case that lit is configured to use an external shell. The current patch extends the implementation to lit's internal shell. 2. In the case that lit is configured to use an external shell, regardless of whether `--per-test-coverage` is actually specified, D154280 causes `%dbg(RUN: at line N)` to be expanded in RUN lines early and in a manner that is specific to sh-like shells. As a result, later code in lit that expands it in a shell-specific manner is useless as there's nothing left to expand. The current patch cleans up the implementation to avoid useless code. 3. Because of issue 2, D154280 corrupts support for windows `cmd` as an external shell (effectively comments out all RUN lines with `:`). The current patch happens to fix that particular corruption by addressing issue 2. However, D122569 (landed in 1041a96 in April, 2022) had already broken support for windows `cmd` as an external shell (discards RUN lines when expanding `%dbg(RUN: at line N)`). The current patch does not attempt to fix that bug. For further details, see the PR discussion of the current patch. The current patch addresses the above issues by implementing `--per-test-coverage` before selecting the shell (internal or external) and by leaving `%dbg(RUN: at line N)` unexpanded there. Thus, it is expanded later in a shell-specific manner, as before D154280. This patch introduces `buildPdbgCommand` into lit's implementation to encapsulate the process of building (or rebuilding in the case of the `--per-test-coverage` implementation) a full `%dbg(RUN: at line N) cmd` line and asserting that the result matches `kPdbgRegex`. It also cleans up that and all other uses of `kPdbgRegex` to operate on the full line with `re.fullmatch` not `re.match`. This change better reflects the intention in every case, but it is expected to be NFC because `kPdbgRegex` ends in `.*` and thus avoids the difference between `re.fullmatch` and `re.match`. The only caveat is that `.*` does not match newlines, but RUN lines cannot contain newlines currently, so this caveat currently shouldn't matter in practice. The original `--per-test-coverage` implementation avoided accumulating `export LLVM_PROFILE_FILE={profile}` insertions across retries (due to `ALLOW_RETRIES`) by skipping the insertion if `%dbg(RUN: at line N)` was not present and thus had already been expanded. However, the current patch makes sure the insertions also happen for commands without `%dbg(RUN: at line N)`, such as preamble commands or some commands from other lit test formats. Thus, the current patch implements a different mechanism to avoid accumulating those insertions (see code comments).
Before <https://reviews.llvm.org/D154984> and <https://reviews.llvm.org/D156954>, lit reported full RUN lines in a `Script:` section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows `cmd`), they aren't reported at all. A fix was requested at the following: * <https://reviews.llvm.org/D154984#4627605> * <https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839/35?u=jdenny-ornl> This patch does not address the case when the external shell is windows `cmd`. As discussed at <llvm#65242>, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.
Before <https://reviews.llvm.org/D154984> and <https://reviews.llvm.org/D156954>, lit reported full RUN lines in a `Script:` section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows `cmd`), they aren't reported at all. A fix was requested at the following: * <https://reviews.llvm.org/D154984#4627605> * <https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839/35?u=jdenny-ornl> This patch does not address the case when the external shell is windows `cmd`. As discussed at <#65242>, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.
In PR llvm#65242 (landed as 9e739fd), I claimed that RUN lines cannot contain newlines. Actually, they can after substitution expansion. More generally, a lit config file can define substitutions or preamble commands containing newlines. While both of those cases seem unlikely in practice, [D154987](https://reviews.llvm.org/D154987) proposes PYTHON directives where it seems very likely. Regardless of the use case, without this patch, such newlines break expansion of `%dbg(RUN: at line N)`, and the fix is simple.
In PR #65242 (landed as 9e739fd), I claimed that RUN lines cannot contain newlines. Actually, they can after substitution expansion. More generally, a lit config file can define substitutions or preamble commands containing newlines. While both of those cases seem unlikely in practice, [D154987](https://reviews.llvm.org/D154987) proposes PYTHON directives where it seems very likely. Regardless of the use case, without this patch, such newlines break expansion of `%dbg(RUN: at line N)`, and the fix is simple.
Before <https://reviews.llvm.org/D154984> and <https://reviews.llvm.org/D156954>, lit reported full RUN lines in a `Script:` section. Now, in the case of lit's internal shell, it's the execution trace that includes them. However, if lit is configured to use an external shell (e.g., bash, windows `cmd`), they aren't reported at all. A fix was requested at the following: * <https://reviews.llvm.org/D154984#4627605> * <https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839/35?u=jdenny-ornl> This patch does not correctly address the case when the external shell is windows `cmd`. As discussed at <llvm/llvm-project#65242>, it's not clear whether that's a use case that people still care about, and it seems to be generally broken anyway.
Key issue
When lit is configured to use windows
cmd
as an external shell, it appears that all RUN lines are effectively commented out. The problematic change landed in July 26, 2023, so it's surprising it hasn't come up yet. I'm afraid I don't have a windows setup, so I could use some help to verify what's happening.Details
In the case that lit is configured to use an external shell, D154280 (landed in 64d1954 on July 26, 2023) causes
%dbg(RUN: at line N)
to be expanded in RUN lines early and in a manner that is specific to sh-like shells. As a result, later code in lit that expands it in a shell-specific manner is useless.That sh-like expansion uses the
:
command as follows:In sh-like shells, the
:
command simply discards its arguments. However, in windowscmd
,:
indicates a goto label. That appears to effectively comment out the rest of the line, including the original commands from the RUN line.I am not aware of any complaints about this change. Did I miss them? Are all tests still passing and so no one noticed? Lit's own test suite has some tests that normally fail if RUN lines don't execute. Is no one running lit's test suite with windows
cmd
as a lit external shell? Or is no one using windowscmd
as a lit external shell at all anymore?Another issue
D154280 doesn't implement
--per-test-coverage
for lit's internal shell.Fix
This patch fixes the above problems by implementing
--per-test-coverage
before selecting the shell (internal or external) and by leaving%dbg(RUN: at line N)
unexpanded. Thus, it is expanded later in a shell-specific manner, as before D154280.I would like to understand whether windows
cmd
as a lit external shell is worthwhile to support anymore.