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

feat: adding makefile to install first pre-commit and then venv #29

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

boekhorstb1
Copy link
Contributor

  • adding makefile with pre-commit installation first and then venv installation

@boekhorstb1 boekhorstb1 requested a review from brueggemann March 7, 2024 12:31
@boekhorstb1 boekhorstb1 self-assigned this Mar 7, 2024
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-makefile branch 2 times, most recently from 3b35c50 to a13c13e Compare March 7, 2024 12:39
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-makefile branch 3 times, most recently from be90a2e to b713898 Compare March 7, 2024 12:49
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-makefile branch from b713898 to 9fb8b51 Compare March 7, 2024 14:36
Makefile Outdated Show resolved Hide resolved
@boekhorstb1 boekhorstb1 force-pushed the feat/adding-makefile branch from 9fb8b51 to 0cd9ec5 Compare March 7, 2024 15:10
@boekhorstb1 boekhorstb1 requested a review from brueggemann March 7, 2024 15:11
@brueggemann
Copy link
Contributor

brueggemann commented Mar 7, 2024

I did a little bit research. So I think the best solution for the rados library is:

  1. We should remove rados from requirements.txt, as this cannot be installed by pip
  2. We alter the setup-venv target in the Makefile to:
    i. Use python -m venv --system-site-packages ./.venv to create the venv
    ii. Do the pip install with the following command: pip install --ignore-installed -r requirements.txt
  3. We do pip freeze -l > requirements.txt to update the requirements.txt with only the local packages
  4. We add some CI/CD job to check if all requirements of the projects are respected in the requirements.txt (This should be another PR)

For explanation:
the problem with --system-site-packages is, that as the commandline switch says, it includes the system site-packages to our venv. This can be a problem, because we probably don't have the package versions installed, that are defined in requirements.txt. But this should be compensated with the pip install --ignore-installed command. This ignores the already installed packages, so pip will install the requirements as defined in requirements.txt into the venv. Python will look for local packages first, before it looks in system directories.
Another problem of --system-site-packages is, that in pip freeze the system site-packages are listed as well as the locally installed packages. so pip freeze -l mitigates this by only listing the local installed packages.

The only problem this cannot resolve is, that a developer could import a package, that is installed as system package on the development machine and so, don't notice that the requirements.txt has to be updated. But this is a problem we can resolve by checking in a CI/CD pipeline

… it checks if rados is installed locally in the correct way

Signed-off-by: R.A. te Boekhorst <[email protected]>
@brueggemann brueggemann merged commit 2d39fb7 into main Mar 14, 2024
2 checks passed
@brueggemann brueggemann deleted the feat/adding-makefile branch March 14, 2024 11:50
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