Python `error: externally-managed-environment` during installation about cpp-linter-action HOT 32 CLOSED

If the --user option is feasible fix then we could accept that change.

Great, I'll test this and make a PR.

The new error you encountered seems similar to cpp-linter/cpp-linter#30 which details known problems with the relative paths that Meson puts in the compilation database file; relative paths used in the database are not friendly with clang-tidy.

I think it's also because I need to build the entire project, not just configure it with Meson, to generate those xdg files. I'll test this too.

EDIT: building the entire project fixes that missing header file.

from cpp-linter-action.

I'm under the impression that that message only happens when pip install is invoked with sudo permission or as the root user. If the runner is somehow configured in such a way, then using a python venv probably won't fix the problem.

from cpp-linter-action.

Just tested by copying the action file to my own branch: https://github.com/zjeffer/Waybar/actions/runs/7129251571/job/19412927359

It seems to install & run cpp-linter correctly now, but I'm getting different errors now:

symbolic link created /github/home/.local/bin/clang-tidy
Run . "$GITHUB_ACTION_PATH/venv/bin/activate"
  
INFO:CPP Linter:Appending submodule to ignored paths: package/archlinux
INFO:CPP Linter:Ignoring the following paths/files:
	./.github
	./package/archlinux
INFO:CPP Linter:processing push event
Event json from the runner
Get list of specified source files
  INFO:CPP Linter:Fetching files list from url: https://api.github.com/repos/zjeffer/Waybar/commits/5f69d6e8675038369414f1b0d65320cd5dc14b32
  INFO:CPP Linter:Giving attention to the following files:
  	src/modules/hyprland/workspaces.cpp
Performing checkup on src/modules/hyprland/workspaces.cpp
  INFO:CPP Linter:Running "clang-tidy-17 --export-fixes=.cpp-linter_cache/clang_tidy_output.yml -p /__w/Waybar/Waybar/build --line-filter=[{"name": "src/modules/hyprland/workspaces.cpp", "lines": [[50, 51]]}] --extra-arg= src/modules/hyprland/workspaces.cpp"
  DEBUG:CPP Linter:Output from clang-tidy:
  ../include/bar.hpp:16:10: error: 'xdg-output-unstable-v1-client-protocol.h' file not found [clang-diagnostic-error]
     16 | #include "xdg-output-unstable-v1-client-protocol.h"
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  
  DEBUG:CPP Linter:clang-tidy made the following summary:
  13697 warnings and 2 errors generated.
  Error while processing /__w/Waybar/Waybar/src/modules/hyprland/workspaces.cpp.
  Suppressed 13697 warnings (13670 in non-user code, 17 due to line filter, 10 with check filters).
  Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
  Found compiler error(s).

but I guess I should open a new issue for that?

Is there a downside to using venv's for every OS, instead of just for macOS?

from cpp-linter-action.

I'm under the impression that that message only happens when pip install is invoked with sudo permission or as the root user. If the runner is somehow configured in such a way, then using a python venv probably won't fix the problem.

I don't think I'm necessarily invoking it with sudo or as the root user explicitly, I think it's because I'm using the debian image as a base. I don't get the errors when running on the default ubuntu-latest image (which maybe is configured to not run as root by default?)

What if you install the python requirements with --user?

from cpp-linter-action.

What if you install the python requirements with --user?

Maybe that would work. I can't say for certain if I don't know what exactly is causing the error. I've never actually tried using pip install as the root user.

You could make a fork and try it by changing the uses: field

    - uses: zjeffer/cpp-linter-action@test-branch-name
      name: clang-tidy
      id: clang-tidy-check
      env:
        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        PIP_NO_CACHE_DIR: false
      with:
        style: '' # empty string => don't do clang-format checks here, we do them in clang-format.yml
        files-changed-only: true # only check files that have changed
        lines-changed-only: true # only check lines that have changed
        tidy-checks: '' # empty string => use the .clang-tidy file
        version: '17' # clang-tools version
        database: 'build' # path to the compile_commands.json file

If the --user option is feasible fix then we could accept that change.

The new error you encountered seems similar to cpp-linter/cpp-linter#30 which details known problems with the relative paths that Meson puts in the compilation database file; relative paths used in the database are not friendly with clang-tidy.

from cpp-linter-action.

Adding --user to python3 -m pip install didn't fix it: https://github.com/zjeffer/Waybar/actions/runs/7129953996/job/19415221902

from cpp-linter-action.

It would be nice to see the output of echo "hello, $USER" when used within this docker.

• If it prints "hello, runner", then a venv would certainly be in order.
• If it prints "hello, root", then the docker needs to be configured with a non-root user.

Our previous attempt to use a python venv didn't work well because we were trying to avoid altering the PATH variable in the runner's context. It may be time to revisit that idea.

from cpp-linter-action.

Looking more into PEP668:

This PEP proposes a mechanism for a Python installation to communicate to tools like pip that its global package installation context is managed by some means external to Python, such as an OS package manager. It specifies that Python-specific package management tools should neither install nor remove packages into the interpreter’s global context, by default, and should instead guide the end user towards using a virtual environment.

Meaning, ALL pip usage should now be done within a venv. I see why they (pip as of v23.0) chose to add this notification, but I think it should've been a warning level notification, not an error (at least for a limited probation period).

This also means the ubuntu-latest runner image is still using an older pip version (v22.0.2 as of this writing) which is why the error hasn't surfaced yet (in our test or as user-feedback).

from cpp-linter-action.

@zjeffer I just pushed an attempt to fix this with a py venv. Please try our use-py-venv branch:

    - uses: cpp-linter/cpp-linter-action@use-py-venv

and let me know if that works better with the docker image (please include link to workflow run).

from cpp-linter-action.

It would be nice to see the output of echo "hello, $USER" when used within this docker.

• $USER outputs an empty string
• id outputs id: uid=0(root) gid=0(root) groups=0(root)

Please try our use-py-venv branch:
- uses: cpp-linter/cpp-linter-action@use-py-venv
and let me know if that works better with the docker image (please include link to workflow run).

https://github.com/zjeffer/Waybar/actions/runs/7133837047/job/19427361234

Run /__t/Python/3.11.6/x64/bin/python -m venv "$env:GITHUB_ACTION_PATH/venv"
  /__t/Python/3.11.6/x64/bin/python -m venv "$env:GITHUB_ACTION_PATH/venv"
  if ("Linux"  -ceq "macOS" -or "Linux" -ceq "Linux") {
    Invoke-Expression -Command "$env:GITHUB_ACTION_PATH/venv/bin/Activate.ps1"
  }
  else {
    Invoke-Expression -Command "$env:GITHUB_ACTION_PATH/venv/Scripts/Activate.ps1"
  }
  pip install -r "$env:GITHUB_ACTION_PATH/requirements.txt"
  clang-tools -i 17 -b
  shell: pwsh -command ". '{0}'"
  env:
    GITHUB_TOKEN: ***
    PIP_NO_CACHE_DIR: false
OCI runtime exec failed: exec failed: unable to start container process: exec: "pwsh": executable file not found in $PATH: unknown
Error: Process completed with exit code 126.

Why use powershell instead of bash?
You also don't have to use a different shell, instead of source (which doesn't work with sh, but it does with bash), you can replace it with . and keep using sh.

from cpp-linter-action.

Why use powershell instead of bash?

TL;DR Because bash can't properly handle windows style paths. C:\path\to\python.exe is used as C:pathotpython.exe. There's actually another problem related to this bash behavior when passing the absolute path to the compilation database on Windows runners.

I'm installing python v3.11 but not adding it to the system PATH (to avoid altering with the user's env). Instead I'm getting the python-3.11 exe path from the actions/setup-python output variable, which uses the path style appropriate to the runner's OS...

"pwsh": executable file not found in $PATH: unknown

That certainly is a problem. You can install pwsh in Linux, but it may not be desirable if only to use this action. I might be able to remedy this by ensuring pwsh is installed in the "Install Linux deps" step.

FWIW, self-hosted windows runners might have the same problem using bash. Thankfully git on windows ships with it's own dist of bash.exe, but no guarantee that its added to the system PATH.

I'm starting to lean toward calling this an edge case since the docker image you're using is running as root.

from cpp-linter-action.

I did do an experimental port of our python code to dart which would have much less problems as far as package install goes. We ended up going against that in deployment because not many involved in this project are familiar with dart.

Locally, I also have the beginnings for a rust port of our python code, but it is far from finished.

from cpp-linter-action.

What if you don't install Python in the action, but assume/require the host image to already have it installed? Then you don't have to pass the exe path through an output variable.

You'll then be able to create a venv with one command, regardless of the OS:
python3 -m venv $env:GITHUB_ACTION_PATH/venv
python3 -m pip install -r "$env:GITHUB_ACTION_PATH/requirements.txt"

And activate it based on the OS:

if windows; then
    # i don't know how this actually works, but I assume there is a way in windows to run a script from sh?
    . "$env:GITHUB_ACTION_PATH/venv/scripts/Activate.ps1" 
else:
    . "$env:GITHUB_ACTION_PATH/venv/bin/activate"
fi

from cpp-linter-action.

I might be able to remedy this by ensuring pwsh is installed in the "Install Linux deps" step.

Powershell requires the .NET core runtime as a dependency. This idea may be non-trivial.

from cpp-linter-action.

What if you don't install Python in the action, but assume/require the host image to already have it installed?

We don't support python 3.7 or earlier, so this assumption is a bit brittle IMO.

from cpp-linter-action.

We don't support python 3.7 or earlier, so this assumption is a bit brittle IMO.

I don't understand, don't you already assume it is installed? In the main branch, you're using python here: https://github.com/cpp-linter/cpp-linter-action/blob/main/action.yml#L134.

You're already using a venv if the OS is macOS, so my proposal is to use a venv for all OSes (like the PEP you posted recommends).

Something like this:

main...zjeffer:cpp-linter-action:main

(not tested on Windows but this is basically what I did in order to make it run in the Waybar repo, using a debian base image)

from cpp-linter-action.

I don't understand, don't you already assume it is installed?

We only assumed that users would be using github's runner images (ubuntu-latest, macos-latest, or windows-latest). At least then we know what the docker image has installed in it, and python v3.7 or earlier is not the default python installation with those images.

Something like this:

main...zjeffer:cpp-linter-action:main

(not tested on Windows but this is basically what I did in order to make it run in the Waybar repo, using a debian base image)

Again the only problem with that patch is that it will not run on Windows because the bash shell removes the windows path delimiters. For example:
$GITHUB_ACTION_PATH resolves to (on Windows) D:\a\_actions\cpp-linter\cpp-linter-action, but in bash this path becomes D:a_actionscpp-lintercpp-linter-action. Remember that the path to the compilation database suffers the same problem in bash on windows runners.

I don't see a robust way around this problem since we can't be expected to know what is or isn't installed in docker images that are not maintained by github actions. We may require users install pwsh in workflows that use non-default docker images in github actions runners (same would apply to self-hosted runners).

from cpp-linter-action.

I'm using Waybar's debian image as a base because I need to configure with meson before running clang-tidy, to generate compile_commands.json. I noticed that with this image, I had to install both sudo and python3-pip to fix errors during the installation.

Since you already understand that further customization is needed for this docker image, what would be the harm in adding/installing pwsh as well?

from cpp-linter-action.

Please note that this action will also break if a fedora or non-debian based linux docker image is used. What I'm trying to say is that we can't be expected to support all possible CI configs because github actions is so flexible. What we have currently took a lot of time and learning to establish (with no funding).

from cpp-linter-action.

An alternative approach would be to use our cpp-linter python package by itself. But without the nuanced setup process that this repo performs, users would still have to ensure that clang-format and clang-tidy are installed before running our cpp-linter CLI. See also pipx as it was designed for that kind of scenario.

from cpp-linter-action.

@zjeffer It isn't pretty, but I used steps.if field to trigger certain shells native to the detected OS. It involves having 2 separate (& nearly identical) scripts for unix or windows. Please try the use-py-venv branch again.

from cpp-linter-action.

Again the only problem with that patch is that it will not run on Windows because the bash shell removes the windows path delimiters. For example:
$GITHUB_ACTION_PATH resolves to (on Windows) D:\a_actions\cpp-linter\cpp-linter-action, but in bash this path becomes D:a_actionscpp-lintercpp-linter-action. Remember that the path to the compilation database suffers the same problem in bash on windows runners.

I tested this on this branch and I can't reproduce the issue with the GITHUB_ACTION_PATH resolving to that path without slashes. Am I testing this wrong?

Here's the workflow: https://github.com/zjeffer/cpp-linter-action/actions/runs/7143659190

I don't see a robust way around this problem since we can't be expected to know what is or isn't installed in docker images that are not maintained by github actions. We may require users install pwsh in workflows that use non-default docker images in github actions runners (same would apply to self-hosted runners).

I fully understand where you're coming from, but I just really dislike PowerShell ;)

@zjeffer It isn't pretty, but I used steps.if field to trigger certain shells native to the detected OS. It involves having 2 separate (& nearly identical) scripts for unix or windows. Please try the use-py-venv branch again.

Works: https://github.com/zjeffer/Waybar/actions/runs/7143645392/job/19455586940

from cpp-linter-action.

I tested this on this branch and I can't reproduce the issue with the GITHUB_ACTION_PATH resolving to that path without slashes. Am I testing this wrong?

That seems like you did everything right. We haven't had success last time we tried that though (probably about a year ago now). I'm still curious to see how the path to the database is shown in bash on Windows (originally reported in #155).

Works: https://github.com/zjeffer/Waybar/actions/runs/7143645392/job/19455586940

Good to hear! I'm not a fan of maintaining duplicate scripts. Thankfully the scripts used here are very basic/simple. I'll keep playing around with this as I'd like to be sure that #155 is resolved (absolute paths should be preferred on any OS).

I fully understand where you're coming from, but I just really dislike PowerShell ;)

This may sound like a broken record, but I can't assume that bash is available on all Windows runners. I can assume that pwsh is because it is shipped with the OS (meant to be a replacement for the old DOS CLI). Just to be clear, bash can be installed on Windows and usually is when git is installed on windows. Luckily, Github doesn't use SVN or Mercurial, but that doesn't guarantee a self hosted Windows runner will have git (& bash.exe) installed and added to PATH. Likewise, we already know that it isn't safe to assume pwsh isn't guaranteed to be installed on all Unix OS.

PS - I'm not a huge fan of the powershell conventions, ie. casing, very long winded API names that might be hyphenated (seems like it was developed by Visual Basic experts), and no way to escape the LF for multi-line statements. But I am a fan of whatever works 😎.

from cpp-linter-action.

That seems like you did everything right. We haven't had success last time we tried that though (probably about a year ago now). I'm still curious to see how the path to the database is shown in bash on Windows (originally reported in #155).

Strangely, that bug still occurs. I'll try to find a fix.

This may sound like a broken record, but I can't assume that bash is available on all Windows runners. I can assume that pwsh is because it is shipped with the OS (meant to be a replacement for the old DOS CLI). Just to be clear, bash can be installed on Windows and usually is when git is installed on windows. Luckily, Github doesn't use SVN or Mercurial, but that doesn't guarantee a self hosted Windows runner will have git (& bash.exe) installed and added to PATH. Likewise, we already know that it isn't safe to assume pwsh isn't guaranteed to be installed on all Unix OS.

I think we have a couple of choices:

1. Use steps.if to choose between shells based on the runner
2. Assume bash is required to be installed on Windows runners and use bash everywhere
3. Assume pwsh is required to be installed on Linux runners and use pwsh everywhere

1 works but is harder to maintain and not as pretty.
Personally, I would prefer 2. The problem is that #155 still occurs.
I don't think 3 is a good idea because I believe there are a lot more people with Linux runners than Windows runners, and I can imagine most people install bash on Windows runners anyway, while very few people install pwsh on Linux runners.

from cpp-linter-action.

I've been coding for nearly ten years now, and I've found it is best to avoid writing code with assumptions in mind. Thus, options 2 and 3 aren't very appealing to me. I can cope with a little duplication (the scripts here aren't very complex).

Unfortunately, as we've seen here, it is hard to know when you are working with an assumption in mind, like the fact that all users will only be using github's runner images and not some custom docker image.

I tried reproducing #155 with the duplicate script approach. The path passed to the database input option needs to use delimiters appropriate to the OS used. I think I can fix that problem in our python code's run_clang_tidy(), but it seems that clang-tidy is still failing to find the database when given a path it can understand and traverse. See my experiment results in this workflow run.

from cpp-linter-action.

I think I fixed the database path in the action.yml file itself: main...zjeffer:cpp-linter-action:feat/zjeffer/test#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R150
If we simply replace backslashes with forward slashes, it works: https://github.com/zjeffer/cpp-linter-action/actions/runs/7151821064/job/19476496676

from cpp-linter-action.

Technically you have a path that uses both / and \ because the ${{github.workspace}} uses Windows path delimiters. I tried the same but got bad results (see run log here). However, I didn't terminate the path with a delimiter in my attempts. I wonder if that is important or crucial for clang-tidy's internal path handing.

from cpp-linter-action.

However, I didn't terminate the path with a delimiter in my attempts. I wonder if that is important or crucial for clang-tidy's internal path handing.

Nope. Still got Could not auto-detect compilation database from directory "D:\a\test-cpp-linter-action\test-cpp-linter-action/build/" in clang-tidy output.

@zjeffer The other difference between our attempts at reproducing #155 is that I'm using a CMake generated compilation database (compile_commands.json). Your attempt is using a static global compilation database (compile_flags.txt). It is much more common to use a generated database as the build system tends to be manipulated often during development.

from cpp-linter-action.

Technically you have a path that uses both / and \ because the ${{github.workspace}} uses Windows path delimiters. I tried the same but got bad results (see run log here). However, I didn't terminate the path with a delimiter in my attempts. I wonder if that is important or crucial for clang-tidy's internal path handing.

Well no, in my case I replace the backslashes with forward slashes right before passing it to cpp-linter:

https://github.com/zjeffer/cpp-linter-action/actions/runs/7151821064/job/19476496676#step:4:116

database_path: D:\a\cpp-linter-action\cpp-linter-action/demo/
database_path after replacing: D:/a/cpp-linter-action/cpp-linter-action/demo/ <= i pass this to cpp-linter

Here's what I do in action.yml: https://github.com/zjeffer/cpp-linter-action/blob/feat/zjeffer/test/action.yml#L146

database_path=$(printf "%s" "${{ inputs.database }}" | sed 's:\:/:g')

(echo removed the backslashes so I had to use printf)

from cpp-linter-action.

in my case I replace the backslashes with forward slashes right before passing it to cpp-linter

Hmm. So you basically workaround the fact that bash can't handle windows path delimiter (outside of regex patterns) by adding extra logic to the bash script. I prefer all our path manipulation happen in our python codebase.

You're also relying on printf being available/installed. I would call that an assumption, but it might be safe to assume all bash-enabled environments have the printf command available.

Similarly, sed may be common to most debian distros, but I have read about instances where it wasn't part of the distro.

from cpp-linter-action.

Hmm. So you basically workaround the fact that bash can't handle windows path delimiter (outside of regex patterns) by adding extra logic to the bash script. I prefer all our path manipulation happen in our python codebase.

Ah, I understand now. I was under the impression the path to the database that was being passed to cpp-linter had no slashes. If it gets passed correctly to python, then it indeed makes sense to let Python replace the backslashes with forward slashes. Have you tried that yet?

from cpp-linter-action.

If it gets passed correctly to python, then it indeed makes sense to let Python replace the backslashes with forward slashes. Have you tried that yet?

It doesn't get correctly passed to python. At least that's how #155 was reported.

In python, we just normalize a relative path to DB into an absolute path, and use an absolute path to DB is used as is. See code here

from cpp-linter-action.