Skip to content

Conversation

@dstaley
Copy link
Contributor

@dstaley dstaley commented Mar 16, 2024

This PR updates the ldd content checks for family and version to account for distributions that are more closely aligned with the official formatting of ldd, which does not contain the string "GLIBC".

This allows faster checks on distros like Amazon Linux, openSUSE, Fedora, RHEL, Arch, and more without falling back to process.report.getReport().

@lovell
Copy link
Owner

lovell commented Mar 17, 2024

Thank you for the PR Dylan.

Could checking for "libc" on its own, which feels rather generic, potentially lead to false positives? What does Bionic libc report? What if musl starts using the string "MUSL libc"?

Would a check for the explicit string "GNU libc" or similar be more robust?

@dstaley
Copy link
Contributor Author

dstaley commented Mar 17, 2024

Great question! I definitely see how being a tad bit more generic could potentially lead to false positives (although I think, at present, those situations are theoretical rather than actually possible given the current file content of ldd for musl and bionic).

GNU libc isn't present, but GNU C Library or even This file is part of the GNU C Library could work, with the latter being highly unlikely to appear in any implementation that isn't at least derived from libc.

@lovell
Copy link
Owner

lovell commented Mar 17, 2024

Thanks, if searching for "GNU C Library" improves things then that seems like a good option here.

$ docker run --rm -it amazonlinux:2 grep GNU /usr/bin/ldd
# This file is part of the GNU C Library.
# The GNU C Library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
# The GNU C Library is distributed in the hope that it will be useful,
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
# You should have received a copy of the GNU Lesser General Public
# License along with the GNU C Library; if not, see
    echo 'ldd (GNU libc) 2.26'

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thank you for the updates, I've left one comment inline about the failing musl-based Void test. (The failing glibc-based Void test is now fixed on the main branch so a rebase will get this passing.)

@dstaley dstaley requested a review from lovell March 19, 2024 00:36
@lovell lovell merged commit 2642d96 into lovell:main Mar 19, 2024
@lovell
Copy link
Owner

lovell commented Mar 19, 2024

Thank you very much Dylan.

@lovell
Copy link
Owner

lovell commented Mar 19, 2024

This change was included in v2.0.3.

@ItsHarper
Copy link

For the record, the musl binary DOES includes the string "musl libc", thank you for not letting a regression make it in.

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.

3 participants