[llvm-lit] Print environment variables when using env without subcommand #98414

Harini0924 · 2024-07-10T23:28:11Z

This patch addresses an issue with lit's internal shell when env is without any arguments, it fails with exit code 127 because env requires a subcommand. This patch addresses the issue by encoding the command to properly return environment variables even when no arguments are provided.

The error occurred when running the command
LIT_USE_INTERNAL_SHELL=1 ninja check-llvm.

fixes: #102383
This is part of the test cleanups proposed in the RFC: [RFC] Enabling the Lit Internal Shell by Default

github-actions · 2024-07-10T23:28:29Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

Harini0924 · 2024-07-10T23:28:51Z

CC: @ilovepi @petrhosek

llvmbot · 2024-07-10T23:28:56Z

@llvm/pr-subscribers-testing-tools

Author: None (Harini0924)

Changes

When executing not --crash env in lit's internal shell without any arguments, it fails with exit code 127 because env requires a subcommand. This patch addresses the issue by encoding the command to properly return environment variables even when no arguments are provided.

Full diff: https://github.com/llvm/llvm-project/pull/98414.diff

1 Files Affected:

(modified) llvm/utils/lit/lit/TestRunner.py (+2-1)

diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index da7fa86fd3917..8f0cd801ae9ff 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -743,7 +743,8 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
                     cmd_shenv = ShellEnvironment(shenv.cwd, shenv.env)
                 args = updateEnv(cmd_shenv, args)
                 if not args:
-                    raise InternalShellError(j, "Error: 'env' requires a" " subcommand")
+                    # Return the environment variables if no argument is provided 
+                    return {key: value for key, value in cmd_shenv.env.items()}
             elif args[0] == "not":
                 not_args.append(args.pop(0))
                 not_count += 1

ilovepi

LGTM, but let’s modify the commit message slightly. First, it’s not really important to include the context about the “not —crash” bits. Additionally, the precise error code isn’t important only that the command failed.

This patch is really about the internal shell’s support for printing the environment as output. So let’s trim it down to just those bits.

llvm/utils/lit/lit/TestRunner.py

jh7370

What's the purpose of doing not --crash env? That sounds to me like you're trying to show that the internal lit env doesn't crash?

Assuming it is needed, shouldn't there be a test for this?

github-actions · 2024-07-11T07:42:44Z

✅ With the latest revision this PR passed the Python code formatter.

ilovepi · 2024-07-11T15:36:38Z

What's the purpose of doing not --crash env? That sounds to me like you're trying to show that the internal lit env doesn't crash?

That comes from https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/not/disable-symbolization.test, so I'd guess its more about not than env. I'd hazard that test could be rewritten w/o env, but I think some other tests outside of llvm also rely on this behavior.

Assuming it is needed, shouldn't there be a test for this?

Actually yes, I agree. I confused this case w/ another one that was NFC.

@Harini0924, let's add a test for the new env behavior. Those should be updated in https://github.com/llvm/llvm-project/blob/main/llvm/utils/lit/tests/shtest-env.py. The fact that that test still seems to be passing means that we're not handling this case correctly. Likely it still fails, but does that differently.

ilovepi

I should have checked a bit more closely when reviewing this the first time. I assumed when the LLVM tests had passed, that this change was doing the right thing, but as pointed out by other reviewers, it is not.

So, please add some testing for env, and instead of returning the dictionary, we can just call a function to correctly print the environment instead.

When executing not --crash env without any arguments, it fails with exit code 127 because env reuires a subcommand. This path addresses the issue by encoding the command to properly return environment variables even when no arguments are provided.

This patch addresses an issue where calling env without any arguments incorrectly returns a "requires subcommand" message. The patch modifies the env command to properly display the current environment variables even when no arguments are provided. Additionally, the test cases have been updated: instead of returning an error, the tests now pass and check if the environment variables are output correctly.

ilovepi

Thanks for adding the tests. I think this is pretty close for me, but I'm still a bit unsure about the early return. @petrhosek brought up a technical point about that, and I think we should determine if those are still a problem before landing this as is. If you can, provide some context on what you found, that may help us make that determination faster.

llvm/utils/lit/tests/Inputs/shtest-env/env-args-none.txt

llvm/utils/lit/lit/TestRunner.py

llvm/utils/lit/tests/Inputs/shtest-env/env-args-none.txt

llvm/utils/lit/lit/TestRunner.py

llvm/utils/lit/tests/Inputs/shtest-env/env-args-none.txt

ilovepi · 2024-07-19T17:52:02Z

@jh7370 I think we have some other tests that have an EMPTY suffix, such as

llvm-project/llvm/utils/lit/tests/Inputs/shtest-env/env-calls-env.txt

Line 6 in 9ba5244

# CHECK-2-EMPTY: BAR = 2

I suppose we should update those too(separately).

ilovepi

LGTM as long as bots are happy. I'll land this for you once those pass and @jh7370 is satisfied.

jh7370

This still hasn't been run through the python code formatter (and there's still trailing whitespace on the line I highlighted before).

See also https://llvm.org/docs/CodingStandards.html#python-version-and-source-code-formatting.

jh7370

The coding standard I linked last time say only to modify the bits of code you touch in the PR, not the whole files. It looks like some of these files aren't even touched by this PR...

(Sorry for being fussy)

Harini0924 · 2024-08-07T04:00:03Z

@jh7370 I've fixed the missing space in env-args-none.txt. Is there anything else I should adjust or add? Please let me know.

Updated test cases to use `CHECK-NOT: # error`.

Harini0924 · 2024-08-07T17:26:10Z

@jh7370 @arichardson I've changed # CHECK-NOT: {{^[^#]}} to # CHECK-NOT: # error: for the tests I modified. To maintain consistency across the whole file, should update the other passing tests to use # CHECK-NOT: # error: as well?

jh7370 · 2024-08-08T08:21:04Z

@jh7370 @arichardson I've changed # CHECK-NOT: {{^[^#]}} to # CHECK-NOT: # error: for the tests I modified. To maintain consistency across the whole file, should update the other passing tests to use # CHECK-NOT: # error: as well?

Generally, I'd avoid making consistency changes in the same PR, and make them in a follow-up instead.

That being said, I'm wondering if the intent of the other lines wasn't to check specifically for an error, but rather to check for any executed command output at all? Unfortunately, I'm not really familiar with this test or its background. I'd be reluctant to change what the test is really checking for without understanding its purpose.

Harini0924 · 2024-08-09T07:42:15Z

@jh7370 To clarify the changes I made: Originally, when the tests were failing, there was a check for errors using the line # CHECK: # error: command failed with exit status: {{.*}}. However, since the tests have been modified and are now passing, I replaced that check with # CHECK-NOT: {{^[^#]}}. This change reflects the fact that the tests are no longer expecting an error, but rather verifying that no unexpected output is generated.

Given this, I think it makes sense to maintain the original format for passing tests, which is # CHECK-NOT: {{^[^#]}}, instead of # CHECK-NOT: # error:. Does this approach make sense to you, or do you have any thoughts on changing this? If this makes sense, I can revert the last commit and use # CHECK-NOT: {{^[^#]}} consistently for the tests that pass. Please let me know if I should clarify anything further.

jh7370 · 2024-08-12T07:44:11Z

@jh7370 To clarify the changes I made: Originally, when the tests were failing, there was a check for errors using the line # CHECK: # error: command failed with exit status: {{.*}}. However, since the tests have been modified and are now passing, I replaced that check with # CHECK-NOT: {{^[^#]}}. This change reflects the fact that the tests are no longer expecting an error, but rather verifying that no unexpected output is generated.

Given this, I think it makes sense to maintain the original format for passing tests, which is # CHECK-NOT: {{^[^#]}}, instead of # CHECK-NOT: # error:. Does this approach make sense to you, or do you have any thoughts on changing this? If this makes sense, I can revert the last commit and use # CHECK-NOT: {{^[^#]}} consistently for the tests that pass. Please let me know if I should clarify anything further.

Just spelling out what the regex means, because I've tripped myself up at least twice trying to interpret it. ^ means "start of line". [^#] means "any character except #. Combined they mean "match a non-empty line that does not start with #". CHECK-NOT instructs FileCheck to fail if it sees the specified between the previous thing it matched and the next thing it matches, so the combined effect is to fail the test if, between the previous and next patterns, a non-empty line without a leading # appears. In other words, it will NOT fail if it sees e.g. # error: in the output, because of the leading # (and based on the checks elsewhere in the file, I assume that is indeed the format that errors will be printed in). Could somebody confirm that I'm not making a mistake?

@Harini0924, if you didn't have any other tests to base things on, and you wanted to write this specific test completely from scratch, what would your CHECK lines look like? The way you've been replying, I feel like you don't really have a clear idea of what you are trying to achieve, which makes me nervous. On the other hand, maybe it's just a communication thing.

Unrelated, but "Resolve env subcommand required error" isn't really describing what this patch does in a useful manner. I think the key thing is that you're changing env without subcommands to print the environment variables, so I'd change this to "
[llvm-lit] Print environment variables when using env without subcommand".

RoboTux · 2024-08-12T12:11:17Z

Just spelling out what the regex means, because I've tripped myself up at least twice trying to interpret it. ^ means "start of line". [^#] means "any character except #. Combined they mean "match a non-empty line that does not start with #". CHECK-NOT instructs FileCheck to fail if it sees the specified between the previous thing it matched and the next thing it matches, so the combined effect is to fail the test if, between the previous and next patterns, a non-empty line without a leading # appears. In other words, it will NOT fail if it sees e.g. # error: in the output, because of the leading # (and based on the checks elsewhere in the file, I assume that is indeed the format that errors will be printed in). Could somebody confirm that I'm not making a mistake?

That would be my reading as well. Therefore I think "CHECK-NOT: # error: " is a better test but I think we should also split Inputs/shtest-env/* into a positive and negative tests and split shtest-env.py in two accordingly. This would allow to use RUN: %{lit} instead of RUN: not %{lit} thus guaranteeing that lit does not return an error.

Harini0924 · 2024-08-12T17:42:11Z

@jh7370 @RoboTux Thank you for the feedback earlier. To clarify, I'm implementing the tests by splitting them into two separate test files—one for positive tests and one for negative tests.

Positive Tests: In the positive test file, I’m removing not from the RUN line (e.g., # RUN: %{lit} -a -v %{inputs}/shtest-env-positive | FileCheck -match-full-lines %s) and using # CHECK-NOT: # error: to ensure no errors occur.
Negative Tests: In the negative test file, I will keep not in the RUN line (e.g., # RUN: not %{lit} -a -v %{inputs}/shtest-env-negative | FileCheck -match-full-lines %s) and will use # CHECK: # error: command failed with exit status: {{.*}}, since these tests are expected to fail and will have an error line.

Additionally, I will be splitting tests/Inputs/shtest-env into tests/Inputs/shtest-env-positive and tests/Inputs/shtest-env-negative, with each containing their respective passing and failing tests.

Does this approach align with what you were suggesting? I want to make sure I'm on the right track.

… test files Created `shtest-env-positive.py` for positive tests, removing `not` from the RUN line and using `CHECK-NOT: # error:` to ensure no errors occur. Created `shtest-env-negative.py` for negative tests, keeping `not` in the RUN line and using `CHECK: # error: command failed with exit status: {{.*}}` to verify expected failures. Split `tests/Inputs/shtest-env` into `tests/Inputs/shtest-env-positive` and `tests/Inputs/shtest-env-negative`, organizing the respective passing and failing tests into their appropriate directories.

As part of the recent restructuring, the `shtest-env` directory has been split into separate `shtest-env-positive` and `shtest-env-negative` directories to better organize the test cases. This commit removes the now obsolete files from the original `shtest-env` directory that have been migrated to the new structure.

jh7370 · 2024-08-14T10:06:02Z

I'd like @RoboTux's feedback on the latest version before I do another round of reviewing this myself.

RoboTux · 2024-08-14T10:58:13Z

@jh7370 @RoboTux Thank you for the feedback earlier. To clarify, I'm implementing the tests by splitting them into two separate test files—one for positive tests and one for negative tests.
* **Positive Tests:**  In the positive test file, I’m removing `not` from the RUN line (e.g., `# RUN: %{lit} -a -v %{inputs}/shtest-env-positive | FileCheck -match-full-lines %s`) and using `# CHECK-NOT: # error: `to ensure no errors occur.

* **Negative Tests:**  In the negative test file, I will keep `not` in the RUN line (e.g., `# RUN: not %{lit} -a -v %{inputs}/shtest-env-negative | FileCheck -match-full-lines %s`) and will use `# CHECK: # error: command failed with exit status: {{.*}}`, since these tests are expected to fail and will have an error line.
Additionally, I will be splitting tests/Inputs/shtest-env into tests/Inputs/shtest-env-positive and tests/Inputs/shtest-env-negative, with each containing their respective passing and failing tests.

Does this approach align with what you were suggesting? I want to make sure I'm on the right track.

Yes, that's exactly it. I'm not sure CHECK-NOT: #error is necessary for the positive case because I would expect lit to fail if any error is thrown but it cannot hurt.

jh7370

Couple more suggestions, but this basically looks good.

I'm not convinced the # error check not lines are needed, but I'm also not opposed to them.

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt

llvm/utils/lit/tests/Inputs/shtest-env-positive/print_environment.py

This patch removes the print_environment.py file, which was previously used to print environment variables in the format KEY=VALUE. The tests have been refactored to directly use the env command, with the output being validated using FileCheck. Comments were updated to use ## for better readability, and the test cases were rewritten to ensure consistency.

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt

Added a comment at the top of every file that I modified explaining the general purpose of the tests in the file.

jh7370

Some nits, but nearly there.

jh7370 · 2024-08-23T07:58:15Z

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt

@@ -1,3 +1,5 @@
+## Tests the env command's ability to handle setting, unsetting, and mixing environment variables.


Nearly there, but now I'm looking at the test file name. What is significant about the "none" part of the file name? I'd interpret it to mean "env with no args", but most of the test cases in this file have at least one argument.

Please don't resolve comment threads I've started, even if you think you've addressed my comment. My comment was about the file name, which to me looks incorrect. What does "args-none" mean in the file name? -u FOO or whatever is not "no arguments".

I'm sorry for the confusion. You're right; the name "args-none" doesn't accurately reflect the content of the test, especially since the file includes scenarios with arguments like -u FOO and others. A more suitable name could be something like env-var-handling.txt or env-command-tests.txt to broadly cover all the scenarios tested in the file. Do either of these work, or do you have any other suggestions?

I think probably env-no-subcommand.txt would be the name that best-conveys its meaning. It looks like the common factor in these tests is that no additional process exists that is run with the env-specified variables.

I agree env-no-subcommand.txt is a better file name. I have renamed env-args-none.txt to env-no-subcommand.txt in my recent commit.

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-calls-env.txt

llvm/utils/lit/tests/Inputs/shtest-env-positive/env.txt

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt

Harini0924 · 2024-08-26T13:28:17Z

@jh7370 Are there any other changes I need to make?

…est order

llvm/utils/lit/tests/shtest-env-positive.py

jh7370 · 2024-08-28T07:23:29Z

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-no-subcommand.txt

@@ -6,32 +6,32 @@
 # NO-ARGS: BAR=2
 # NO-ARGS: FOO=1
 # NO-ARGS: QUX=3
-
+   


Nit, you've added trailing whitespace again here and on many other lines in this file. Please fix!

…oved in my last commit.

jh7370

LGTM, assuming the pre-merge checks pass.

github-actions · 2024-08-29T17:01:15Z

@Harini0924 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added llvm-lit testing-tools labels Jul 10, 2024

ilovepi approved these changes Jul 10, 2024

View reviewed changes

petrhosek reviewed Jul 11, 2024

View reviewed changes

llvm/utils/lit/lit/TestRunner.py Outdated Show resolved Hide resolved

jh7370 reviewed Jul 11, 2024

View reviewed changes

ilovepi requested changes Jul 11, 2024

View reviewed changes

Harini0924 marked this pull request as draft July 17, 2024 23:00

[llvm-lit]

bd4f897

When executing not --crash env without any arguments, it fails with exit code 127 because env reuires a subcommand. This path addresses the issue by encoding the command to properly return environment variables even when no arguments are provided.

Harini0924 force-pushed the llvm_env branch 2 times, most recently from 920d5b1 to bd4f897 Compare July 18, 2024 06:50

Harini0924 added 6 commits July 18, 2024 07:13

hi

94d958a

Harini0924 force-pushed the llvm_env branch from 75a0eda to 4990d39 Compare July 18, 2024 16:49

ilovepi reviewed Jul 18, 2024

View reviewed changes

llvm/utils/lit/tests/Inputs/shtest-env/env-args-none.txt Outdated Show resolved Hide resolved

llvm/utils/lit/lit/TestRunner.py Show resolved Hide resolved

jh7370 reviewed Jul 19, 2024

View reviewed changes

llvm/utils/lit/tests/Inputs/shtest-env/env-args-none.txt Outdated Show resolved Hide resolved

llvm/utils/lit/lit/TestRunner.py Outdated Show resolved Hide resolved

llvm/utils/lit/tests/Inputs/shtest-env/env-args-none.txt Outdated Show resolved Hide resolved

Harini0924 marked this pull request as ready for review July 19, 2024 21:54

ilovepi approved these changes Jul 19, 2024

View reviewed changes

petrhosek approved these changes Jul 20, 2024

View reviewed changes

jh7370 requested changes Jul 22, 2024

View reviewed changes

jh7370 reviewed Jul 22, 2024

View reviewed changes

[llvm-lit] Modified CHECK-NOT: {{^[^#]}}

957c7a6

Updated test cases to use `CHECK-NOT: # error`.

Harini0924 changed the title ~~[llvm-lit] Resolve env subcommand required error~~ [llvm-lit] Print environment variables when using env without subcommand". Aug 12, 2024

Harini0924 changed the title ~~[llvm-lit] Print environment variables when using env without subcommand".~~ [llvm-lit] Print environment variables when using env without subcommand Aug 12, 2024

Harini0924 added 2 commits August 12, 2024 19:21

jh7370 reviewed Aug 15, 2024

View reviewed changes

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt Outdated Show resolved Hide resolved

llvm/utils/lit/tests/Inputs/shtest-env-positive/print_environment.py Outdated Show resolved Hide resolved

jh7370 reviewed Aug 22, 2024

View reviewed changes

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt Outdated Show resolved Hide resolved

[llvm-lit] Improved comments

053e7eb

Added a comment at the top of every file that I modified explaining the general purpose of the tests in the file.

jh7370 reviewed Aug 23, 2024

View reviewed changes

Updated the comments in files modified

61f2bf9

ilovepi reviewed Aug 23, 2024

View reviewed changes

llvm/utils/lit/tests/Inputs/shtest-env-positive/env-args-none.txt Outdated Show resolved Hide resolved

Fixed trailing space.

69809a6

Renamed env-args-none.txt to env-no-subcommand.txt and alphabetized t…

070ccde

…est order

jh7370 reviewed Aug 28, 2024

View reviewed changes

Fixed trailing whitespaces and # CHECK: -- which I accidentally rem…

7e2be8b

…oved in my last commit.

jh7370 approved these changes Aug 28, 2024

View reviewed changes

Merge branch 'main' into llvm_env

4406d4b

Harini0924 merged commit 1783924 into llvm:main Aug 29, 2024
8 checks passed

		@@ -1,3 +1,5 @@
		## Tests the env command's ability to handle setting, unsetting, and mixing environment variables.

[llvm-lit] Print environment variables when using env without subcommand #98414

[llvm-lit] Print environment variables when using env without subcommand #98414

Uh oh!

Conversation

Harini0924 commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 10, 2024

Uh oh!

Harini0924 commented Jul 10, 2024

Uh oh!

llvmbot commented Jul 10, 2024

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovepi commented Jul 11, 2024

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilovepi commented Jul 19, 2024

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Harini0924 commented Aug 7, 2024

Uh oh!

Harini0924 commented Aug 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jh7370 commented Aug 8, 2024

Uh oh!

Harini0924 commented Aug 9, 2024

Uh oh!

jh7370 commented Aug 12, 2024

Uh oh!

RoboTux commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Harini0924 commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jh7370 commented Aug 14, 2024

Uh oh!

RoboTux commented Aug 14, 2024

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

jh7370 Aug 23, 2024

Choose a reason for hiding this comment

Harini0924 commented Jul 10, 2024 •

edited

Loading

github-actions bot commented Jul 11, 2024 •

edited

Loading

jh7370 left a comment •

edited

Loading

Harini0924 commented Aug 7, 2024 •

edited

Loading

RoboTux commented Aug 12, 2024 •

edited

Loading

Harini0924 commented Aug 12, 2024 •

edited

Loading