azurelinux icon indicating copy to clipboard operation
azurelinux copied to clipboard

Add minimal sensible `gcc` dependencies

Open bossmc opened this issue 2 years ago • 6 comments

Merge Checklist

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

  • [x] The toolchain has been rebuilt successfully (or no changes were made to it)
  • [x] The toolchain/worker package manifests are up-to-date
  • [x] Any updated packages successfully build (or no packages were changed)
  • [x] Package tests (%check section) have been verified with RUN_CHECK=y for existing SPEC files, or added to new SPEC files
  • [x] All package sources are available
  • [x] cgmanifest files are up-to-date and sorted (./cgmanifest.json, ./toolkit/tools/cgmanifest.json, ./toolkit/scripts/toolchain/cgmanifest.json, .github/workflows/cgmanifest.json)
  • [x] 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)
  • [x] All source files have up-to-date hashes in the *.signatures.json files
  • [x] sudo make go-tidy-all and sudo make go-test-coverage pass
  • [x] Documentation has been updated to match any changes to the build system
  • [x] 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 avatar Apr 12 '22 19:04 bossmc

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.

anphel31 avatar Apr 25 '22 22:04 anphel31

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 avatar Apr 27 '22 20:04 bossmc

@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)

bossmc avatar Apr 27 '22 20:04 bossmc

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.

anphel31 avatar May 24 '22 21:05 anphel31

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 avatar May 25 '22 17:05 bossmc

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)

bossmc avatar May 25 '22 18:05 bossmc