Bug 8064 - Cannot build packages on Fedora 36
Summary: Cannot build packages on Fedora 36
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Build system (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.15.0
Assignee: Pierre Ossman
URL:
Keywords: frifl_tester, prosaic, tobfa_tester
Depends on:
Blocks:
 
Reported: 2023-01-02 13:15 CET by Pierre Ossman
Modified: 2023-06-30 16:02 CEST (History)
3 users (show)

See Also:
Acceptance Criteria:


Attachments

Description Pierre Ossman cendio 2023-01-02 13:15:11 CET
Trying to build our build system packages on Fedora 36 results in:

> gcc: error: /usr/lib/rpm/redhat/redhat-hardened-cc1: No such file or directory

The reason for this is:

https://fedoraproject.org/wiki/Changes/SetBuildFlagsBuildCheck

tl;dr: The now implicitly set CFLAGS and friends at the start of %build, and their flags are incompatible with our builds.
Comment 7 Pierre Ossman cendio 2023-01-03 09:07:54 CET
I figured it was best to try to mimic what Fedora does for us as well. There are two complications, however:

 * They do a lot of other magical stuff that we don't want

 * We build our packages using the system's rpm, which means we don't know which version, or which distribution, or even if the distro macros are installed

So, I did a general clean up where we try to avoid the distro magic a bit more, and rely more on the base functionality of rpm itself. I then added the new stuff Fedora has added directly in to our macros.

The new basic principle is that our macros correspond to the macros Fedora add on top of the basic rpm macros.
Comment 8 Pierre Ossman cendio 2023-01-03 09:08:55 CET
Packages built using the new macros seem to work fine. I can see CFLAGS and friends being set correctly in the build log.

I also rebuilt the entire build system on a RHEL 8 VM, so no obvious side effects.

Good enough for me.
Comment 9 Frida Flodin cendio 2023-01-10 16:37:04 CET
Tried to reproduce the issue by building everything on a fresh Fedora 36 VM. It failed, but I got another error. When building cendio-build-gmp-bootstrap-i386 I get:  

> checking size of mp_limb_t... 4
> configure: error: Oops, mp_limb_t is 32 bits, but the assembler code
> in this configuration expects 64 bits.
> You appear to have set $CFLAGS, perhaps you also need to tell GMP the
> intended ABI, see "ABI and ISA" in the manual.

I don't know if it's the same issue. I have not tried to build after the fixes on this bug. 

NOTE: I had to install some dependencies in order to get this far:
> dnf install -y make gcc rpmdevtools rpm-sign zlib-devel flex glibc-devel.i686 perl
Comment 10 Pierre Ossman cendio 2023-01-11 12:51:52 CET
Hmm... Builds just fine on my Fedora 36 VM. Must be something subtle going on here.
Comment 11 Frida Flodin cendio 2023-01-17 12:27:59 CET
To clarify, the error in comment #9 was found when trying to reproduce the error in the description. When building the current version of cenbuild, with Pierres fixes, I don't get that error. So this seems to be another symptom of the same issue.
Comment 12 Frida Flodin cendio 2023-01-17 12:42:12 CET
Tested on Fedora 36, and it builds without any errors. Also did a complete rebuild on RHEL 8.

Tested that the resulting build systems worked by building ThinLinc client and server and tested those. Everything seems to work just fine. 

Checked the code changes and they look good.
Comment 14 Linn cendio 2023-06-21 12:20:46 CEST
The reson we don't get a cleanup at the start of %install is since commit r39405, there is nothing removing the build root folder anymore. The path of the build root folder can be found in $RPM_BUILD_ROOT.

I have looked into which macros that seem to be responsilbe for this cleanup. I found that it is the macros belonging to the dist, rather than the macros belonging to rpm itself, that should cleanup the folder. In cenbuild, we are taking the role of the dist, so we are the ones who should clean up the build root.
Comment 15 Linn cendio 2023-06-21 12:30:17 CEST
Previously, we used variable $RPM_BUILD_ROOT to get the path to the build root folder. However, there is another variable, %{buildroot}, that contains the same path. I tried finding information about which of these variables are recommended. Most of what I found was quite old (10-20 years ago), and they seemed to favour $RPM_BUILD_ROOT. 

I also looked through some more recent spec files in Fedora Infrastructure repos [1][2] to see what the current standard is, and the more recent spec files favoured using %{buildroot}. We should likely also use %{buildroot} going forward to adjust to this guideline.

[1]: https://github.com/fedora-infra/bodhi
[2]: https://github.com/fedora-infra/the-new-hotness
Comment 16 Linn cendio 2023-06-21 15:29:17 CEST
The way we previously removed the buildroot folder was by having this line in first in %install of all affected spec files:
> rm -rf $RPM_BUILD_ROOT
One way to reduce duplication of this is to place this line in our install macro defined in utils/macros, since those macros are shared between all our spec files. As mentioned in comment 15, we should also likely change $RPM_BUILD_ROOT to %{buildroot}.
Comment 17 Linn cendio 2023-06-21 15:33:11 CEST
Below is the script redhat uses to clear the build root folder, found in /usr/lib/rpm/redhat/macros:
> %__spec_install_pre %{___build_pre}\
>     [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf "${RPM_BUILD_ROOT}"\
>     mkdir -p "`dirname "$RPM_BUILD_ROOT"`"\
>     mkdir "$RPM_BUILD_ROOT"\
>     %{?_auto_set_build_flags:%{set_build_flags}}\
> %{nil}
The commit message is quite sparse in information [1], but it seems this code was taken from OpenSUSE. However, I wasn't able to find the OpenSUSE commit that it references.

Either way, making sure that the build root folder is created seems like a reasonable thing to do, and is probably something that we want to do as well. As long as the folder is created safely in all scenarios, I don't really see a drawback from adding this to our code as well.

[1]: https://src.fedoraproject.org/rpms/redhat-rpm-config/c/159a65fb611beb4851efe96dbf1a7ced8859b7be
Comment 18 Linn cendio 2023-06-26 11:12:26 CEST
(In reply to Linn from comment #17)
> Below is the script redhat uses to clear the build root folder, found in
> /usr/lib/rpm/redhat/macros:
> > %__spec_install_pre %{___build_pre}\
> >     [ "$RPM_BUILD_ROOT" != "/" ] && rm -rf "${RPM_BUILD_ROOT}"\
> >     mkdir -p "`dirname "$RPM_BUILD_ROOT"`"\
> >     mkdir "$RPM_BUILD_ROOT"\
> >     %{?_auto_set_build_flags:%{set_build_flags}}\
> >  %{nil}
While Red Hat does have the macro shown above, a variant of that script has also been included in the rpm package since version 4.16.0 (see [1]):
> %__spec_install_pre %{___build_pre}\
>     %{__rm} -rf "%{buildroot}"\
>     %{__mkdir_p} "%{dirname:%{buildroot}}"\
>     %{__mkdir} "%{buildroot}"\
> %{nil}
[1]: https://github.com/rpm-software-management/rpm/commit/f49671d6f3442cfff08d6167b618607e96cf0970

This script was added to "Ensure empty buildroot for %install", as this is expected behaviour and each dist should not have to this manually. Hence, this functionality should be added to the part of our code this is mimicking rpm's macros, rather than mimicking dist-specific macros. 

In commit r39404, we added a manual definition of %__spec_install_pre, which overwrote the definition made by rpm. This explains why our buildroot is currently not cleaned before install.
Comment 20 Linn cendio 2023-06-27 09:57:15 CEST
As a part of this bug, we also want to swap all occurences of $RPM_BUILD_ROOT to %{buildroot} to follow current guidelines (see comment 15 for more info).

Most of our use for this variable is in %clean. Modern spec files seem to omit %clean and I found that RPM since version 4.8 calls clean by default if it is omitted (bug ticket [1] and commit [2]). We should also omit %clean to get this default cleanup.

[1]: https://rpm.org/ticket/81
[2]: https://github.com/rpm-software-management/rpm/commit/3fc58248d23d6f720942e5cbf4f92db246a802f0
Comment 21 Linn cendio 2023-06-27 12:30:21 CEST
Tested the new utils package by installing it and building another package (I chose pcsc-lite). The building went fine and I did not see any errors in the build log.

Also tested upgrading cendio-build-utils via dnf and I could see that I got the new version.
Comment 24 Linn cendio 2023-06-27 15:55:51 CEST
Tested building a package after removing %clean and changing all references of $RPM_BUILD_ROOT to %{buildroot}. The package built just fine and did not see any errors in the log.

To summarise, the changes made after reopening this bug have been:

 1) Ensure that the buildroot folder always is empty before installing. To test this, the updated version of the cendio-build-utils package must be installed before building.
 2) Clean up spec-files by removing %clean. Should have no impact on building.
 3) Use variable %{buildroot} instead of $RPM_BUILD_ROOT. Should have no impact on building.

With that, this bug is ready for testing.
Comment 25 Tobias cendio 2023-06-30 16:02:21 CEST
I’ve tested this bug on Fedora36 and RHEL8, in particular the recent fixes pertaining to comment 13 and comment 14. This was in part achieved by building cendio-build-utils lacking the recent cleanup fixes, and comparing the results with a corresponding build exhibiting said fixes. 

A couple of test packages were built; pcsc-lite, zlib, and gtk2. Applying the pre-fix build of cendio-build-utils, potential existing files in the build root folder are not properly removed when building, inhibiting the build process. Conversely, such residual files are not present with the post-fix build.

Furthermore, in case of successful builds, the build root is cleansed properly, leaving no residues. This was the case prior to the recent additions however, which confirms that the effect of the removal of the %clean section in the spec-files is working as intended.

Finally, confirmed that dnf offers the latest version of cendio-build-utils.

In conclusions, packages are successfully building and no lingering files are allowed to remain to potentially corrupt future builds.

Closing.

Note You need to log in before you can comment on or make changes to this bug.