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

Packaging: expand DEBHELPER in deb maintainer scripts and macros in rpm scriptlets #6319

Merged
merged 13 commits into from
Mar 19, 2025

Conversation

cognifloyd
Copy link
Member

This PR is working towards doing packaging via pantsbuild. Eventually, I hope to archive and stop using st2-packages.git.

When building system packages with native tooling, both deb maintainer scripts and rpm scriptlets get expanded.

  • For deb, the #DEBHELPER# marker in maintainer scripts gets replaced with "debhelper snippets" from things registered in deb rules and other control files.
  • For rpm, macros (which start with %) get expanded during rpm build so that all macros have been expanded/replaced in the scripts that actually get embedded in the built rpm.

When using pants + nfpm to build the deb/rpm packages, the build process is simplified so no expansion occurs on the scripts/scriptlets that get embedded in the package. Therefore, we need to expand these scripts ourselves.

Almost all of the debhelper snippets and rpm macros we rely on have to do with managing the systemd services. This PR focuses on expanding and then refactoring to simplify those scripts. An example of that refactoring is that I use a var _ST2_SERVICES and then run the systemd commands once--or in a loop--instead of repeating the command for every service (which is what the native expansion does).

At first, I extracted the expanded scripts from st2 deb/rpm files generated with native tooling. Then I used that to find the sources for the debhelper snippets and rpm macros we actually use. I tried to include comments linking to those upstream sources so that we can consult them in the future as we upgrade to newer versions of each OS. Debian and EL provide unique systemd management utils so the logic used to run them is necessarily a little different between Debian and EL (and sometimes between releases of the same OS).

NOTE: Beyond the systemd debhelper snippets and rpm macros, we also rely on some debhelper snippets and rpm macros to manage building the /opt/stackstorm/st2 virtualenv. I am NOT expanding those in this PR. Instead--in a follow-up PR--I will make the post-install scripts run the pex (added in #6307) to extract the virtualenv. Using the pex avoids the dh_virtualenv debhelper that is unsupported upstream, and skips the complexity of using the external tool we used in rpms. This way, we'll use the same process to generate the venv for both deb and rpm.

The commits add and then refactor this expanded code. So, reviewing each commit would probably be more annoying than just reviewing the final result. I do, however, recommend reading the descriptions of the commits to get some more background on why I've done some of the changes.

@cognifloyd cognifloyd added this to the pants milestone Mar 18, 2025
@cognifloyd cognifloyd self-assigned this Mar 18, 2025
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Mar 18, 2025
I initially copied the logic from some of the rpm's generated with
native deb tooling. I wanted to link to the actual sources, however,
to facilitate comparing the logic for future updates.
I initially copied the logic from some of the rpm's generated with
native rpm tooling. I wanted to link to the actual sources, however,
to facilitate comparing the logic for future updates.

st2.spec used %service_* macros which were defined in helper.spec
which used %systemd_* macros. Now, I've linked to the actual
systemd macro sources and cleanly combined the logic for EL8+9.
@cognifloyd cognifloyd force-pushed the packaging-scriptlets_expand branch from 438c4d8 to fae9319 Compare March 18, 2025 17:26
Comment on lines -9 to -11
PACKS_GROUP=%{packs_group}
SYS_USER=%{stanley_user}
ST2_USER=%{svc_user}
Copy link
Member Author

Choose a reason for hiding this comment

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

These macros were defined in st2.spec: https://github.com/StackStorm/st2-packages/blob/9974d04084b01cde9d11994f02abf46290c37fe3/packages/st2/rpm/st2.spec#L3-L5

The equivalent hard-coded values were already embedded in the deb maintainer scripts, so, this does not change anything. It only inlines the values for rpm.

@cognifloyd cognifloyd requested a review from a team March 18, 2025 17:35
Comment on lines +39 to +40
# TODO: Maybe remove this as 'preset' (on install above) enables units by default
systemctl --no-reload enable ${_ST2_SERVICES} &>/dev/null || :
Copy link
Member Author

@cognifloyd cognifloyd Mar 18, 2025

Choose a reason for hiding this comment

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

I debated removing this now. I left a TODO for now. I think we should wait to remove it until we can actually test the built rpms.

This does not come from upstream. We defined it in st2-packages.git: https://github.com/StackStorm/st2-packages/blob/6c81c2e8e1c1e0f0ec0132c74c1f67730f6b9bf8/rpmspec/helpers.spec#L84-L90

@cognifloyd
Copy link
Member Author

I just pushed another commit that enables shellcheck for these sources and fixes several issues identified by shellcheck (including one place where I misspelled systemd). Almost all of the changed lines were added in this PR, so it made sense to include that here.

@cognifloyd cognifloyd enabled auto-merge March 19, 2025 21:45
@cognifloyd cognifloyd merged commit 6535ff5 into master Mar 19, 2025
123 of 127 checks passed
@cognifloyd cognifloyd deleted the packaging-scriptlets_expand branch March 19, 2025 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pantsbuild refactor size/L PR that changes 100-499 lines. Requires some effort to review. st2-packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants