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 missing dependency #1043

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Add missing dependency #1043

merged 3 commits into from
Jan 8, 2025

Conversation

agateau-gg
Copy link
Collaborator

@agateau-gg agateau-gg commented Jan 7, 2025

Context

The Python interpreter we bundle in our RPM package depends on libcrypt.so.1, but some distributions do not ship it anymore. This is why ggshield fails when it's installed using its RPM package on Red Hat Enterprise Linux 9 (See #1036).

What has been done

  • Update nfpm configuration to bundle the version of libcrypt we depend on.
  • Update CI to test the RPM on RockyLinux 9 (more-or-less equivalent to RHEL 9).
  • Build the RPM using rockylinux:8 instead of rockylinux:8.8 because the 8.8 Docker image does not receive security upgrades (we still test with 8.8 to ensure it works)

Validation

  • Start a rockylinux 9 container
  • Install git
  • Install ggshield from the RPM
  • Run ggshield --version → with the released version it fails, with the version from the CI it works

PR check list

  • As much as possible, the changes include tests (unit and/or functional)
  • If the changes affect the end user (new feature, behavior change, bug fix) then the PR has a changelog entry (see doc/dev/getting-started.md). If the changes do not affect the end user, then the skip-changelog label has been added to the PR.

@agateau-gg agateau-gg requested a review from a team as a code owner January 7, 2025 11:35
@agateau-gg agateau-gg marked this pull request as draft January 7, 2025 11:36
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.09%. Comparing base (82a2486) to head (7f68d32).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1043   +/-   ##
=======================================
  Coverage   92.09%   92.09%           
=======================================
  Files         181      181           
  Lines        7756     7756           
=======================================
  Hits         7143     7143           
  Misses        613      613           
Flag Coverage Δ
unittests 92.09% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Make the package bundle a copy of libcrypt.so.1 because our bundled Python
still uses this version, but some distributions now ship libcrypt.so.2.
Our RPM only depends on git-core, so install only that.
@agateau-gg agateau-gg force-pushed the agateau/add-missing-dependency branch from 6af82c5 to 5d39600 Compare January 7, 2025 15:16
@agateau-gg agateau-gg force-pushed the agateau/add-missing-dependency branch from 7db2dd0 to 7f68d32 Compare January 7, 2025 15:59
@agateau-gg agateau-gg marked this pull request as ready for review January 7, 2025 16:31
strategy:
fail-fast: false
matrix:
include:
- os: ubuntu-22.04
# When building on Linux we use a container to build using an old enough version
container: rockylinux/rockylinux:8
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I recall correctly, we initially fixed the minor version of rockylinux to 8.8 for python version consideration. Maybe it would be safer to pin a version here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is the 8.8 images do not receive security upgrades (See Rocky Linux Docker page), so it's less work for the CI to start from the latest 8 to build. The smoke tests run at the end still use 8.8 so it can't break without us noticing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All right, all good then, thanks for the clarification !

@salome-voltz salome-voltz merged commit dcd87b4 into main Jan 8, 2025
34 checks passed
@salome-voltz salome-voltz deleted the agateau/add-missing-dependency branch January 8, 2025 09:11
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