Skip to content

Why prepend a path to database? #73

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

Closed
2bndy5 opened this issue Jul 17, 2022 · 20 comments
Closed

Why prepend a path to database? #73

2bndy5 opened this issue Jul 17, 2022 · 20 comments
Labels
compilation database clang-tidy didn't use a compilation database

Comments

@2bndy5
Copy link
Collaborator

2bndy5 commented Jul 17, 2022

Why prepend a base path at all and not just pass it to clang-tidy as specified? That would be less surprising since all other paths are relative to the current working directory as well (e.g. ignore).

Originally posted by @BurningEnlightenment in #65 (comment)

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2022

Moved this question to a new thread because it could be a new feature if we work it right.

@2bndy5 2bndy5 added the compilation database clang-tidy didn't use a compilation database label Jul 17, 2022
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2022

Note to self: This feature would be considered a breaking change (major version bump).

@BurningEnlightenment
Copy link

I just had a look at your sources; is there a specific reason for using RUNNER_WORKSPACE instead of GITHUB_WORKSPACE?

RUNNER_WORKSPACE = os.getenv("RUNNER_WORKSPACE", "")

if RUNNER_WORKSPACE:
path_to_db = RUNNER_WORKSPACE

According to the github actions documentation GITHUB_WORKSPACE points to the default checkout path. While I find no mention of RUNNER_WORKSPACE it seems to always be pointing to the wrong directory in this case.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2022

Because the runner workspace is different from the docker's workspace.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2022

@shenxianpeng It might be easier to identify when using the docker at runtime if the docker image set a environment variable. We could probably do this from the action.yml in this repo, but it might be good to have in the docker container.

If we could more easily identify when the docker is in use (at runtime), then we can make this path prepending conditional on the presence of the docker workspace.

@BurningEnlightenment
Copy link

A mostly backward compatible option could be to only prepend a path if database isn't an absolute path. In which case I could simply do --database=${{ github.workspace }}/build/x64-linux-clang.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 17, 2022

I like where your head is at! If you're comfortable with python, you could submit a PR with that idea. Otherwise, it may take me a while to carve out some time to test/implement it myself.

@shenxianpeng
Copy link
Collaborator

@2bndy5 OK, we can set an environment variable to the docker image to identify it is running.

@shenxianpeng
Copy link
Collaborator

A new docker image xianpengshen/clang-tools:all with environment variable CLANG_VERSIONS is ready.

Status: Downloaded newer image for xianpengshen/clang-tools:all
root@bb3520f7c535:/src# echo $CLANG_VERSIONS
14 13

2bndy5 added a commit that referenced this issue Jul 26, 2022
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

I'm starting to think that making the given database path absolute is required for compatibility with all versions of clang-tidy... I'm getting inconsistent results in the unit test.

  • clang-tidy's working directory may not be the same as the repo-root option (even though the action does cd repo-root)
  • clang-tidy v10 seems to attempt making the database path absolute if it is given as relative

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

Turns out RUNNER_WORKSPACE is always declared (even when not using the docker env), but it is only useful to us when in the docker env. So, I used the new docker env var CLANG_VERSIONS to identify when to use GITHUB_WORKSPACE. Now the action correctly converts a relative database path to absolute if the given path to database is relative.

The unit tests are passing now, but I have to make sure this doesn't break compatibility with the docker env (unit tests are not using the docker env).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

@shenxianpeng I don't think the new env var from the docker image can be accessed from the github runner. I'm setting a custom env var from actions.yml instead.

It looks like the database option wasn't working before... See #65 (comment) for context. The action wasn't showing any problems when clang-tidy exited with 0. But now I have the action displaying any stderr output (despite the return code), and the RUNNER_WORKSPACE and GITHUB_WORKSPACE vars don't seem to point to the correct root path.

Tested with docker env

With RUNNER_WORKSPACE:

Could not auto-detect compilation database from directory "/home/runner/work/test-cpp-linter-action/build"
  No compilation database found in /home/runner/work/test-cpp-linter-action/build or any parent directory

With GITHUB_WORKSPACE:

LLVM ERROR: Cannot chdir into "/home/runner/work/test-cpp-linter-action/test-cpp-linter-action/build"!

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

I think part of the problem is that everything the user does in a github workflow is executed with sudo permission.

Maybe the programs in the docker doesn't have the same root user permission. So, when we create a build dir and generate a database from other steps in the workflow, this action's docker-contained clang-tidy may not be able to freely browse the build dir (owned by the user root).

@shenxianpeng
Copy link
Collaborator

shenxianpeng commented Jul 29, 2022

About Docker container filesystem. and we don't set USER instruction in Dockerfile.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

ok I think the GITHUB_WORKSPACE var is specific to the runner's context, not the docker's context. I added some log output to see the contents of the WORKDIR:

working dir: /github/workspace
  DEBUG:CPP Linter:
  	.git
  	.github
  	build
  	src
  	.clang-tidy
  	LICENSE
  	.gitignore
  	.clang-format
  	README.md

where the GITHUB_WORKSPACE env var resolves to /home/runner/work/test-cpp-linter-action/test-cpp-linter-action, we need to use github/workspace when running in the docker.

@BurningEnlightenment This might also mean that if a user passes an absolute path like ${{ github.workspace }}/build when using the docker, then the we might have to "normalize" the path with respect to /github/workspace.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

I think there's something else going on (probably a symlink resolution).

  INFO:CPP Linter:Running "clang-tidy-11 --export-fixes=clang_tidy_output.yml -p /github/workspace/build src/demo.hpp"

  ...  

  INFO:CPP Linter:clang-tidy made the following summary:
  LLVM ERROR: Cannot chdir into "/home/runner/work/test-cpp-linter-action/test-cpp-linter-action/build"!

So, now I'm passing the right path to the database (/github/workspace/build), but clang-tidy seems to resolve it as /home/runner/work/test-cpp-linter-action/test-cpp-linter-action/build

I'm not sure we can even support the database option on the docker. 😡 Maybe if we only use relative path to the database on the docker, but I think I need to inspect the clang-tidy src (which is not easy because it is an abstraction from clang's tooling library).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

Using relative only paths to a db didn't work. Clang-tidy still looked for the db using an absolute path in the runner's context. Reverting that attempt/commit since it's better to use absolute paths when possible.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Jul 29, 2022

Maybe if we copy the db file to the repo-root, then clang-tidy will find it implicitly (without using the -p CLI arg) as that is the default behavior. If the db file is faulty, then there may not be any warning from clang-tidy to indicate that the db file wasn't used.

This would open us up to other problems like naming scheme for various generated db files; most use the filename compile_commands.json, but clang tools also support a generic "compile_flags.txt" (which must use relative paths within). I suppose we could detect if the db path is a dir or a file too.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 8, 2022

Well, this issue is mostly solved for users running this action as a python package. The database option may have to remain broken for users using the docker image. I'd temporarily encourage users to copy the database over to the root source directory of the repo being linted.

We can probably change this action into a composite action (a series of run commands that doesn't need a docker image) in combination with the cpp-linter/clang-tools-pip project. From the user perspective, nothing would really change (other than a faster setup time). But first, the binaries for v14 need to be fixed (& hopefully v15 gets added) over at muttleyxd/clang-tools-static-binaries.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Aug 10, 2022

So, I modified the test repo's CI to do

- name: generate database
  run: cmake -Bbuild src -DCMAKE_EXPORT_COMPILE_COMMANDS=ON
- name: Move database into src folder for implicit clang-tidy discovery
  run: mv build/compile_commands.json src/
- name: Run cpp-linter as action (not python pkg)
  uses: cpp-linter/cpp-linter-actioin@backlogged-updates
  with:
    ignore: build  # does not affect clang-tidy directly (& not supposed to)
    version: 12
    file-annotations: true

Unfortunately, clang-tidy finds the build folder and tries to traverse it anyway.

  INFO:CPP Linter:Running "clang-tidy-12 --export-fixes=clang_tidy_output.yml -checks=boost-*,bugprone-*,performance-*,readability-*,portability-*,modernize-*,clang-analyzer-*,cppcoreguidelines-* src/demo.cpp"
  DEBUG:CPP Linter:Output from clang-tidy:
  
  DEBUG:CPP Linter:clang-tidy made the following summary:
  LLVM ERROR: Cannot chdir into "/home/runner/work/test-cpp-linter-action/test-cpp-linter-action/build"!

This error prevents clang-tidy from doing any further analysis, meaning there are no file annotations.


Running the action as python pkg seems to have no problems converting a relative database path into an absolute path. Everything is working as expected there. So, I think the only real way to fully support a generated database, is to convert the action into a composite action (which removes the use of the docker env).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compilation database clang-tidy didn't use a compilation database
Projects
None yet
Development

No branches or pull requests

3 participants