[all-commits] [llvm/llvm-project] 9764cf: [llvm-lit] Add precommit test to verify current be...

[Harini0924 via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 9764cf888502fe6dd15ab21de5c2f73cae47a2c0
      https://github.com/llvm/llvm-project/commit/9764cf888502fe6dd15ab21de5c2f73cae47a2c0
  Author: Harini0924 <harinidonthula at google.com>
  Date:   2024-08-30 (Fri, 30 Aug 2024)

  Changed paths:
    A llvm/utils/lit/tests/Inputs/shtest-glob/example_file1.input
    A llvm/utils/lit/tests/Inputs/shtest-glob/example_file2.input
    A llvm/utils/lit/tests/Inputs/shtest-glob/glob-echo.txt
    A llvm/utils/lit/tests/Inputs/shtest-glob/glob-mkdir.txt
    A llvm/utils/lit/tests/Inputs/shtest-glob/lit.cfg
    A llvm/utils/lit/tests/shtest-glob.py

  Log Message:
  -----------
  [llvm-lit] Add precommit test to verify current behavior of glob expansion in lit's internal shell (#106325)

This patch introduces a precommit test to verify the current behavior of
glob expansion in lit's internal shell. The motivation for this test
stems from an issue encountered during the BOLT test suite when running
with the lit internal shell using the command:

`LIT_USE_INTERNAL_SHELL=1 ninja check-bolt`

During execution, the following error was observed:
```
File "/usr/local/google/home/harinidonthula/llvm-project/llvm/utils/lit/lit/TestRunner.py", line 416, in executeBuiltinEcho
    stdout.write(encode(maybeUnescape(args[-1])))
TypeError: string argument expected, got 'GlobItem'
```
The `executeBuiltinEcho` function in the lit testing framework expects a
string to be passed to `stdout.write`, but it received a `GlobItem`
object instead. This precommit test is designed to check the current
behavior where the glob pattern isn't correctly expanded, leading to
this `TypeError`.

While this patch doesn't fix the issue, it helps in understanding and
verifying the current behavior. The feedback I received from this
[PR](https://github.com/llvm/llvm-project/pull/105925) suggests using
`cmd.args = expand_glob_expressions(cmd.args, shenv.cwd)` to match the
behavior of `executeBuiltinMkdir` and `executeBuiltinRm`, but it is
recognized that the internal shell should ideally expand globs before
calling any built-in command.

**Request for Feedback:**
I'm looking for feedback on how to improve this precommit test,
specifically regarding the handling and expansion of glob patterns for
commands like mkdir and rm within the internal shell. Currently, the
args are expanded at the beginning of these functions, which should
ensure proper glob expansion. However, I'd appreciate guidance on
whether I should write additional tests to verify that mkdir and rm are
handling glob expansions correctly.

If such tests are recommended, I would also appreciate advice on the
best approach to implement them, considering the existing framework and
the way glob expansion is expected to function in the internal shell.
Should these tests confirm that the current implementation passes, or
are there specific edge cases I should be aware of?

**Next Steps:**

In my follow-up PR, I plan to address the UNRESOLVED error by expanding
the entire command, ensuring correct and consistent behavior across all
commands. The current test checks for an unresolved issue with the glob
expansion, specifically looking for a `TypeError` due to an unexpanded
`GlobItem`. This will be updated to reflect the correct behavior once
the issue is resolved.

This change is relevant for [[RFC] Enabling the Lit Internal Shell by
Default](https://discourse.llvm.org/t/rfc-enabling-the-lit-internal-shell-by-default/80179/3)



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications