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

Add bindep for package dependencies #10474

Closed
wants to merge 1 commit into from

Conversation

rhmdnd
Copy link
Collaborator

@rhmdnd rhmdnd commented Apr 19, 2023

We have a bunch of utility scripts and some of them require package
dependences instead of dependencies we can manage with a
requirements.txt (or pip).

This commit introduces a tool call bindep [0] that helps python
projects manage package dependencies.

To use it, you can install bindep:

$ pip install bindep
$ sudo yum install $(bindep -b)

@rhmdnd rhmdnd requested a review from Mab879 April 19, 2023 20:52
@github-actions
Copy link

github-actions bot commented Apr 19, 2023

Start a new ephemeral environment with changes proposed in this pull request:

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@Mab879 Mab879 self-assigned this Apr 19, 2023
@Mab879
Copy link
Member

Mab879 commented Apr 19, 2023

Helps address #7988

@Mab879 Mab879 added this to the 0.1.68 milestone Apr 19, 2023
@Mab879 Mab879 added Documentation Update in project documentation. Highlight This PR/Issue should make it to the featured changelog. labels Apr 19, 2023
@rhmdnd
Copy link
Collaborator Author

rhmdnd commented Apr 21, 2023

I took this a step further by including all the various packages in the documentation in the bindep.txt file.

However, we can have multiple dep files, keep the main bindep.txt file for build dependencies, and put other things in doc-bindep.txt for example.

I can see how someone might not want to install the entire dependency set if they're not going to use it all.

Copy link
Member

@Mab879 Mab879 left a comment

Choose a reason for hiding this comment

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

I did some test on RHEL 9, F37, and Ubuntu 22.04, and Ubuntu 23.04, and I left some feedback.

I am unsure about how to best handle the Python dependencies as RHEL doesn't have many of the Python deps that Ubuntu and Fedora have.

bindep.txt Outdated Show resolved Hide resolved
bindep.txt Outdated Show resolved Hide resolved
bindep.txt Outdated Show resolved Hide resolved
bindep.txt Outdated Show resolved Hide resolved
bindep.txt Outdated
python-jinja2 [platform:rpm]
ninja-build [platform:rpm]

# Red Hat Enterprise Linux 8 and Fedora dependencies, primarily for supporting Python 3
Copy link
Member

Choose a reason for hiding this comment

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

No match for argument: ansible-lint
No match for argument: bats
No match for argument: ninja-build
No match for argument: python3-PyGithub
No match for argument: python3-mypy
No match for argument: python3-openpyxl
No match for argument: python3-pandas
No match for argument: python3-sphinx
No match for argument: yamllint

These will need [platform: fedora] or somehow get rid of RHEL as RHEL doesn't have these packages. How that leaves our requirements.txt.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I took a pass at moving these into a Fedora section.

@rhmdnd
Copy link
Collaborator Author

rhmdnd commented Apr 24, 2023

I am unsure about how to best handle the Python dependencies as RHEL doesn't have many of the Python deps that Ubuntu and Fedora have.

Could we work around the issue by moving them to a python-based dependency management tool, and forego the system packages that differ between platforms?

We have a bunch of utility scripts and some of them require package
dependences instead of dependencies we can manage with a
requirements.txt (or pip).

This commit introduces a tool call bindep [0] that helps python
projects manage package dependencies.

To use it, you can install bindep:

$ pip install bindep
$ sudo yum install $(bindep -b)

```bash
apt-get install cmake make libopenscap8 libxml2-utils ninja-build python3-jinja2 python3-yaml python3-setuptools xsltproc
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't think the ninja-build package was necessary by default (as noted further down in the documentation), so I didn't include it for the Debian-based systems.

Please correct me if I'm wrong though.

@codeclimate
Copy link

codeclimate bot commented Apr 24, 2023

Code Climate has analyzed commit 4c1d7af and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 52.4% (0.0% change).

View more on Code Climate.

@rhmdnd
Copy link
Collaborator Author

rhmdnd commented Apr 24, 2023

Python-specific dependencies that aren't packaged by distributions are being handled in #10487

@matejak
Copy link
Member

matejak commented Apr 25, 2023

A bit late to the party, but I am not so happy with introducing a tool that is not so common to manage dependencies on a project that has relatively simple dependency tree.
We have talked about it in our group, and came up with these findings:

  • It is possible to install build dependencies using dnf builddep ..., a similar mechanism exists on Debian-based systems as well.
  • Missing dependencies could then be installed by pip install -r .... Documenting these two steps should be easy and future-proof.
  • Those two steps won't catch non-Python non-build deps s.a. shellcheck or bats.
  • There are Ansible roles in our documentation focused on obtaining a developer environment.

As the bindep is not widely known, not packaged, and its syntax is far from self-explanatory, I am not so happy about including this bit of technology in the project.
Please give it a thought again, and proceed if you are still convinced that it is worth it.
As a side note, would it make sense to work with those Ansible roles to increase their usability, or is Ansible simply too heavyweight?

@rhmdnd
Copy link
Collaborator Author

rhmdnd commented Apr 25, 2023

A bit late to the party, but I am not so happy with introducing a tool that is not so common to manage dependencies on a project that has relatively simple dependency tree. We have talked about it in our group, and came up with these findings:

  • It is possible to install build dependencies using dnf builddep ..., a similar mechanism exists on Debian-based systems as well.

Cool, so you're envisioning dnf builddep scap-security-guide.spec to handle all the build dependencies?

  • Missing dependencies could then be installed by pip install -r .... Documenting these two steps should be easy and future-proof.

I proposed #10487 to implement requirements files for tracking python dependencies that aren't packaged by distributions. Sounds like this is still useful.

  • Those two steps won't catch non-Python non-build deps s.a. shellcheck or bats.

  • There are Ansible roles in our documentation focused on obtaining a developer environment.

Nice, yeah I dug through these, but they installed a bit more than what I needed.

As the bindep is not widely known, not packaged, and its syntax is far from self-explanatory, I am not so happy about including this bit of technology in the project. Please give it a thought again, and proceed if you are still convinced that it is worth it.

That's fair. I proposed using bindep as the result of a side discussion with @Mab879 about general dependency management.

As a side note, would it make sense to work with those Ansible roles to increase their usability, or is Ansible simply too heavyweight?
If ansible is the preferred method for standing up development environments, then investing there makes sense.

I didn't see a way to restrict the installed packages to only what I needed short of modifying the playbook directly.

@matejak
Copy link
Member

matejak commented Apr 26, 2023

...

Cool, so you're envisioning dnf builddep scap-security-guide.spec to handle all the build dependencies?

Just checked, dnf builddep scap-security-guide seems to do the right thing.

  • Missing dependencies could then be installed by pip install -r .... Documenting these two steps should be easy and future-proof.

I proposed #10487 to implement requirements files for tracking python dependencies that aren't packaged by distributions. Sounds like this is still useful.

Yes, the format is simple, and useful for those without access to package managers support.

...

As a side note, would it make sense to work with those Ansible roles to increase their usability, or is Ansible simply too heavyweight?
If ansible is the preferred method for standing up development environments, then investing there makes sense.

I didn't see a way to restrict the installed packages to only what I needed short of modifying the playbook directly.

Pinging @marcusburghardt. I am not sure to what position we got though - perhaps we have the minimal set of packages covered by dnf, so the Ansible way makes sense as it is for developers who want more rounded environment, so no modifications are actually needed? Or would it still make sense to have a multi-tier package installation option?

Anyway, I feel that we need to update some documentation, but bindep may be an overkill. Would you agree with this?

@marcusburghardt
Copy link
Member

...

Cool, so you're envisioning dnf builddep scap-security-guide.spec to handle all the build dependencies?

Just checked, dnf builddep scap-security-guide seems to do the right thing.

  • Missing dependencies could then be installed by pip install -r .... Documenting these two steps should be easy and future-proof.

I proposed #10487 to implement requirements files for tracking python dependencies that aren't packaged by distributions. Sounds like this is still useful.

Yes, the format is simple, and useful for those without access to package managers support.

...

As a side note, would it make sense to work with those Ansible roles to increase their usability, or is Ansible simply too heavyweight?
If ansible is the preferred method for standing up development environments, then investing there makes sense.

I didn't see a way to restrict the installed packages to only what I needed short of modifying the playbook directly.

Pinging @marcusburghardt. I am not sure to what position we got though - perhaps we have the minimal set of packages covered by dnf, so the Ansible way makes sense as it is for developers who want more rounded environment, so no modifications are actually needed? Or would it still make sense to have a multi-tier package installation option?

This role was designed in "modular" view. Currently, it is possible to enable/disable some tasks in the Playbook. e.g.:

  vars:
    - available_tasks:
      - { enabled: True, name: 'install_packages' }
      - { enabled: True, name: 'configure_git' }
      - { enabled: True, name: 'configure_env' }
      - { enabled: False, name: 'populate_env' }
      - { enabled: False, name: 'configure_vscode' }
      - { enabled: False, name: 'configure_labs' }
      - { enabled: True, name: 'install_python_modules' }

But there is not a segmentation of packages, which I personally think that would be a cool improvement, not only for packages but maybe also for python modules. And the good news is that this is relatively easy to achieve with some updates in this Ansible role.

Anyway, I feel that we need to update some documentation, but bindep may be an overkill. Would you agree with this?

@openshift-merge-robot openshift-merge-robot added the needs-rebase Used by openshift-ci bot. label Apr 27, 2023
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jan-cerny jan-cerny removed this from the 0.1.68 milestone May 29, 2023
@jan-cerny jan-cerny added this to the 0.1.69 milestone May 29, 2023
@rhmdnd
Copy link
Collaborator Author

rhmdnd commented Jun 6, 2023

But there is not a segmentation of packages, which I personally think that would be a cool improvement, not only for packages but maybe also for python modules. And the good news is that this is relatively easy to achieve with some updates in this Ansible role.

It sounds like the Ansible approach would need an update to add support for installing package dependencies then. For the python packages, are you planning to point Ansible's pip module to the python requirements file? I'm curious because I'd like to understand if we can still manage the python dependencies in a single place.

@rhmdnd
Copy link
Collaborator Author

rhmdnd commented Jun 6, 2023

Anyway, I feel that we need to update some documentation, but bindep may be an overkill. Would you agree with this?

I'm indifferent to the tool that installs, or updates, the binary dependencies required to use the project. I proposed bindep since it was created to solve this specific problem, but we could certainly use something else.

I think the important part is having an abstraction, be it Anisble, bindep, or something else, in CI that allows us to install the dependencies, while simplifying the installation path for users. Otherwise, I envision it will only be a matter of time before a package name changes, a dependency is removed, etc, and the documentation will be out-of-sync with what the project actually requires.

@Mab879 Mab879 removed this from the 0.1.69 milestone Jul 14, 2023
@Mab879
Copy link
Member

Mab879 commented Jul 14, 2023

@matejak @rhmdnd Is something we want to continue to explore or should try something else?

@matejak
Copy link
Member

matejak commented Aug 30, 2023

So now, we use invocations of the package manager + pip in our tests. We don't even use Ansible, because the list of dependencies is small and doesn't really change. This makes the added value of bindep very low - whoever wants to, may simply copy-paste the basic and pretty stable list of non-Python packages from tests, and install the rest (which is more likely to change) using pip and the convenient interface of the requirements file that provides those dependency management capabilities where it is needed.
So I propose to close this PR, as the problem that it tries to solve is relatively small, and it is addressed by pip + requirements file, which is some sort of abstraction.

@rhmdnd rhmdnd closed this Aug 30, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Update in project documentation. Highlight This PR/Issue should make it to the featured changelog. needs-rebase Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants