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

Clean up CMake packaging code in preparation for migrating RPM packaging to CPack. #17590

Closed
wants to merge 8 commits into from

Conversation

Ferroin
Copy link
Member

@Ferroin Ferroin commented May 3, 2024

Summary

This PR can largely be described as two related but mostly distinct sets of changes.

The first change is a mass rename and restructure of packaging/cmake/control. This covers all the changes in CMakeLists.txt and all the renames, and is needed to simplify differentiation of what package type each file is involved with (since the RPM packages will need completely different pre/post install/remove scripts, among other things). I expect this part to be non-controversial, but it’s included as part of this PR simply because it is a hard requirement for a few parts of the other set of changes and it is logically part of the same cleanup.

The second change is a conversion of the Packaging module from a long series of variable defines to a custom function used to declare each package. The idea here is to decouple the large amount of boilerplate code involved in defining each package component from the actual information involved in defining each component.

A significant percentage of the work needed for adding RPM packages involves defining an almost identical set of variables to those used for DEB packages, with only trivial differences between the values. CPack technically provides some handling for this, but it’s woefully insufficient for our actual needs, and it does not cover large parts of what will otherwise be nearly duplicate code that could be programmatically handled instead.

Because of this, the shift to using a custom function to declare each package will significantly simplify the changes needed to add RPM support, and should also significantly reduce the long-term maintenance burden for the packaging code, as well as making it much easier to add a new package and have it just work for both DEB and RPM packages.

In addition to these two primary changes, this PR also does some cleanup of the dependencies for our native packages:

  • CUPS and FreeIPMI now rely on automatic dependency generation to only pull in what they actually need instead of pulling in the corresponding metapackages. This has essentially zero impact on their ability to collect data, but should significantly shrink their dependency graphs.
  • A few fundamentally implicit dependencies on the main package have been removed (we don’t provide packages anymore for systems old enough for the requirement of a specific version of dpkg, libcap2-bin, or lsb-base to matter, dpkg is an implicit dependency so if no specific version is needed it can be dropped, and lsb-base is only needed for the LSB-style init scripts and can be safely assumed to be present if a user is using sysvinit).
  • Hard version dependencies between the individual components have been relaxed. We don’t technically need these strict version requirements, and not having them makes updates, upgrades, and similar work much more reliably.
  • The Python plugin now correctly has a hard dependency on Python instead of just recommending it (it will not run without Python, so this is a hard dependency, like it or not).
Test Plan

Preliminary testing involves checking that the set of packages that gets built is the same before and after this PR. Supplementary checking is required to confirm that the package control files are the same (barring the dependency changes mentioned above).

@github-actions github-actions bot added area/packaging Packaging and operating systems support area/build Build system (autotools and cmake). labels May 3, 2024
@Ferroin Ferroin marked this pull request as ready for review May 6, 2024 17:16
@Ferroin Ferroin requested review from vkalintiris and a team as code owners May 6, 2024 17:16
Copy link
Contributor

@vkalintiris vkalintiris left a comment

Choose a reason for hiding this comment

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

Simply, no. The rest of the team should be able to understand the cmake code and make changes easily.

@Ferroin
Copy link
Member Author

Ferroin commented May 16, 2024

Simply, no. The rest of the team should be able to understand the cmake code and make changes easily.

I contend that the current code does not meet that criteria either, and is also extremely error-prone to maintain even for people who do know what they are doing because of the massive amount of repetition involved. This will be more relevant once we have RPM packaging support, because numerous things do need to be kept in sync between the DEB/RPM packages, but there are no shared variables for declaring most of those things, so any change to them will require remembering to change things in multiple places if we don’t consolidate stuff in some way.

Irrespective of the shift to a function or not for package declarations, the mass rename part needs to happen in some form if we’re going to keep things well organized (none of the ‘control’ files other than the copyright file can be shared between the DEB and RPM packages, and control is not sufficiently descriptive for our purposes here), and at least some of the dependency changes need to happen as well. I’ll split those out to separate PRs for now, and we can revisit the rest of this later.

@Ferroin Ferroin closed this May 16, 2024
@Ferroin Ferroin deleted the cmake-packaging-cleanup branch May 16, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build Build system (autotools and cmake). area/packaging Packaging and operating systems support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants