std: replace builtin.Version with SemanticVersion · Pull Request #13998 · ziglang/zig

ghost · 2022-12-18T21:17:55Z

This removes std.builtin.Version in favor of std.SemanticVersion.

I don't think this is something that belongs in std.builtin.

This avoids the confusion when you don't know which of the two structs to use.
The patch field must now always explicitly be specified and no longer defaults to 0.
With this upgrade, some strings previously accepted by std.builtin.Version are no longer accepted by std.SemanticVersion, and vice versa. There aren't a lot though. std.SemanticVersion is probably more correct.
I think the name is good as-is and coincidentally SemanticVersion is the same length as builtin.Version.
SemanticVersion gives you less errors than builtin.Version does on parse.
I don't expect there to be too much code breakage because the two are still quite compatible.

I took over the old tests from std.builtin.Version and added them in lib/std/SemanticVersion.zig (basically the only valuable things).

Fixes #12862

src/link/MachO/load_commands.zig

ghost · 2022-12-19T08:31:53Z

Ok so there's this problem where we have versions that we want to parse but they don't conform to SemVer

zig/lib/std/zig/system/darwin/macos.zig

Lines 378 to 382 in 3bfae2a

    
           \\	<string>11.1</string> 
        
           \\	<key>ProductVersion</key> 
        
           \\	<string>11.1</string> 
        
           \\	<key>iOSSupportVersion</key> 
        
           \\	<string>14.3</string>

Semantic Versioning does not accept "1.0" for example. The patch part must be specified.

Edit: so I decided to implement version parsing specifically for the cases there, which wasn't very complicated at all.

lib/std/zig/system/darwin/macos.zig

ghost · 2022-12-22T15:38:36Z

#12862 (comment)

ghost · 2022-12-22T22:49:46Z

src/main.zig

@@ -1048,7 +1048,7 @@ fn buildOutputType(
                        try cssan.addIncludePath(.iframework, arg, args_iter.nextOrFatal(), false);
                    } else if (mem.eql(u8, arg, "--version")) {


The docs for this is "Dynamic library semver" which wasn't actually true. Now it is because it no longer accepts an omitted patch component (according to the spec "1.0" is invalid). This might cause breakage.

Vexu · 2023-01-23T18:12:58Z

lib/std/zig/CrossTarget.zig

+    const major = version_components.first();
+    if (major.len == 0) return error.InvalidVersion;
+    const minor = version_components.next() orelse return error.InvalidVersion;
+    const patch = version_components.next() orelse "0";


This version doesn't check that there aren't additional components after the patch. Same error is repeated in zig/system/darwin/macos.zig, would it instead be better to provide a properly tested non-standard function somewhere so that other people don't repeat it by implementing their own version?

The above brings me to this comment: where shall we put this function and its tests? It should probably be an entirely internal function. lib/std/zig/CrossTarget.zig and lib/std/zig/system/darwin/macos.zig share lib/std/zig/ so should it be lib/std/zig/version.zig or something?

I'd either put it in SemanticVersion.zig and clearly document it as non-standard or just lib/std/zig.zig.

Hmm I was just doing this but now I'm thinking is std.zig really the right space for this? Version parsing like that isn't really part of parsing Zig code. Doesn't really seem to fit in that file.

Well, I was gonna say another motivation of this PR was to reduce the public API surface a bit, and really, after this missing check at the end it seems to be complete now. The code really isn't that complicated.
The advantage is that now we have a single, standard way to parse a version.
For all other cases the user has to write this themselves, and it really isn't that complicated, and also only an optional patch component seems kind of specific. What if the user wants more components to be optional, etc.?
So this way it's also very formidable for the use case at hand.

So for now I went back to the way it was and fixed up the parsing in both places.
However if you can think of a better place, where ideally it doesn't get into the public std, maybe we can do it the way your proposing.

Maybe lib/std/zig/version.zig? Can we put it there publicly and then only use it in those two places and not expose it to the public std API surface?

And if we put it in std.SemanticVersion as non-standard, I think we should make it customizable and allow the minor component to be optional too, at which point maybe it gets a bit too complicated? I don't know. Maybe it's ok then.

lib/std/zig/CrossTarget.zig

andrewrk · 2023-02-19T19:31:05Z

Looks pretty good. Would you mind rebasing on latest master branch?

ghost · 2023-02-21T20:01:15Z

Going to wait for a new master build first.

ghost · 2023-02-22T16:34:36Z

Rebased.

ghost · 2023-02-22T16:36:23Z

stage1/zig.h has a lot of changes. All I did was zig build update-zig1 --zig-lib-dir lib. So, most changes are coming from the second commit, but I guess it's intended?

kcbanner · 2023-02-22T16:42:37Z

#14691 added a step that copies zig.h into stage1/ but maybe that wasn't run as part of that PR?

ghost · 2023-02-22T16:50:09Z

That could very well be it. Maybe it's ok to do it here. I might split it into 3 commits at least, though.
Also, I can see a CI failure, which looks weird. I should probably investigate that first.

ghost · 2023-02-24T22:39:04Z

Not yet sure what's up here. Running zig build test --zig-lib-dir lib gives me an enormous amount of memory leak logs bigger than my terminal buffer size. I seem to be hitting some kind of a compiler bug?
I wonder if this is in fact related to those stage1/zig.h changes. In the future someone else will run zig build update-zig1 and then if they have the same issue, that's probably it.

Vexu · 2023-02-24T23:33:27Z

Running zig build test --zig-lib-dir lib gives me an enormous amount of memory leak

Known issue, build with llvm backend enabled so that the compiler uses malloc.

ghost · 2023-02-26T13:34:31Z

So I ran the zig build test-std tests locally and they all passed so I suspect this is an issue outside the std and possibly caused by the stage1/zig.h changes or something else. It's pretty hard to debug the compiler bug at this scale so for now I could just wait and see if the issue resolves itself sometime after some rebases.

ghost · 2023-03-04T09:54:04Z

Another PR of mine #14782 is hit with the exact same CI failure. Can anyone tell me the correct way to rebuild stage1/? I don't seem to be doing it correctly.
Am I supposed to run zig build update-zig1 with or without --zig-lib-dir lib and am I supposed to use a Zig compiler built by following the steps https://github.com/ziglang/zig/wiki/Building-Zig-From-Source#option-a-use-your-system-installed-build-tools in this branch or can I also use a Zig compiler downloaded from https://ziglang.org/download?
Anyway, should work just fine when I simply don't update stage1/zig1.wasm...

ghost · 2023-03-04T17:34:34Z

So this has some interesting build-related breaking changes:

const std = @import("std");

pub fn build(b: *std.Build) void {
    const optimize = b.standardOptimizeOption(.{});
    const target: std.zig.CrossTarget = .{ .os_tag = .macos };

    const lib = b.addSharedLibrary(.{
        .name = "a",
        .version = .{ .major = 1, .minor = 0 },
        .optimize = optimize,
        .target = target,
    });

The patch version component must be specified:

        .version = .{ .major = 1, .minor = 0, patch = 0 },

Here's a shorter more nicer-looking alternative:

        .version = std.SemanticVersion.parse("1.0.0"),

(shortness depends on what you have imported as well)

In the future this might become shorter and more nicer when we have anonymous function calls like this:

        .version = .parse("1.0.0"),

(don't remember the proposal for this)
I think that'll be pretty nice.
And it's a build script so it doesn't matter whether it parses that string at comptime or at runtime, anyway.

So, packages previously used std.builtin.Version and now std.SemanticVersion, but that makes me think--don't we force a certain way of doing versioning on all users now? What if they don't follow semver? Isn't this a bit wrong?

I am thinking if maybe std.SemanticVersion should be std.Version and people that wish to interpret it as semver can do so and everyone else can interpret it as simply a version or something else or however they do versioning.
I think this would provide more freedom and flexibility and would also be shorter to type.
We could still state that std.Version would be inspired by Semantic Versioning.
While we're at it, we could probably change the patch version components major, minor, and patch to be u16 because usize is just kind of weird IMO (why the size of a pointer; these components aren't used to index data) and if you have a component bigger than 65535 then you're doing something wrong anyway.
CC @andrewrk you probably have some opinions on this with your recent package-related changes.
If you want you can just merge this and I can open an issue for that to discuss that later.

kcbanner · 2023-03-04T17:44:11Z

Another PR of mine #14782 is hit with the exact same CI failure. Can anyone tell me the correct way to rebuild stage1/? I don't seem to be doing it correctly. Am I supposed to run zig build update-zig1 with or without --zig-lib-dir lib and am I supposed to use a Zig compiler built by following the steps https://github.com/ziglang/zig/wiki/Building-Zig-From-Source#option-a-use-your-system-installed-build-tools in this branch or can I also use a Zig compiler downloaded from https://ziglang.org/download? Anyway, should work just fine when I simply don't update stage1/zig1.wasm...

You only have to update zig1.wasm if the changes you made are required to build the compiler itself. I usually build zig stage4 (with a prebuilt or bootstrapped zig), then run zig build update-zig1 using that stage4 zig.

perillo · 2023-04-04T08:06:06Z

I think this PR can be closed.

ghost · 2023-04-08T12:55:23Z

Why?

perillo · 2023-04-09T12:47:23Z

Why?

I started searching the oldest PR to find if there where some abandoned one. From the title of the PR I thought the builtin was referring to the old compiler builtin module. My bad for not checking the code (and the date, since the move of builtin types to std was done in 2019).

Sorry for the noise.

This broke with ziglang/zig#13998 so I copied the old Version struct into the file. `std.SemanticVersion` does not support parsing e.g. "2.38" because it does not allow the implicit ".0" to be dropped.

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 . Signed-off-by: Eric Joldasov <bratishkaerik@getgoogleoff.me>

ghost commented Dec 18, 2022

View reviewed changes

src/link/MachO/load_commands.zig Show resolved Hide resolved

ghost commented Dec 22, 2022

View reviewed changes

lib/std/zig/system/darwin/macos.zig Show resolved Hide resolved

ghost commented Dec 22, 2022

View reviewed changes

Vexu reviewed Jan 23, 2023

View reviewed changes

std: replace builtin.Version with SemanticVersion

7d5a73e

andrewrk merged commit 6e84f46 into ziglang:master Jun 17, 2023

kamidev mentioned this pull request Jun 17, 2023

Replace std.builtin.Version in favor of std.SemanticVersion zig-gamedev/zig-gamedev#320

Closed

BratishkaErik mentioned this pull request Oct 22, 2023

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

Merged

		@@ -1048,7 +1048,7 @@ fn buildOutputType(
		try cssan.addIncludePath(.iframework, arg, args_iter.nextOrFatal(), false);
		} else if (mem.eql(u8, arg, "--version")) {

Uh oh!

Conversation

ghost commented Dec 18, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 19, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ghost commented Dec 22, 2022

Uh oh!

ghost Dec 22, 2022

Choose a reason for hiding this comment

Uh oh!

Vexu Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Jan 23, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vexu Jan 23, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Jan 24, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewrk commented Feb 19, 2023

Uh oh!

ghost commented Feb 21, 2023

Uh oh!

ghost commented Feb 22, 2023

Uh oh!

ghost commented Feb 22, 2023

Uh oh!

kcbanner commented Feb 22, 2023

Uh oh!

ghost commented Feb 22, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 24, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vexu commented Feb 24, 2023

Uh oh!

ghost commented Feb 26, 2023

Uh oh!

ghost commented Mar 4, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Mar 4, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kcbanner commented Mar 4, 2023

Uh oh!

perillo commented Apr 4, 2023

Uh oh!

ghost commented Apr 8, 2023

Uh oh!

perillo commented Apr 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ghost commented Dec 18, 2022 •

edited by ghost

Loading

ghost commented Dec 19, 2022 •

edited by ghost

Loading

ghost Jan 23, 2023 •

edited by ghost

Loading

ghost Jan 24, 2023 •

edited by ghost

Loading

ghost commented Feb 22, 2023 •

edited by ghost

Loading

ghost commented Feb 24, 2023 •

edited by ghost

Loading

ghost commented Mar 4, 2023 •

edited by ghost

Loading

ghost commented Mar 4, 2023 •

edited by ghost

Loading