zig icon indicating copy to clipboard operation
zig copied to clipboard

std: replace builtin.Version with SemanticVersion

Open wooster0 opened this issue 1 year ago • 17 comments

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

wooster0 avatar Dec 18 '22 21:12 wooster0

Ok so there's this problem where we have versions that we want to parse but they don't conform to SemVer https://github.com/ziglang/zig/blob/3bfae2a0d9503dbd832774c3b39a74dbd55e8b6e/lib/std/zig/system/darwin/macos.zig#L378-L382 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.

wooster0 avatar Dec 19 '22 08:12 wooster0

https://github.com/ziglang/zig/issues/12862#issuecomment-1362984212

wooster0 avatar Dec 22 '22 15:12 wooster0

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

andrewrk avatar Feb 19 '23 19:02 andrewrk

Going to wait for a new master build first.

wooster0 avatar Feb 21 '23 20:02 wooster0

Rebased.

wooster0 avatar Feb 22 '23 16:02 wooster0

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?

wooster0 avatar Feb 22 '23 16:02 wooster0

https://github.com/ziglang/zig/pull/14691 added a step that copies zig.h into stage1/ but maybe that wasn't run as part of that PR?

kcbanner avatar Feb 22 '23 16:02 kcbanner

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.

wooster0 avatar Feb 22 '23 16:02 wooster0

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.

wooster0 avatar Feb 24 '23 22:02 wooster0

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.

Vexu avatar Feb 24 '23 23:02 Vexu

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.

wooster0 avatar Feb 26 '23 13:02 wooster0

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...

wooster0 avatar Mar 04 '23 09:03 wooster0

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.

wooster0 avatar Mar 04 '23 17:03 wooster0

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.

kcbanner avatar Mar 04 '23 17:03 kcbanner

I think this PR can be closed.

perillo avatar Apr 04 '23 08:04 perillo

Why?

wooster0 avatar Apr 08 '23 12:04 wooster0

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.

perillo avatar Apr 09 '23 12:04 perillo