-
Notifications
You must be signed in to change notification settings - Fork 20
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
Python error: externally-managed-environment
during installation
#171
Comments
I'm under the impression that that message only happens when |
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:
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? |
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? |
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 You could make a fork and try it by changing the - 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 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. |
Great, I'll test this and make a PR.
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. |
Adding --user to |
It would be nice to see the output of
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. |
Looking more into PEP668:
Meaning, ALL This also means the |
@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). |
https://github.com/zjeffer/Waybar/actions/runs/7133837047/job/19427361234
Why use powershell instead of bash? |
TL;DR Because bash can't properly handle windows style paths. 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...
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. |
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. |
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: 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 |
Powershell requires the .NET core runtime as a dependency. This idea may be non-trivial. |
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) |
We only assumed that users would be using github's runner images (
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: 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). |
Since you already understand that further customization is needed for this docker image, what would be the harm in adding/installing pwsh as well? |
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). |
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 |
@zjeffer It isn't pretty, but I used |
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 fully understand where you're coming from, but I just really dislike PowerShell ;)
Works: https://github.com/zjeffer/Waybar/actions/runs/7143645392/job/19455586940 |
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).
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).
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 😎. |
Strangely, that bug still occurs. I'll try to find a fix.
I think we have a couple of choices:
1 works but is harder to maintain and not as pretty. |
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 |
I think I fixed the database path in the action.yml file itself: main...zjeffer:cpp-linter-action:feat/zjeffer/test#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R150 |
Technically you have a path that uses both |
Nope. Still got @zjeffer The other difference between our attempts at reproducing #155 is that I'm using a CMake generated compilation database ( |
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
Here's what I do in action.yml: https://github.com/zjeffer/cpp-linter-action/blob/feat/zjeffer/test/action.yml#L146
(echo removed the backslashes so I had to use printf) |
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 Similarly, |
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? |
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 |
* use a python venv adheres to PEP668 and addresses #171 * use powershell to manage py venv in windows runners * leverage steps.if with duplicated scripts should support thee OS's native shell * Add warning to README about debian only support
I'm setting up a clang-tidy action for the Waybar repo: https://github.com/zjeffer/Waybar/blob/90208f8c17b815e3a50265e679a184354f5dcd9b/.github/workflows/clang-tidy.yml
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
andpython3-pip
to fix errors during the installation.After this, there's one error left when it runs
python3 -m pip install -r "$GITHUB_ACTION_PATH/requirements.txt"
in this repo's action.yaml:I was wondering if this error could be fixed by using a venv for all OSes, not just macOS like what you're doing here: https://github.com/cpp-linter/cpp-linter-action/blob/v2/action.yml#L130-L134. Would that fix this error?
The text was updated successfully, but these errors were encountered: