libbpf-cargo: Remove clang version check logic#1057
Merged
d-e-s-o merged 1 commit intolibbpf:masterfrom Jan 16, 2025
Merged
Conversation
Remove the logic for checking the clang version in use. This logic is
worrisome and insufficient for a couple of reasons.
It is insufficient, because we don't actually know what clang version we
require -- that is a moving target. Nobody is actively testing against
the lowest supported clang version to make sure that our stuff actually
works with it. At this point, the stated minimum of 10.0.0 is also
increasingly unlikely to be found in the wild, rendering the check less
and less useful with every passing year. Ultimately, if a necessary
feature really is not available, one would hope that the emitted error
isn't too cryptic for anybody to make sense of it.
On the "worrisome" front, the over reliance on anything and everything
clang (not just with this check, but with naming more generally) boxes
us into a single-compiler world. It is entirely conceivable that a GCC
BPF backend arrives eventually (has it already?), which, if past is any
guidance, shoulder understand roughly the same arguments as clang
itself and, hence, potentially being a drop-in replacement. We should be
ready to support this future without shenanigans such as "gcc" being
passed to an API asking for "clang". Furthermore, parsing the compiler's
output using regular expressions is at best a fragile endeavor.
What's more, the amount of dependencies (direct + transitive) pulled in
just in service of this one feature is penalizing everybody, for no
obvious value-add.
Lastly, the plumping necessary for this feature is part of the reason we
have nonsensical call sites such as the following to deal with:
make(
true,
Some(&cargo_toml),
None,
Vec::new(),
true,
true,
Vec::new(),
None,
)
In short, removal of this checking logic is our best option. Take it.
Signed-off-by: Daniel Müller <deso@posteo.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove the logic for checking the clang version in use. This logic is worrisome and insufficient for a couple of reasons.
It is insufficient, because we don't actually know what clang version we require -- that is a moving target. Nobody is actively testing against the lowest supported clang version to make sure that our stuff actually works with it. At this point, the stated minimum of
10.0.0is also increasingly unlikely to be found in the wild, rendering the check less and less useful with every passing year. Ultimately, if a necessary feature really is not available, one would hope that the emitted error isn't too cryptic for anybody to make sense of it.On the "worrisome" front, the over reliance on anything and everything clang (not just with this check, but with naming more generally) boxes us into a single-compiler world. It is entirely conceivable that a GCC BPF backend arrives eventually (has it already?), which, if past is any guidance, shoulder understand roughly the same arguments as clang itself and, hence, potentially being a drop-in replacement. We should be ready to support this future without shenanigans such as "gcc" being passed to an API asking for "clang". Furthermore, parsing the compiler's output using regular expressions is at best a fragile endeavor. What's more, the amount of dependencies (direct + transitive) pulled in just in service of this one feature is penalizing everybody, for no obvious value-add.
Lastly, the plumping necessary for this feature is part of the reason we have nonsensical call sites such as the following to deal with:
In short, removal of this checking logic is our best option. Take it.