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

std.Target.Query: Fix parse test on ABIs like gnueabi, gnuabi64, etc. #21109

Merged
merged 1 commit into from
Aug 24, 2024

Conversation

alexrp
Copy link
Member

@alexrp alexrp commented Aug 17, 2024

The zigTriple() implementation simply returns gnu when a glibc version is provided but a more specific ABI isn't.

if (self.glibc_version) |v| {
const name = if (self.abi) |abi| @tagName(abi) else "gnu";
try result.ensureUnusedCapacity(name.len + 2);
result.appendAssumeCapacity('-');
result.appendSliceAssumeCapacity(name);
result.appendAssumeCapacity('.');
try formatVersion(v, result.writer());
} else if (self.abi) |abi| {
const name = @tagName(abi);
try result.ensureUnusedCapacity(name.len + 1);
result.appendAssumeCapacity('-');
result.appendSliceAssumeCapacity(name);
}

I do realize I sound like a broken record at this point, but this is another problem we have because we conflate libc and ABI.

Note: I think this test has been broken for a long time, but it was not failing in CI because you need to be testing for a target like arm-linux-gnueabi, mips64-linux-gnuabi64, etc, and AFAIK the CI machines do not have non-native glibcs installed (which is a bit of a shame IMO, but I digress).

…, etc.

The `zigTriple()` implementation simply returns `gnu` when a glibc version is provided but a more specific ABI isn't.
@andrewrk
Copy link
Member

You're trying to fix this on arm-linux-gnueabi but then the expected value is wrong because the zip triple should be "arm-linux-gnueabi".

@alexrp
Copy link
Member Author

alexrp commented Aug 23, 2024

The issue is that the resulting Zig triple can't become *-*-gnueabi; there is simply not enough information provided for zigTriple() to determine whether it should return gnu, gnuabi64, gnueabi, etc for the ABI. So right now, it just returns gnu. And if we're targeting e.g. arm-linux-gnueabi (or gnueabihf, gnuabi64, ...), that means we construct the expected target triple native-native-gnueabi.2.1.1 which will not match native-native-gnu.2.1.1 as returned by zigTriple().

So as I see it, either a) the test needs to be fixed like I've done here, or b) the test is actually unworkable and should be deleted.

@andrewrk
Copy link
Member

There's a third option, you can skip the test on a non-applicable host.

@alexrp
Copy link
Member Author

alexrp commented Aug 23, 2024

That works too I suppose. Although with #20690 accepted, the "API" component of the target becomes just gnu anyway, so in that light, perhaps it makes sense to take the PR as-is? 🤷 I don't actually have a strong opinion either way; I'd just like this test to stop failing my local multi-glibc builds.

@andrewrk
Copy link
Member

I see your point, OK 👍

@andrewrk andrewrk merged commit cad69e2 into ziglang:master Aug 24, 2024
10 checks passed
@alexrp alexrp deleted the patch-1 branch August 24, 2024 14:41
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.

2 participants