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

build_runner: add min_zig_version build.zig config #11987

Closed
wants to merge 1 commit into from

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented Jul 4, 2022

build_runner now checks build.zig for min_zig_version configuration, i.e.

//config min_zig_version 0.10.0-dev.2836+2360f8c49

This provides a standard for projects to document their min zig version. In
addition, if zig detects it's too old, it will print an error:

error: zig is too old, have 0.9.0 but build.zig has min_zig_version 0.10.0

Note that supporting a max_zig_version would be more difficult. Since
zig version numbers contain the commit height, we can tell whether Zig
might be "too old", but we can't tell whether zig is "too new". If commit height
A is less than B, then commit A is too old, but if A is greater than B,
then A is not necessarily too new because it could just be a different
branch with extra commits.

More clarification on min/max version checks and a full-proof solution

Note that it's possible that we can get "false negatives" with a min_zig_version check, meaning zig could be too old but we can't tell. "false negatives" are fine because in the worst case we just have the same behavior we have today, namely, the check does nothing and compilation fails because Zig is too old. However, a max_version_check can get "false positives" which puts us in a worse position than we are today, where the user has to disable the check somehow even if their compiler is able to build the project. If we wanted a full-proof solution, we would need a way to take any zig version and map it to a commit on the master branch. One way to do this would be to modify the zig version to include the "master commit height", meaning, the commit height of the merge base with master branch. There are other ways this could be done as well if we decide this is something we want to solve.

@InKryption
Copy link
Contributor

Should this maybe be in the top level doc comment? E.g. //!config min_zig_version 0.10.0-dev.2836+2360f8c49.

@nektro
Copy link
Contributor

nektro commented Jul 5, 2022

std.ZigVersion should be removed, they're already std.SemanticVersions

@marler8997
Copy link
Contributor Author

marler8997 commented Jul 5, 2022

std.ZigVersion uses SemanticVersiom during parsing, then goes a step further to extract the commit height and sha. Using Semantic version alone would mean needing to reparse these pieces each time a comparison is done, and allocating extra storage alongside the SemanticVersiom to hold the underlying string components. Having to manage this extra allocation would also likely result in a type that wraps SemanticVersion and holds a reference to the memory used to store the commit/height and sha, and would also complicate the readConfig function to take an allocator and the resulting Config type to implement a deinit function to free the results. Means a more complex API, more complex implementation, more lines of code, and redoing the work to parse the commit height each time it's needed. Unless you had a design in mind I'm not seeing?

@ominitay
Copy link
Contributor

ominitay commented Jul 5, 2022

Would it not make more sense to check the minimum version from lib/build_runner.zig? Then you can just use a const min_zig_version: ZigVersion = .{ .version = .{ major = 0, .minor = 9, .patch = 1 } }; anywhere in build.zig, which is more consistent with the way things like std.debug.todo and std.builtin.panic handle configuration.

@marler8997
Copy link
Contributor Author

That would be nice, except build runner can't run if zig is too old to compile build.zig.

@ominitay
Copy link
Contributor

ominitay commented Jul 5, 2022

That would be nice, except build runner can't run if zig is too old to compile build.zig.

Ah, I guess you're right. Unless there's a way to ensure the order of the comptime eval?

Edit: I'm going to have a look at this. I'm sure I can get it to work.

@andrewrk
Copy link
Member

@marler8997 could you please rebase this on latest master? My auto-rebase script failed due to conflicts.

@nektro
Copy link
Contributor

nektro commented Sep 19, 2022

request: std.ZigVersion -> std.zig.Version

@andrewrk
Copy link
Member

CI failure will be fixed by running zig fmt:

Looking for non-conforming code formatting...
../_release/staging/lib/zig/std/build.zig
../_release/staging/lib/zig/std/zig/Version.zig
../lib/std/build.zig
../lib/std/zig/Version.zig
../src/main.zig
../test/standalone/min_zig_version/build.zig

build_runner now checks build.zig for min_zig_version configuration, i.e.

```zig
//config min_zig_version 0.10.0-dev.2836+2360f8c49
```

This provides a standard for projects to document their min zig version. In
addition, if zig detects it's too old, it will print an error:

```
error: zig is too old, have 0.9.0 but build.zig has min_zig_version 0.10.0
```

Note that supporting a max_zig_version would be more difficult.  Since
zig version numbers contain the commit height, we can tell whether Zig
is "too old", but we can't tell whether zig it "too new".  If commit height
A is less than B, then commit A is too old, but if A is greater than B,
then A is not necessarily too new because it could just be a different
branch with extra commits.
@andrewrk
Copy link
Member

Transferred to this proposal: #14475

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.

5 participants