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

Standalone linux packages #878

Merged
merged 8 commits into from
Apr 22, 2024
Merged

Conversation

agateau-gg
Copy link
Collaborator

@agateau-gg agateau-gg commented Apr 17, 2024

Context

This PR does the following:

  • Removes the zipapp (.pyz file): stats shows its never used. Using the standalone binaries is more interesting: the zipapp bundles ggshield dependencies but still requires Python. The standalone binaries do not require Python.
  • Changes the way we build .deb and .rpm packages: instead of packaging the zipapp, they now package the standalone binary.

What has been done

Quite a lot of refactoring happened:

  • scripts/build-packages has been removed (it was responsible for building the .whl, the source .tar.gz, the zipapp, the .deb and the .rpm).
  • the .whl and the source .tar.gz are now built by the CI without going through a script (because it boils down to pip install build ; python -m build).
  • scripts/build-standalone-exe has been renamed/moved to scripts/build-os-packages/build-os-packages and now builds the .deb and .rpm.
  • The build_release_assets.yml CI workflow was previously only used by the tag.yml workflow. It is now also used by the ci.yml workflow. This reduces code duplication.

I also extended the doc on this topic a bit. doc/dev/os-packages.md now contains a flowchart of build-os-packages steps.

Testing

Since build_release_assets.yml is now used by ci.yml, it is executed by this PR. Neverthless, I temporarily added a faketag.yml workflow to test the changes from tag.yml. It's a copy of tag.yml with all the publishing steps replacing by code listing the expected assets to publish. I will remove the commit adding that file before merging (This is similar to what I did in #871).

Review

Best reviewed commit by commit.

@agateau-gg agateau-gg force-pushed the agateau/standalone-linux-packages branch 13 times, most recently from cc489f8 to 12d3df6 Compare April 18, 2024 13:54
ggshield-test added 3 commits April 18, 2024 16:15
Deprecate build-packages:
- the .deb and .rpm part is going to move to build-standalone-exe
- the .whl and .sdist part is now done by the CI itself (no need for a
  wrapper script: the command is simple enough)
Rename build-standalone-exe to build-os-packages because the script is
going to take care of building the .deb and .rpm, replacing build-packages.

Other changes in the script:
- Its output directory is now `packages`
- It has its own directory inside `scripts`, to hold the files it is going
  to need (only the README template for now)
Move code to build .deb and .rpm to build-os-packages. Remove
build-packages script.
@agateau-gg agateau-gg force-pushed the agateau/standalone-linux-packages branch from 12d3df6 to ee87ab0 Compare April 18, 2024 14:17
@agateau-gg agateau-gg force-pushed the agateau/standalone-linux-packages branch from ee87ab0 to 14f790e Compare April 18, 2024 14:20
@agateau-gg agateau-gg marked this pull request as ready for review April 18, 2024 14:37
@agateau-gg agateau-gg changed the title agateau/standalone linux packages Standalone linux packages Apr 19, 2024
Copy link
Contributor

@fnareoh fnareoh left a comment

Choose a reason for hiding this comment

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

Looks good, I'm just curious about one thing: our package is named "os-ubuntu-22.04" but it can most likely be installed on all versions of ubuntu ? could the naming discourage users ?

scripts/build-os-packages/nfpm.yaml Outdated Show resolved Hide resolved
@fnareoh fnareoh self-requested a review April 19, 2024 12:32
@agateau-gg
Copy link
Collaborator Author

Looks good, I'm just curious about one thing: our package is named "os-ubuntu-22.04" but it can most likely be installed on all versions of ubuntu ? could the naming discourage users ?

This name is internal: it's generated by GitHub when it combines the job name and the matrix values. If you download it from the run summary page you can see it contains the following files:

  • ggshield-1.26.0-1.x86_64.rpm
  • ggshield-1.26.0-x86_64-unknown-linux-gnu.tar.gz
  • ggshield_1.26.0-1_amd64.deb

The deb file does not mention any Ubuntu version. I can confirm it works on Ubuntu 22.04 and Debian 12 (It does not work on older versions though, because it needs a glibc >= the-one-in-ubuntu-22.04. We would need to build it on an older distribution to fix this)

@agateau-gg agateau-gg force-pushed the agateau/standalone-linux-packages branch from 14f790e to 5e91511 Compare April 19, 2024 12:56
Copy link
Collaborator

@salome-voltz salome-voltz left a comment

Choose a reason for hiding this comment

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

Looks good 👍

scripts/build-os-packages/build-os-packages Outdated Show resolved Hide resolved
scripts/build-os-packages/nfpm.yaml Show resolved Hide resolved
@pierrelalanne
Copy link
Collaborator

Hi @agateau-gg: I see the last commit introduces a faketag CI workflow. Is this intentional, how is it supposed to be used ?

ggshield-test added 4 commits April 19, 2024 16:42
- define section
- provide a short first line for description, and 80-column-wrapped content
  for the extended-description field
- make sure files are not group-writable
- fix absolute symbolic link
- add dependency on libc6
Make ci.yml use build_release_assets.yml too. ci.yml was basically
embedding a copy of build_release_assets.yml. This commit removes the copy
and makes ci.yml call build_release_assets.yml.

The only change in build_release_assets.yml is the addition of a
"release_mode" input option, so that ci.yml produces unsigned packages with
commit-sha in their version numbers, whereas tag.yml produces signed
packages without commit-sha in version numbers.
@agateau-gg agateau-gg force-pushed the agateau/standalone-linux-packages branch from 5e91511 to f5e4d88 Compare April 19, 2024 14:43
@agateau-gg
Copy link
Collaborator Author

Hi @agateau-gg: I see the last commit introduces a faketag CI workflow. Is this intentional, how is it supposed to be used ?

@pierrelalanne this workflow is there for testing. I am going to remove it before merging.

You can see a run of it here

See the "Testing" section in the PR description for more details.

@agateau-gg agateau-gg force-pushed the agateau/standalone-linux-packages branch from f5e4d88 to 5e785a5 Compare April 22, 2024 14:20
@agateau-gg agateau-gg merged commit 054d4ce into main Apr 22, 2024
26 checks passed
@agateau-gg agateau-gg deleted the agateau/standalone-linux-packages branch April 22, 2024 14:29
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.

4 participants