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

update to SDK v0.27.0 #2428

Merged
merged 12 commits into from
Sep 27, 2022
Merged

Conversation

bcressey
Copy link
Contributor

@bcressey bcressey commented Sep 15, 2022

Issue number:
Fixes #2419
Fixes #2418
Fixes #2175

Description of changes:
Update to the latest SDK, from bottlerocket-os/bottlerocket-sdk#82.

This adds two workarounds for new rpm behavior in Fedora 36, and syncs glibc and binutils with the versions found in the SDK. The version of Rust used for CI builds is also updated to match 1.63.0 in the SDK. The newer meson lets us update dbus-broker to v32, and requires some minor changes to iputils to drop unknown options.

The only controversial change may be the addition of -Bsymbolic-functions to the default linker flags. There's some light background reading here and here. The quick summary is that I don't really expect problems since we've been using -fno-semantic-interposition on the compiler side since 237946d, so it's hopefully just free performance at runtime from simplifying the dynamic linker's job. 🤞🏻

Testing done:
Ran the quick tests for aws-* using testsys. Everything passed except for aws-k8s-1.20 on aarch64, which needs a custom conformance image, and aws-k8s-1.23, which needs a newer eksctl.

Verified that aws-k8s-1.22-nvidia still works. Ran some basic smoke tests for vmware-dev and vmware-k8s-1.22.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

.github/workflows/build.yml Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
macros/shared Show resolved Hide resolved
Patch0026: 0026-io-Add-fsync-call-in-tst-stat.patch
Patch0027: 0027-nss-Do-not-mention-NSS-test-modules-in-gnu-lib-names.patch
Patch0028: 0028-nss-Protect-against-errno-changes-in-function-lookup.patch
Patch001: 0001-stdlib-Suppress-gcc-diagnostic-that-char8_t-is-a-key.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

There are 39 patches (or at least that's the last number in the patches' names), but only 6 are applied, is there a reason why the remaining patches are included but not applied? Also there are two patches that modify NOTES and NEWS, are they required even if they don't make any functional change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops - that's my mistake. There were only six patches when I first added glibc 2.36, then the number grew 😀 I added the patches but forgot to update the spec.

As for the patches modifying NOTES and NEWS - the patch series is more maintainable if it's just "all patches to 2.36 since release" rather than cherry-picking a subset.

tools/testsys/src/aws_resources.rs Outdated Show resolved Hide resolved
@bcressey
Copy link
Contributor Author

Force push above:

  • applies all the patches in glibc.spec
  • updates cargo-make as well
  • drops the clippy fix from 23255a8

Signed-off-by: Ben Cressey <[email protected]>
Fedora 36 automatically sets certain flags during RPM builds:
  https://fedoraproject.org/wiki/Changes/SetBuildFlagsBuildCheck

Since the `set_cross_build_flags` macro will use these flags if they
are specified in the environment, this causes the expected flags for
cross-compilation to be ignored, leading to build failures.

It's necessary to undefine the macro to suppress the new behavior.
Unfortunately, that can't be done through the usual macro inheritance
approach, because macros cannot be undefined, only reset. Therefore
the `rpmbuild` invocation is modified to remove the definition.

Signed-off-by: Ben Cressey <[email protected]>
Fedora 36 moved the RPM database to /usr:
  https://fedoraproject.org/wiki/Changes/RelocateRPMToUsr

For Bottlerocket, /usr should be a symlink to the sysroot directory.
If the RPM database is stored in /usr, then it will be created prior
to installing any Bottlerocket packages, and create a conflict with
the intended symlink.

Ordinarily this would be changed in the shared macros, but doing that
could confuse `rpm` during the package build step, since the SDK's
rpm database is still in `/usr` and package builds require a mix of
host and target packages to be installed.

Instead the definition is overridden just before image creation, when
host packages are no longer needed and the target packages have not
yet been installed.

Signed-off-by: Ben Cressey <[email protected]>
In 237946d, `-fno-semantic-interposition` was added to the default
build flags. This allows the compiler to assume that exported symbols
will not be interposed and to optimize accordingly, for example by
inlining functions.

`-Bsymbolic-functions` tells the linker to bind references to global
function symbols to the definition inside the shared library, if any,
which should be safe given that the compiler has already optimized
the code under the assumption that those definitions will be used.

Signed-off-by: Ben Cressey <[email protected]>
Includes all patches since the initial release.

Signed-off-by: Ben Cressey <[email protected]>
The 5.4 kernel was dropped in 42cdb1e, so all new releases will have
a 5.10+ kernel.

Signed-off-by: Ben Cressey <[email protected]>
The libdep plugin is always built now, so the option to build without
plugins was removed.

Signed-off-by: Ben Cressey <[email protected]>
Signed-off-by: Ben Cressey <[email protected]>
@bcressey
Copy link
Contributor Author

Force push for a rebase to pull in the lint fix.

Copy link
Member

@jpculp jpculp left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@bcressey
Copy link
Contributor Author

Force push applies go fmt from Go 1.19.

Comment on lines -42 to -43
-DBUILD_TFTPD=false
-DBUILD_TRACEROUTE6=false
Copy link
Member

Choose a reason for hiding this comment

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

I was curious why these cause trouble now, since they are/should be defined by iputils and the iputils build didn't change. All those options were already gone in the iputils build referenced here, but it wasn't until Meson 0.60 that passing unkonwn configure options causes build failures rather than warnings (source).

packages/binutils/binutils.spec Show resolved Hide resolved
@bcressey bcressey marked this pull request as ready for review September 26, 2022 21:33
Copy link
Contributor

@stmcginnis stmcginnis left a comment

Choose a reason for hiding this comment

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

Tests are happy and everything looks fine to me.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🍦

@bcressey bcressey merged commit d04eaa7 into bottlerocket-os:develop Sep 27, 2022
@bcressey bcressey deleted the sdk-shenanigans branch September 27, 2022 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update binutils to 2.38 update glibc to 2.36 Many packages require newer build tools in the SDK
6 participants