workflows/release-tasks: Setup FileCheck and not for release-lit #66799
Conversation
|
@llvm/pr-subscribers-github-workflow Changeslit tests require commands FileCheck and not. They must be available in the PATH. Reuse the tools provided by Ubuntu and create local symlinks in order to deal with the versioning suffix. This also guarantees that python3-psutil is installed in order to enable more tests. Fixes #64892. Full diff: https://github.com/llvm/llvm-project/pull/66799.diff 1 Files Affected:
diff --git a/.github/workflows/release-tasks.yml b/.github/workflows/release-tasks.yml
index 013714005d1124e..b3da8c5b47da0ef 100644
--- a/.github/workflows/release-tasks.yml
+++ b/.github/workflows/release-tasks.yml
@@ -84,12 +84,14 @@ jobs:
uses: actions/checkout@v4
- name: Install dependencies
- run: sudo apt-get install -y python3-setuptools
+ run: sudo apt-get install -y python3-setuptools python3-psutil llvm-15-tools
- name: Test lit
run: |
cd llvm/utils/lit
- python3 lit.py tests
+ ln -s /usr/bin/FileCheck-15 ./FileCheck
+ ln -s /usr/bin/not-15 ./not
+ PATH=$(pwd):$PATH python3 lit.py tests
- name: Package lit
run: |
|
|
Thanks for the fix! I had that on my todo list. But I am worried that we use a fixed version from apt when I bet that lit tests will only be guaranteed to work with the file check from the same source. I think to be safe we need to build it. |
|
@jdenny-ornl @tstellar What do you think about this? How closely tied is lit with the current in-tree version of FileCheck? I would like to fix this before the next release so that we can run the lit tests in the CI. |
FileCheck doesn't grow new features frequently. However, when it does, tests can use those features immediately. I think it would be worthwhile to build the latest FileCheck. @RoboTux @jh7370 might wish to comment as well. |
I don't know enough about the context here to be able to provide clear feedback, but generally, I don't think it makes sense to use any LLVM tools other than the contemporary ones (i.e. the versions built from the same source as providing the tests) when running tests. Other tools that are considered part of "test supporting tools" include |
|
Likewise, I'm not sure what is the story behind this PR but since they are in the same tree I feel it should be fine to use the latest feature. It also sounds easier to just depend on the in-tree version with appropriate dependencies in CMake rather than detecting if it's available on the system, error out if not, and deal with bug report due to people using a too old version of lit (or test the version beforehand). |
|
I updated this PR in order to build both |
.github/workflows/release-tasks.yml
Outdated
| run: sudo apt-get install -y python3-setuptools | ||
| run: | | ||
| sudo apt-get update | ||
| sudo apt-get install -y python3-setuptools python3-psutil clang cmake |
There was a problem hiding this comment.
I think we should maybe use the setup-cpp action that installs clang from our apt.llvm.org instead and we can pin the version, so it doesn't change under us randomly and then we get Ninja as well.
There was a problem hiding this comment.
- name: Setup Cpp
uses: aminya/setup-cpp@v1
with:
compiler: llvm-16.0.6
cmake: true
ninja: true
There was a problem hiding this comment.
I've just pushed a fix for this.
.github/workflows/release-tasks.yml
Outdated
| - name: Build lit requirements | ||
| run: | | ||
| mkdir build && cd build | ||
| cmake ../llvm -DCMAKE_BUILD_TYPE=Release |
There was a problem hiding this comment.
Using ninja is going to be faster.
.github/workflows/release-tasks.yml
Outdated
|
|
||
| - name: Test lit | ||
| run: | | ||
| cd llvm/utils/lit | ||
| python3 lit.py tests | ||
| PATH=$(pwd)/../../../build/bin:$PATH python3 lit.py tests |
There was a problem hiding this comment.
Why not use the check-lit CMake target? It should depend on all the right tools being built and avoid the all Build lit requirements step.
| python3 lit.py tests | ||
| mkdir build && cd build | ||
| cmake ../llvm -DCMAKE_BUILD_TYPE=Release -G Ninja | ||
| ninja -v -j $(nproc) check-lit |
There was a problem hiding this comment.
To make debugging any failed tests easier, would you please consider adding the following to the environment when running check-lit?
export FILECHECK_OPTS='-dump-input-filter=all -vv -color'
There was a problem hiding this comment.
On second thought, I realize I don't know that I understand the context. If this is run manually and can easily be re-run with different environment variables, then the person running it should decide whether to make this change. Otherwise, defaulting to verbosity would be helpful for whoever has to understand test failures.
There was a problem hiding this comment.
It doesn't hurt to have extra verbosity.
Running this is not complex, but does require a specific setup.
I added included this suggestion in my last commit.
|
Thanks for your work on this! I think we should merge this, but probably also spin it out to its own action to run on the release branch PRs since this current workflow only runs when we make a new release. And at that point it's almost "too late". We ideally would use this feedback earlier in the release to fix issues. |
|
@tuliom planning on merging this - or do you need someone with access to do that? |
lit tests require commands FileCheck and not. They must be available in the PATH. This also guarantees that python3-psutil is installed in order to enable more tests. Fixes llvm#64892.
|
I've just updated the description in order to remove a paragraph that didn't make sense anymore after the reviews and squashed all commits. |
lit tests require commands FileCheck and not. They must be available in the PATH.
This also guarantees that python3-psutil is installed in order to enable more tests.
Fixes #64892.