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

Add minimal sensible gcc dependencies #2716

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add minimal sensible gcc dependencies #2716

wants to merge 2 commits into from

Conversation

bossmc
Copy link
Contributor

@bossmc bossmc commented Apr 12, 2022

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

  • The toolchain has been rebuilt successfully (or no changes were made to it)
  • The toolchain/worker package manifests are up-to-date
  • Any updated packages successfully build (or no packages were changed)
  • Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • All package sources are available
  • cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/tools/cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • LICENSE-MAP files are up-to-date (./SPECS/LICENSES-AND-NOTICES/data/licenses.json, ./SPECS/LICENSES-AND-NOTICES/LICENSES-MAP.md, ./SPECS/LICENSES-AND-NOTICES/LICENSE-EXCEPTIONS.PHOTON)
  • All source files have up-to-date hashes in the *.signatures.json files
  • sudo make go-tidy-all and sudo make go-test-coverage pass
  • Documentation has been updated to match any changes to the build system
  • Ready to merge

Summary

Out of the box, tdnf install gcc only installs the compiler, not the archiver, nor the glibc headers needed to actually compile ~99% of all C code. Following the lead of Fedora this change pulls in glibc-devel, kernel-headers and binutils. Fedora also pulls in make but that doesn't feel right to me (using gcc without make is pretty common these days thanks to things like ninja).

Change Log
  • None, feels like a minor internal change
Does this affect the toolchain?

No

Associated issues

None

Links to CVEs

None

Test Methodology

None

@bossmc bossmc requested a review from a team as a code owner April 12, 2022 19:22
@ghost ghost added Packaging main PR Destined for main labels Apr 12, 2022
Copy link
Contributor

@oliviacrain oliviacrain left a comment

Choose a reason for hiding this comment

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

Since gcc is in our toolchain, we'll need to update the toolchain manifests - if you're not familiar with the process, I can provide you with a suitable patch when I have some free time this week.

Once that's done, I'll go ahead and start a full pipeline test build for this PR. Specifically, I want to make sure that the removal of the Requires: libstdc++-devel line doesn't reveal any implicit dependencies on libstdc++-devel in any of our specs.

@oliviacrain oliviacrain requested a review from anphel31 April 13, 2022 16:26
@anphel31
Copy link
Member

We will need to perform a full build with this change, since it will change the dependency graph (and could cause unexpected issues building all packages). To update the manifests before testing you can run "sed -i 's/-11.2.0-2.cm2./-11.2.0-3.cm2./g' ./resources/manifests/package/*.txt". I'll also note that we do have the "build-essential" metapackage which installs gcc, kernel-headers, etc.

@bossmc
Copy link
Contributor Author

bossmc commented Apr 27, 2022

Thanks @anphel31, I've updated the toolchain manifests as you recommended.

Yes, I'm aware of build-essential but installing that pulls 360Mb of packages whereas installing gcc (post this change) pulls merely 10Mb. For simple use cases there's a sizeable win to just installing the things you need (e.g. as setup for a pipeline job) and, in general, having packages that are effectively dead on arrival is pretty off-putting to users.

@bossmc
Copy link
Contributor Author

bossmc commented Apr 27, 2022

@oliviacrain the change to remove the libstdc++-devel requirement from gcc is a no-op since it's already transitively required though the gcc-c++ requirement that continues to exist (I only removed the redundant requirement for future-proofing)

@anphel31
Copy link
Member

anphel31 commented May 24, 2022

I've run pipeline builds with this change, and saw an issue when building images. Both marketplace images failed to build with the following:

WARN[0038] installing package rsyslog-8.2108.0-1.cm2.x86_64 needs 46MB more space on the / filesystem
WARN[0038] Failed to tdnf install: exit status 245. Package name: rsyslog
ERRO[0039] Failed to build image

The marketplace disk size is set to 1500MB. When I reran with 2000MB disk, the images build.
I'm seeing the following additional packages that are now present in the marketplace image, including:
binutils
binutils-devel
glibc-devel
libpq
Please take a look since we want to minimize unnecessary packages in the image, especially -devel packages.

@bossmc
Copy link
Contributor Author

bossmc commented May 25, 2022

That installation is pulling in a whole compiler toolchain (due to rsyslog -> net-snmp-libs -> libperl -> perl -> perl-Extutils-CBuilder -> gcc -> glibc-devel -> etc). I can't (quite) justify needing a compiler to use libperl (it's needed to actively use the CBuilder package but even that is set up with a have_compiler() utility to detect the lack of compiler).

I think the marketplace image shouldn't be pulling in gcc and friends at all (we've seen very stringent requirements from customers that shipping a compiler on a product image is no-bueno for security reasons, and in general it's bloating the image in a way that's not going to be expected by a consumer.

Looking around other distros for inspiration for how they approach this:

  • Ubuntu (Jammy)'s rsyslog doesn't depend on libsnmp, nor does their perl-modules (which includes CBuilder) depend on gcc
  • Centos 7's rsyslog doesn't depend on snmp and their perl doesn't even install CBuilder (so doesn't install gcc)
  • RHEL 8's rsyslog doesn't depend on snmp, their perl does install CBuilder but that doesn't install gcc
  • Fedora 35's rsyslog doesn't depend on snmp, their net-snmp-libs doesn't depend on libperl but their perl does install CBuilder and gcc

Looking down the dependency chain, we could break the chain at (any subset of):

  1. rsyslog -> net-snmp-libs (and remove the ability to emit syslogs over SNMP INFORM messages)
  2. net-snmp-libs -> libperl (not sure what the ramifications of this are)
  3. perl -> CBuilder (I don't think this is a good idea, despite precedent in Centos, since perl declares CBuilder as a core package
  4. CBuilder -> gcc (this means the module isn't fully functional when installed, but it seems to be designed with this in mind and installing a perl runtime feels like a different intent to installing a perl toolchain to me)

Personally (using the logic that debian generally make good decisions when it comes to packaging) I'd go for 1 and 4, but I'm not sure what motivated the inclusion of SNMP support in rsyslog (maybe just "because we can"?)

@bossmc
Copy link
Contributor Author

bossmc commented May 25, 2022

It looks like an alternative option would be to package (some of) the rsyslog plugins as their own packages (crucially in this case the omsnmp module is the one that's pulling in the dependency on libnetsnmp.so)

@PawelWMS PawelWMS requested a review from a team as a code owner December 12, 2024 00:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
main PR Destined for main Packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants