Skip to content
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

fix: expose python libraries #184

Closed
wants to merge 1 commit into from
Closed

fix: expose python libraries #184

wants to merge 1 commit into from

Conversation

carlosala
Copy link
Contributor

When update-environment is false, setup-python does not export LD_LIBRARY_PATH and PKG_CONFIG_PATH variables, which are required to find properly libpython.so and this kind of libs. Without this change, it's required to have the libpython311.so already installed in the system, that kinda breaks the purpose of using setup-python.
Thanks for the great work you do!

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 30, 2024

We chose to use a direct path to the python executable. This way the user-configured workflow/env will not conflict with the venv that cpp-linter-action uses.

If you need a python setup where the env is updated/altered, then you can still use actions/setup-python in your own workflow.

@carlosala
Copy link
Contributor Author

I don't need it, the problem is when creating the venv. It's not able to find libpython when doing python -m venv.
Maybe we could export the variables right there

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 30, 2024

Activating the venv does export modifications to env variables, but I'm more concerned with the fact that you are using a python venv that is supposed to be specific to cpp-linter-action.

Is there a reason you can't use actions/setup-python in your user workflow?

@@ -117,7 +117,6 @@ runs:
id: setup-python
with:
python-version: '3.11'
update-environment: false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are undesired, unless you can tell me why you want to use the same venv this action uses instead of setting up your own venv.

@carlosala
Copy link
Contributor Author

I think I got misunderstood. The problem I face is in the venv creation that cpp-linter-action does here.
With the following minimal example it can be reproduced:

name: Test
on:
  push:

jobs:
  cpp-linter:
    runs-on: ubuntu-latest
    container: ubuntu:22.04
    steps:
      - name: Install missing deps
        env:
          DEBIAN_FRONTEND: noninteractive
        run: apt-get update -y && apt-get install -y sudo
      - uses: cpp-linter/cpp-linter-action@latest

Here you have a run with it. Note the error that some python library is missing there.

@carlosala
Copy link
Contributor Author

Note that in the minimal example I'm using a container (ubuntu:22.04 in that case) that does not have python3.11 installed.

@carlosala carlosala requested a review from 2bndy5 January 30, 2024 23:04
@carlosala
Copy link
Contributor Author

carlosala commented Jan 30, 2024

We could export the necessary variables to avoid polluting the env, but the internals of the action shouldn't rely on the particular system being used.
I now realized that I wasn't clear at all in my explanations, sorry for that 😉

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 30, 2024

And you can't use actions/setup-python to installed a cached version of python 3.11?

Honestly, we can't be expected to support every docker image that people throw at us. If your docker image doesn't have python 3.11 installed, then your workflow needs to ensure it is installed.

@carlosala
Copy link
Contributor Author

carlosala commented Jan 30, 2024

I see, but I don't need python at all. It's just used in the internals of cpp-linter-action. For example, only by changing the version of python used in the action would basically break any container that does not have that installed.
And it's not any container, I'd say is the exact requirement specified in the README.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 31, 2024

My concern is when users build with a python venv of their own (common practice for meson and bazel build system generators). We don't want cpp-linter-action deps to cause a conflict with build env's dependencies. By updating the environment, then we risk causing conflicts.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 31, 2024

Reading the docs for update-environment, I don't thing this flag will update the LD_LIBRARY_PATH env var.

I think the problem is from trying to install one of cpp-linter's dependencies, pygit2, when using a docker image that isn't fully prepared to build python c-extensions from source. If pygit2 is installing from source (instead of from a compiled binary wheel), then cffi is used and would require the python3-dev and libffi-dev apt packages.

@carlosala
Copy link
Contributor Author

This is what update-environment does.

@carlosala
Copy link
Contributor Author

I mean, it gets solved by setting setup-python right before cpp-linter-action, but I'd say it should be included. At this point not executing setup-python inside cpp-linter-action has the same effect as doing it, since the libraries are not properly set I'd say.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 31, 2024

At this point not executing setup-python inside cpp-linter-action has the same effect as doing it

This is not true. You are getting an error because you are using a docker image that is not well enough equipped for cpp-linter-action.

Tip

Using ubuntu-latest as the job's base image does not help when another docker image is specified.

since the libraries are not properly set I'd say.

I'm not exactly sure what you mean here. Installing our cpp-linter python package requires a few other third-party dependencies be installed. How those dependencies are distributed (binary wheels for pygit2 may not be available for all OS and architecture types) are not within our control. The dependencies and our python cpp-linter package are all installed by pip, which can have different behavior depending on the environment used.

If you don't know the difference between a binary wheel versus a source distribution of a python package (written in C or pure python), then I'd say that you have misdiagnosed the problem.

@carlosala
Copy link
Contributor Author

Looking at the minimal example I posted, here you have the line with the error. The problem appears when executing python (in the venv creation inside cpp-linter-action). Since CPython is being used, libpython must be correctly installed and set in the system.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 31, 2024

libpython3.11.so.1.0: cannot open shared object file: No such file or directory

libpython3.11.so is the python library for building python libraries/packages from C sources. The only dependency written in C that we rely on is pygit2. Please add python3-dev and libffi-dev packages to you workflow's "Install missing deps" step. There may be other libraries missing (maybe libssl-dev) from the Ubuntu:22.04 docker image that you are using.

@carlosala
Copy link
Contributor Author

That's the first thing I tried, installing python3-dev and avoid opening a PR but the version in latest ubuntu LTS is python 3.10, so that does not help.
I as well tried if adding setup python was making any difference in the action, and tested against your test suite. It seems that is not making any difference.
My whole point is that python usage in the internals of cpp-linter-action relies on the system python (at least in Linux systems, of course), so having setup-python is not needed, or is not properly set up.

@carlosala
Copy link
Contributor Author

I'm sorry if I'm not being clear, or if you are fine with the current situation of relying on the system Python installation, that would be a design decision that is completely fine.
Thanks once again for the work in the project.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 31, 2024

We are using actions/setup-python to copy a cached build of python at a specified version. We do this to avoid using any system python installed as we cannot garauntee the installed python version is compatible with cpp-linter pkg (v3.8+ should work fine).

Honestly, to solve your problem would require intimate understanding of exactly what is shipped with that docker image you are using. I'm not open to these changes because... #184 (comment)

Ultimately, there seems to be a problem with installing python C-extension package (likely pygit2). I would start there.

@2bndy5
Copy link
Collaborator

2bndy5 commented Jan 31, 2024

I would be open to an input option that specifies what version of python to use. But that seems like it would be specific to edge cases like this.

@carlosala
Copy link
Contributor Author

I understand your point. Even if it's just for cpp-linter, I'll set up python 3.11.
One thing that would be nice to have in the README is that Python 3.11 needs to be installed in the base image; that would extend the requirements and make it easier to understand.
Thanks for your time, have a nice day!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants