-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Conversation
a9a99d3
to
8a4680c
Compare
There was a problem hiding this 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.
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 |
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 inCMakeLists.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:
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).