CPack RPM packaging #3601

robaerd · 2020-12-20T19:13:23Z

Basics

These points need to be fulfilled for every PR:

Short descriptions of your changes are in the release notes
(added as entry in doc/news/_preparation_next_release.md which
contains _(my name)_)
Please always add something to the release notes.
Details of what you changed are in commit messages
(first line should have module: short statement syntax)
References to issues, e.g. close #X, are in the commit messages.
The buildservers are happy. If not, fix in this order:
- add a line in doc/news/_preparation_next_release.md
- reformat the code with scripts/dev/reformat-all
- make all unit tests pass
- fix all memleaks
The PR is rebased with current master.

If you have any troubles fulfilling these criteria, please write
about the trouble as comment in the PR. We will help you.
But we cannot accept PRs that do not fulfill the basics.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

I added unit tests for my code
I fully described what my PR does in the documentation
(not in the PR description)
I fixed all affected documentation
I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
I updated all meta data (e.g. README.md of plugins and METADATA.ini)
I mentioned every code not directly written by me in THIRD-PARTY-LICENSES

Review

Reviewers will usually check the following:

Documentation is introductory, concise, good to read and describes everything what the PR does
Examples are well chosen and understandable
Code is conforming to our Coding Guidelines
APIs are conforming to our Design Guidelines
Code is consistent to our Design Decisions

Labels

If you are already Elektra developer:

Add the "work in progress" label if you do not want the PR to be reviewed yet.
Add the "ready to merge" label if the basics are fulfilled and you also
say that everything is ready to be merged.

robaerd · 2020-12-20T19:13:48Z

Limitations that couldn't be solved:

CPack RPM raises a few warnings that CPACK_RPM_PACKAGE_RELOCATABLE will be deactivated because CPACK_SET_DESTDIR is set. CPACK_RPM_PACKAGE_RELOCATABLE is explicitly unset to deactivate this, but apparently CPack still handles it, as it would be set. I tried setting it to "OFF", "FALSE", unseting, caching it etc, but none of this worked. I think this could be a bug with CPack RPM itself. But since this only yields warnings and doesn't affect any functinality this shouldn't be a problem.
The default path of the source of debuginfo packages is /usr/src/debug/. To be able to place the source in this directory, the Source dir must be on a path that is longer than the path where the debuginfo source is installed. E.g.: debuginfo src of package libelektra4 is installed at /usr/src/debug/Elektra-0.9.3-Fedora33-amd64/libelektra4/src_0/ so the source dir must have at least 6 parent dirs e.g. /folder/folder/folder/folder/folder/folder/libelektra. But this shouldn't be a problem since we can simply copy the libelektra dir to another folder in the CI.

markus2330 · 2020-12-21T08:45:50Z

I think this could be a bug with CPack RPM itself. But since this only yields warnings and doesn't affect any functinality this shouldn't be a problem.

Can you do a bug report about these warnings we receive (also the ones in Debian). Maybe the CMake devs know some easy way to fix it or at least they'll know about the problem and can fix it in the future.

But this shouldn't be a problem since we can simply copy the libelektra dir to another folder in the CI.

I do not really understand the problem. Can someone who wants to build the RPM packages on his machine trigger this problem? If yes, we should write about it in doc/INSTALL.md. In general we should update doc/INSTALL.md as from now on, creating packages via cpack will be the favorable option, and make install should be only used if you are on a system where cpack support is not (yet) available.

robaerd · 2020-12-21T14:31:08Z

Can you do a bug report about these warnings we receive (also the ones in Debian).

Yes I will do this.

Can someone who wants to build the RPM packages on his machine trigger this problem?

Yes, unless someone sets CPACK_RPM_BUILD_SOURCE_DIRS_PREFIX to a smaller path, they will need to place the libelektra source to a longer path (with 6 parent directories) and build the package from there. I will update doc/INSTALL.md and document these things.

robaerd · 2020-12-22T19:30:43Z

Since make package will become the standard way of installing Elektra we will need to explicitly disable the generation of individual packages depending on what plugins/bindings are included. If someone decides to build a package with less plugins/bindings than expected by each package, the packaging will fail because of the debuginfo stripping of the CPack generators.

If will add this right to this PR if the approach is OK for you @markus2330.

markus2330 · 2020-12-23T08:42:17Z

Can you give more details about the approach? How do you want to disable packages?

What is important for CMake usability is to have as little CACHE variables as possible, as with these variables every maintainer gets confronted. So I would not like a solution where we would need to introduce a CACHE variable for every package. Two ways that sound fine to me would be:

In the case of make package we simply require more dependencies to be installed so that we can produce useful modular packages. If someone wants to build Elektra without these dependencies, she needs to use make install. This is more or less a usability&documentation task: we somehow need to avoid that people get confused with the output of the "debuginfo stripping" problem and tell them they need to install the deps instead.
We can somehow fix the "debuginfo stripping" problem and produce empty packages or automatically exclude packages if the whole content of a package was not built.

robaerd · 2020-12-23T13:18:50Z

I just testet the case for RPM and DEB generators. The build does not fail for RPM packages because the RPM generator detects that there is no source (except for /usr/share/) present and disables the debuginfo generation.
The DEB generator does really weird things, it doesn't throw any errors, but instead shuffles around the dependencies of the dbgsym packges. For example: if qt-gui is disabled, the qt-gui-dbgsym package is not built, but the elektra-bin-dbgsym packages gets the control file of the qt-gui-dbgsym package (with the dependencies to qt-gui and the package name qt-gui-dbgsym).

My idea was to check the ADDED_PLUGINS, ADDED_BINDINGS and ADDED_TOOLS variables to determine which of the plugins, bindings and tools are not included. Especially for the components/packages where there is a one-to-one relationship to plugins etc.
My initial approach to avoid wrapping everything in conditionals was to modify CPACK_COMPONENTS_ALL after the include (CPack) command, but it seems that this must happen before. So now, we would have to check for each of the packages (at least for those with a one-to-one relationship to a plugin/binding/tool) if the plugin etc. is present and if it is not present disable the debuginfo package generation.

This way no CACHE variables would need to be set.

markus2330 · 2020-12-23T15:40:01Z

does not fail for RPM packages [...] The DEB generator does really weird things

Maybe this is a bug in CMake and what we do is actually correct and should already work?

determine which of the plugins, bindings and tools are not included; [...] This way no CACHE variables would need to be set.

Yes, this proposal is perfect from the user's point of view!

ADDED_PLUGINS, ADDED_BINDINGS and ADDED_TOOLS

Maybe it is better to track which components are added (e.g. within add_plugin) and then simply exclude the component which were never added? But if it is less code the other way round, feel free to ignore this suggestion.

It would be good to print which components were added or excluded.

robaerd · 2020-12-25T16:16:07Z

Maybe this is a bug in CMake and what we do is actually correct and should already work?

I'm not 100% sure about this, since they state in their docu that binaries must contain debug symbols before packaging. But I will include this in the bug report, because the RPM generator actually handles this case.

Maybe it is better to track which components are added (e.g. within add_plugin) and then simply exclude the component which were never added?

This way we would not have the option to yield a message that a part of the expected plugins of a package is missing. For example libelektra4-zeromq depends on the plugins zeromqrecv and zeromqsend. But if this isn't a problem for you, then I can also rewrite it like you suggested.

markus2330 · 2020-12-27T06:23:00Z

No need to rewrite if there are no problems, let us see how the current implementation works.

markus2330

Looks good! Can you also update doc/INSTALL.md how to install the Ubuntu packages produced by our build server. At the moment only buster is explained (with a wrong heading, where it says "For Stretch.") and the outdated Jessie (does it even work? I think we can remove it now).

doc/INSTALL.md

robaerd · 2020-12-31T00:52:55Z

Can you also update doc/INSTALL.md how to install the Ubuntu packages produced by our build server.

Yes of course.

Since the update of the release-pipeline depends on the changes of this PR (and also fixes a few things), should I add all these changes (including the update of the release-pipeline) to this PR or create a separate one after this one is merged?

markus2330 · 2020-12-31T06:23:22Z

Thank you! We should also start offering both released packages and packages build from master. Do we have all of them (Debian+Ubuntu) already in repositories?

It is up to you how you prefer to split up PRs. Smaller is usually better. The docu should only reflect what is, not what it might be with upcoming PRs.

robaerd · 2020-12-31T11:58:25Z

None of the released packages are currently offered in our repositories. I will make myself familiar with the reprepro tool and add this functionality to the release pipeline.

markus2330 · 2021-01-02T06:57:26Z

Thank you! Do we already have Ubuntu Focal debs build from master? The docu does not mention it at all even though #3522 is resolved.

robaerd · 2021-01-02T12:12:51Z

Oh I think I misunderstood #3522 and thought that Ubuntu Focal packages should only be built for releases. I will add this to the upcoming PR with the updated release pipeline.

markus2330 · 2021-01-02T13:17:02Z

You are right, in #3522 nothing was mentioned about master builds. I think, however, it would be the easiest for the user if we build the same set of packages for both master and releases.

Specifically, for Ubuntu Focal we need packages once #3608 is merged. It would be possible to do a release afterwards. So if it is a lot of effort to do master packages, we can consider to provide only release packages and make some pre-releases so that we have the ability to create packages when needed. Or you find a way to share code between master and release packaging (or even creating packages for branches), this would be best from my point of view. What do you think?

robaerd · 2021-01-02T13:48:25Z

I don't think it is a lot of effort to also provide them in the main pipeline, since I also have to create the repositories for the released packages.

Should we then also include the stage where the built packages are installed and tested in a docker container? This is currently done in the updated release-pipeline, but IMO it wouldn't make much sense to include it in the main pipeline until #2856 is not fixed.

Since we also create Fedora packages and the reprepro tool doesn't support RPM, should these packages also be created in the main pipeline, or should we wait until we also have a repository for RPM packages?

Or you find a way to share code between master and release packaging

I planned to refactor the shared code of the main and release Jenkinsfile into a shared groovy library, since I already introduced a lot of duplicate code. Depending on when the release will happen, I'm not sure if I manage to get this done for the current one.

... (or even creating packages for branches)

So for Pull Requests?

markus2330 · 2021-01-02T15:18:39Z

Should we then also include the stage where the built packages are installed and tested in a docker container?

As we want the possibility of problems during release to be kept at a minimum, such tests ideally are in the main pipeline.

This is currently done in the updated release-pipeline, but IMO it wouldn't make much sense to include it in the main pipeline until #2856 is not fixed.

Then let us fix #2856 by excluding these tests from the installation.

Since we also create Fedora packages and the reprepro tool doesn't support RPM, should these packages also be created in the main pipeline, or should we wait until we also have a repository for RPM packages?

Is there maybe some tool that supports several distributions and package formats? reprepro is some legacy tool I used to use many years ago. There is no special reason to keep it.

I planned to refactor the shared code of the main and release Jenkinsfile into a shared groovy library, since I already introduced a lot of duplicate code.

That would be amazing.

Depending on when the release will happen, I'm not sure if I manage to get this done for the current one.

In any case it is good that you always create PRs whenever you have something that is working.

So for Pull Requests?

To have this would be the best. But consider it as strictly nice-to-have. Afaik I am the only one sometimes building packages from branches. With cpack it is not a big deal to build packages for yourself anyway.

robaerd · 2021-01-02T17:19:14Z

Is there maybe some tool that supports several distributions and package formats?

I only could find Pulp as a tool that supports both, but it seems to be really hard to setup and to integrate it into our current setup. aptly is another well used tool for debian package management which appears to be much easier to use than reprepro and they have RPM repositories on their roadmap. The go-to tool for RPM repositories is createrepo and it seems very easy to use. Maybe switching to aptly would be a good idea but for now I think using reprepro and createrepo would be best.

robaerd · 2021-01-11T11:49:10Z

Can I set the ready to merge label, or is there still something open? All the release and package repository related things were done in #3623.

Caching the revision number makes package revisions possible without changing any CMake files

mpranj · 2021-01-11T14:32:16Z

When you finish your work you can always set the ready to merge label. This signals that the PR is ready for review and can be merged if everything is fine. If the reviewer requests changes the label is removed and you re-add the label when you've implemented the changes.

The label just avoids premature reviews and also gets PRs merged faster (visibility).

robaerd · 2021-01-11T16:29:46Z

Oh then I misunderstood it, thanks!

mpranj

Thank you so much! I'm looking forward to testing the Fedora packages.

doc/INSTALL.md

markus2330

Great work! (Partly reviewed)

markus2330 · 2021-01-12T08:38:00Z

doc/INSTALL.md

@@ -111,7 +116,55 @@ Please refer to the section OS Independent below.

 First follow the steps in [COMPILE](COMPILE.md).

-After you completed building Elektra on your own, there are multiple options how to install it. For example, with make or cPack tools.
+After you completed building Elektra on your own, there are multiple options how to install it. For example, with make or CPack tools.
+We recommend that you generate your own packages with CPack so ensure compatibility with future releases.


Suggested change

We recommend that you generate your own packages with CPack so ensure compatibility with future releases.

We recommend to use the packages from our build server or that you generate your own packages with CPack.

markus2330 · 2021-01-12T08:39:09Z

doc/INSTALL.md

+To install the packages run this in the `package` directory:
+
+```sh
+dpkg -i *


With apt you can install directly with also installing dependencies.

markus2330 · 2021-01-12T08:39:53Z

doc/INSTALL.md

@@ -134,20 +187,6 @@ or in the build directory (will not honor `DESTDIR`!):
 xargs rm < install_manifest.txt
 ```

-### CPack
-
-First follow the steps in [COMPILE](COMPILE.md).


Maybe we should keep this info somewhere?

It is already present in the parent heading ## OS Independent (not visible in the git diff). Is it really necessary to write this multiple times?

Yes, you are right, I did not see it.

markus2330 · 2021-01-12T08:40:31Z

scripts/cmake/ElektraCache.cmake

+#
+
+set (
+	CPACK_DEBIAN_PACKAGE_RELEASE


Why not only one variable CPACK_PACKAGE_RELEASE?

CPACK_DEBIAN_PACKAGE_RELEASE and CPACK_RPM_PACKAGE_RELEASE are both predefined variables of CPack, but I agree, caching only CPACK_PACKAGE_RELEASE and setting CPACK_DEBIAN_PACKAGE_RELEASE and CPACK_RPM_PACKAGE_RELEASE to CPACK_PACKAGE_RELEASE would be better.

If they are already predefined we do not need to set them as cache variable anyway?

I agree, but I would at least define CPACK_PACKAGE_RELEASE as cache variable with a default value. This is especially useful for our release pipeline where the package revision of each package can be changed with the parameterized build.

Cache variables are afaik only needed if you want it to be visible in the cmake GUIs. I do not think this is needed in this situation.

With something like:

if (CPACK_RPM_PACKAGE_RELEASE) set (CPACK_RPM_PACKAGE_RELEASE ...)

you should be able to set a default if the variable is not set from outside.

markus2330

Great job! Can imho be merged and the small things fixed later.

markus2330 · 2021-01-12T14:08:42Z

scripts/packaging/package.sh

+if [ $OS_NAME = "Fedora" ]; then
+
+	CMAKE_ARGS="-DTARGET_PLUGIN_FOLDER='elektra4' \
+    -DBUILD_STATIC=OFF \


Some args, like -DBUILD_STATIC=OFF, seem to be the same in both branches.

This is already fixed in #3623.

markus2330 · 2021-01-12T14:12:19Z

scripts/cmake/Modules/PackagingFedora.cmake

+
+# install copyright file
+configure_file ("${CMAKE_SOURCE_DIR}/LICENSE.md" "${CMAKE_BINARY_DIR}/doc/LICENSE" COPYONLY)
+configure_file ("${CMAKE_SOURCE_DIR}/doc/THIRD-PARTY-LICENSES" "${CMAKE_BINARY_DIR}/doc/THIRD-PARTY-LICENSES" COPYONLY)


Do we really need distribution-specific installation of files? Maybe put it together with execute_process, here it is a bit surprising.

Unfortunately yes. /usr/share/licenses/ is the default directory for licenses on Fedora based distributions and is not present on Debian based distributions. I will move it up like you suggested.

markus2330 · 2021-01-12T14:13:59Z

scripts/cmake/Modules/PackagingFedora.cmake

@@ -0,0 +1,199 @@
+set (CPACK_RPM_PACKAGE_VERSION "${PROJECT_VERSION}")


Give a short info what this file is about (that it sets CPACK_RPM_ vars and also installs some rpm specific files). Maybe also split this into two files (with separate headers)

markus2330 · 2021-01-12T14:15:54Z

scripts/cmake/ElektraPackaging.cmake

+# ADDITIONAL_DEPENDENCIES additional dependencies if the component
+# 				          has multiple dependencies
+# ~~~
+macro (check_component_dependencies dependency_name component_name)


Maybe you can put this to some other file, the files already get lengthy.

markus2330 · 2021-01-12T14:16:09Z

scripts/cmake/ElektraPackaging.cmake

+# ~~~
+# check_component_dependencies
+#
+# check and excludes a component from packaging if


nice docu 👍

mpranj · 2021-01-12T22:55:39Z

Thank you. 🚀

restyled-io bot mentioned this pull request Dec 20, 2020

Restyle CPack RPM packaging #3602

Closed

restyled-io bot mentioned this pull request Dec 25, 2020

Restyle CPack RPM packaging #3611

Closed

robaerd force-pushed the cpack-rpm-packaging branch from b4ca737 to 40b6410 Compare December 27, 2020 15:31

markus2330 reviewed Dec 30, 2020

View reviewed changes

doc/INSTALL.md Outdated Show resolved Hide resolved

restyled-io bot mentioned this pull request Dec 31, 2020

Restyle CPack RPM packaging #3616

Closed

robaerd mentioned this pull request Jan 8, 2021

Updated Release and Main Pipeline #3623

Merged

24 tasks

robaerd and others added 5 commits January 11, 2021 15:13

packaging: add RPM packaging

a1fa4bf

doc: add release notes

92c6c4a

packaging: fix missing last line

91320d9

Restyled by shfmt

e3e52a2

scripts: remove bashism

bdcf361

robaerd and others added 14 commits January 11, 2021 15:13

packaging: generate yamlcpp debuginfo package on debian

0db67f5

packaging: exclude components where deps are not fulfilled

66d3fad

packaging: disable debuginfo packages on Release build type

90c96e8

doc: update install.md for packaging

6f7e8a3

Restyled by prettier-markdown

1a867da

style: reformat cmake

75b3650

cmake: fix syntax error

f107879

packaging: only exclude libelektra4-all if dependent packages excluded

ddfbdfb

packaging: cache package revision number

621978e

Caching the revision number makes package revisions possible without changing any CMake files

doc: update package installation commands to resolve dependencies

2f0b58b

packaging: rename packaging output directory

c36bd93

doc: update ubuntu-bionic installation and remove debian-jessy

4737e05

Restyled by prettier-markdown

928eb37

doc: resolve merge problems release notes

708e311

robaerd force-pushed the cpack-rpm-packaging branch from 23a6941 to 708e311 Compare January 11, 2021 14:22

robaerd added the ready to merge label Jan 11, 2021

mpranj approved these changes Jan 12, 2021

View reviewed changes

doc/INSTALL.md Outdated Show resolved Hide resolved

Update doc/INSTALL.md

b62a658

markus2330 reviewed Jan 12, 2021

View reviewed changes

markus2330 approved these changes Jan 12, 2021

View reviewed changes

packaging: apply suggestions from review

c5e708a

mpranj merged commit 1d9c00e into ElektraInitiative:master Jan 12, 2021

robaerd mentioned this pull request Feb 1, 2021

0.9.4 release #3553

Closed

7 tasks

	We recommend that you generate your own packages with CPack so ensure compatibility with future releases.
	We recommend to use the packages from our build server or that you generate your own packages with CPack.

		@@ -0,0 +1,199 @@
		set (CPACK_RPM_PACKAGE_VERSION "${PROJECT_VERSION}")

CPack RPM packaging #3601

CPack RPM packaging #3601

Conversation

robaerd commented Dec 20, 2020

Basics

Checklist

Review

Labels

robaerd commented Dec 20, 2020

markus2330 commented Dec 21, 2020

robaerd commented Dec 21, 2020

robaerd commented Dec 22, 2020

markus2330 commented Dec 23, 2020

robaerd commented Dec 23, 2020

markus2330 commented Dec 23, 2020

robaerd commented Dec 25, 2020

markus2330 commented Dec 27, 2020

markus2330 left a comment

Choose a reason for hiding this comment

robaerd commented Dec 31, 2020

markus2330 commented Dec 31, 2020

robaerd commented Dec 31, 2020

markus2330 commented Jan 2, 2021

robaerd commented Jan 2, 2021

markus2330 commented Jan 2, 2021

robaerd commented Jan 2, 2021

markus2330 commented Jan 2, 2021

robaerd commented Jan 2, 2021

robaerd commented Jan 11, 2021 • edited Loading

mpranj commented Jan 11, 2021 • edited Loading

robaerd commented Jan 11, 2021

mpranj left a comment

Choose a reason for hiding this comment

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpranj commented Jan 12, 2021

robaerd commented Jan 11, 2021 •

edited

Loading

mpranj commented Jan 11, 2021 •

edited

Loading