Closed ghutchis closed 2 years ago
To be clear, I simply want to transform:
/home/runner/work/avogadrolibs/avogadrolibs/build/thirdparty/spglib-src
into
thirdparty/spglib-src
So
basedir = original_directory[:build_dir_index]
newbasedir = os.getcwd()
become something like:
basedir = original_directory[len(args.prefix_path):build_dir_index]
newbasedir = os.getcwd()
I don't think there's a way to get CMake or bear to use relative paths in compile_commands.json
. CMake, for instance, is just not designed to work like that.
But if you know in advance what the absolute path is, then we could take that as an argument and do as you suggest.
Just to make sure I'm doing the right transformation, can you give me an example of what basedir
should look like after transformation?
(I have a good idea, but want to ensure I don't introduce a subtle break.)
I can try a pull request today.
Sure, here's the output from one our recent runs:
Replacing 'Replacing '/home/runner/work/BOUT-dev/BOUT-dev' with '/github/workspace'' with '/github/workspace'
This results from:
args.build_dir
: build
original_directory
: /home/runner/work/BOUT-dev/BOUT-dev/build
basedir
: /home/runner/work/BOUT-dev/BOUT-dev
newbasedir
: /github/workspace
original_directory
is the "directory"
key for each element in compile_commands.json
. Currently, it assumes every file has the same directory -- or at least has the same basedir
part that needs replacing.
Here's an entry which causes issues for me:
[
{
"directory": "/__w/openblack/openblack/build/externals",
"command": "/usr/bin/clang++ -I/__w/openblack/openblack/externals/imgui -I/__w/openblack/openblack/externals/imgui_user -isystem /usr/include/SDL2 -g -std=gnu++17 -o CMakeFiles/imgui.dir/imgui/imgui.cpp.o -c /__w/openblack/openblack/externals/imgui/imgui.cpp",
"file": "/__w/openblack/openblack/externals/imgui/imgui.cpp"
}
]
Where build_dir
would be "build"
and the workspace root is /__w/openblack/openblack
It is like this because there is a sub CMake in externals:
So not every directory will be terminating in build_dir
, some will have subpaths.
Could you compute relative paths using https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.relative_to ?
@bwrsandman That's a very good point about the subpaths. I'm not quite sure how to handle that just yet -- I've not touched this in some time, so I need to remember how it works. I think the issue we're dealing with is that we don't necessarily know what the original directory is. This means that we can't even transform the paths to be relative, because we don't know which parts of the path we need to transform.
Currently, there's some logic to work out what the original directory is, which works by assuming that the first entry in compile_commands.json
has a directory
which is just /original_directory/build_dir
.
One fix would be to add an optional argument allowing the user to specify the original directory. That could default to pwd
, assuming we can get that from the github
context.
@ZedThree So what is the recommended way of handling the error now ?
I currently generate my compile databass with:
cmake . -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=1
I've got a branch, cmake-commands
, that now lets you run CMake inside the Action's docker container. This means we no longer need to fix the paths in compile_commands.json
.
@ghutchis @bwrsandman @revol-xut If you'd like to test this for your use case, that would be very helpful!
I can test this within an action on one of my repos. How can I use the cmake-commands
branch?
Thanks!
You can use it by specifying the branch instead of a tag, and then pass something to the cmake_command
argument:
- uses: ZedThree/clang-tidy-review@cmake-command
with:
build_dir: build
cmake_command: cmake . -B build
The issues that I'm running into is that it seems like it's running cmake from within a container. The version of cmake in your container (3.16) and my project requires 3.20. Also I have system dependencies such as SDL2 which aren't installed. I should note that this is new behavior. It wasn't like that before. Before (with manual fix using sed): https://github.com/openblack/openblack/runs/4218160573?check_suite_focus=true Now: https://github.com/openblack/openblack/runs/5014007413?check_suite_focus=true
Thanks for checking @bwrsandman.
It looks like it'll need a different fix for your project. I can install the latest cmake for the Action, and it's possible to install additional apt packages too, but that won't help if you're installing things not available in the ubuntu version used in the Action.
I'll see if I can get a better fix for your use case
@bwrsandman Please could you try the fix-absolute-paths
branch? I'm hoping the following will work:
- name: Create compile_commands.json
run: |
git submodule update --init externals/imgui
cmake . -B build -DCMAKE_EXPORT_COMPILE_COMMANDS=On
- name: Run clang-tidy
uses: ZedThree/clang-tidy-review@fix-absolute-paths
id: review
with:
build_dir: build
It's not completely clear to me if GITHUB_WORKSPACE
is the current directory for you. If it's not, you can set the base_dir
argument manually:
- name: Run clang-tidy
uses: ZedThree/clang-tidy-review@fix-absolute-paths
id: review
with:
build_dir: build
base_dir: $PWD
although the docs don't make it clear if environment variables work in that context. Worst case scenario you have to set it via a separate step:
- name: Set the value
id: step_one
run: |
echo "cwd=$(pwd)" >> $GITHUB_ENV
- name: Run clang-tidy
uses: ZedThree/clang-tidy-review@fix-absolute-paths
id: review
with:
build_dir: build
base_dir: ${{ env.cwd }}
The configure seems to have worked but looks like clang-tidy crashed???
I fixed a couple of silly bugs. Please try restarting the Action
I've confirmed that setting base_dir
via an environment variable as below works:
- name: Set the value
id: step_one
run: |
echo "base_dir=$(pwd)" >> $GITHUB_ENV
- name: Run clang-tidy
uses: ZedThree/clang-tidy-review@fix-absolute-paths
id: review
with:
base_dir: ${{ env.base_dir }}
Any idea why clang is crashing here? https://github.com/openblack/openblack/runs/5102794841?check_suite_focus=true#step:12:38
Sorry, yours is one of the cases where this action can't automatically work out the base_dir
, so you'll need to set it manually like this:
- name: Set the base directory for clang-tidy
run: |
echo "base_dir=$(pwd)" >> $GITHUB_ENV
- name: Run clang-tidy
uses: ZedThree/clang-tidy-review@v0.8.0
id: review
with:
build_dir: build
base_dir: ${{ env.base_dir }}
This is because you're running in an external container and $(pwd)
isn't the same as ${GITHUB_WORKSPACE}
, and from inside the Action it seems to be impossible to tell the real absolute path of the workflow running clang-tidy-review
.
This looks fantastic and I appreciate all the work that went into it.
I'm trying to figure out the details.. my most recent builds (https://github.com/OpenChemistry/avogadrolibs/pull/551/files) fail with:
Is there a good way to ensure only relative paths are stored in the JSON?
Could I add a parameter to the script/action to remove part of the absolute paths and have Python rewrite them to relative paths?