std.zig.system.NativeTargetInfo: fix glibc version parsing

[BratishkaErik]

In most cases "GLIBC_2.X" strings and /lib/libc-2.x.so files do not contain third (patch) field, which causes std.SemanticVersion.parse function to return error. To fix this, we reuse [now-public] std.zig.CrossTarget.parseVersion function, which accounts for this third field and makes it 0 in case it was not found. This new behaviour is similar to std.builtin.Version.parse, which was removed in 6e84f46

Fixes regression from 6e84f46 and #13998 .

Related: #17626 . Results with zig end:

Before: "target": "x86_64-linux.6.5.7...6.5.7-gnu.2.19",
After: "target": "x86_64-linux.6.5.7...6.5.7-gnu.2.36",

Also, while we are here, write explicit error sets and remove duplicate logic from std.zig.system.darwin.macos.parseSystemVersion .

[rootbeer]

Looks good to me. I've got a couple suggestions, but they're all minor and you're free to ignore them.

[rootbeer]

Not really part of your change, but do you know if this parseVersionComponent initialized via struct member is just trying define a nested function? Or is it doing something else?

[BratishkaErik]

IDK what do you precisely mean here, but I renamed it to "parseVersionComponentFn" and put struct { ... } in parentheses.

[rootbeer]

I'm probably just confused. I don't understand why there is a struct declaration here at all. I am guessing its a hack to effectively declare a nested function inside parseVersion. But I'm curious if its accomplishing something else.

(Seems cleaner to just define a top-level, non-public function to me, but I'm not clear if there is some Zig style or other motivation for the current approach.)

Anyway, probably not a discussion to have on a PR. I should just bring this up in Discord or somesuch.

The fixes and test cases look great, btw.

[BratishkaErik]

I'm probably just confused. I don't understand why there is a struct declaration here at all. I am guessing its a hack to effectively declare a nested function inside parseVersion. But I'm curious if its accomplishing something else.

(Seems cleaner to just define a top-level, non-public function to me, but I'm not clear if there is some Zig style or other motivation for the current approach.)

Anyway, probably not a discussion to have on a PR. I should just bring this up in Discord or somesuch.

The fixes and test cases look great, btw.

Oh, sorry I misunderstood it. IIRC the reason was to not pollute this upper namespace with function that is used in only one function and allowing to use same identifier in other functions (with top-level function you would've need to rename new declarations to avoid shadowing). You can define sub-namespace std.CrossTarget.Version tho:

// maybe different name, idk.
pub const Version = struct {
    /// current docs
    pub fn parse(ver: []const u8) error{ InvalidVersion, Overflow }!SemanticVersion {
        // current parseVersion implementation
    }

    fn parseComponent(component: []const u8) error{ InvalidVersion, Overflow }!usize {
        // current parseVersionComponentInner implementation
        // This way parseComponent does not pollute upper namespace
    }
};

// To avoid unneccessary breakage, with deprecation comment
pub const parseVersion = Version.parse;
// std.zig.CrossTarget.versionParse becomes std.zig.CrossTarget.Version.parse

But IMHO this goes out-of-scope of this PR and would require more time for reviewings.

Thank you for your reviews, they were very helpful :)

[BratishkaErik]

Oh sorry did I overwrote your commit? Didn't noticed it sorry, I thought you were asleep.

[andrewrk]

I'm the one who should apologize for force pushing to your branch. It was only the github rebase branch button that I did:

So, if you rebased against latest master just now, then there is no problem 👍

[BratishkaErik]

So, if you rebased against latest master just now, then there is no problem 👍

did rebase right now.

[BratishkaErik]

Thanks!