Skip to content

provide buildpkgs module to build.zig #8072

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

Closed
wants to merge 8 commits into from

Conversation

marler8997
Copy link
Contributor

@marler8997 marler8997 commented Feb 25, 2021

Alternative to #8033 (add @tryImport)

implement the main part of #8070 (Dynamic build.zig dependencies)

See the linked proposal for more details. To summarize what this PR includes. zig build has been modified to generate a corresponding buildpkgs.zig module for any build.zig that it compiles (also one for all its sub-packages). This buildpkgs.zig module contains a list of all the packages that are available to build.zig. This allows build.zig to know at comptime whether a package is available, before it attempts to import it. The linked issue above explains why this is necessary for build.zig.

implements one part of ziglang#8070 (Dynamic build.zig dependencies)
@marler8997 marler8997 force-pushed the buildpkgs branch 4 times, most recently from aec0f13 to de6ee41 Compare February 26, 2021 14:22
The reason we want to force buildpkgs.has to be comptime is it is meant to be surrounding a block that contains an @import, so if a user accidently forgets to mark it as comptime then their build.zig will be invalid, but this error will only be caught with certain configurations.  I've marked @import("buildpkgs").has with callconv(.Inline) which should force it to be evaluated at comptime, but it looks like this behavior is not implemented yet.
buildpkgs.zig is small and generating it should be relatively cheap.  So we can just generate the hash used to generate it's file path based on the contents.  This means that if zig updates and changes how it generates buildpkgs.zig, it will get a new hash.
@marler8997 marler8997 force-pushed the buildpkgs branch 4 times, most recently from 580c325 to 2180b28 Compare February 26, 2021 16:52
@marler8997 marler8997 force-pushed the buildpkgs branch 2 times, most recently from 32d42a3 to 819d26d Compare February 26, 2021 18:06
Copy link
Contributor

@daurnimator daurnimator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on implementation. I still need to think about the design.

\\};
\\
\\// using .Inline to force this to be comptime
\\pub fn has(comptime name: []const u8) callconv(.Inline) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the below inline for uses names rather than name

Copy link
Contributor Author

@marler8997 marler8997 Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's supposed to be names, names is declared above

try buffer.writer().writeAll(
\\};
\\
\\// using .Inline to force this to be comptime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? The parameter is already comptime known, why must the mem.eql happen at comptime?

Copy link
Contributor Author

@marler8997 marler8997 Feb 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the doc comment in test/standalone/buildpkgs/missing-comptime/build.zig.

//! Currently the user MUST specify comptime when calling "buildpkgs.has"
//! until #425 is implemented.
//! This build.zig file tries to call "buildpkgs.has" without comptime and the
//! test is to make sure it produces a compile error.
//!
//! Note that the reason why "buildpkgs.has" must be evaluated at comptime is
//! because it will always surround an @import statement. The problem is
//! that if they forget to add "comptime" to their call, then their build.zig
//! file will "sometimes work" so long as they are building with the necessary
//! packages configured, but then it will fail once the @import is missing
//! which defeats the whole purpose of providing "buildpkgs.has" in the first place.
//!

This is one disadvantage this solution currently has over @tryImport. If the user forgets to add comptime, then Zig evaluates it at runtime becuase buildpkgs.has returns a bool type. This is being addressed with #425 which allows you to force comptime with callconv(.Inline). With @tryImport it returns ?type so it has to be evaluated at comptime so there's no issue. The real problem though is that missing comptime won't always result in a compile error, if you're only testing cases where the package is supplied then it will just work and you'll miss the bug. This code makes sure that you always get a compile error when you forget comptime instead of leaving a bug in your code.

Note that the other solution I thought of was to create a tryImport wrapper around the has function in buildpkgs.zig:

pub fn tryImport(comptime name: []const u8) ?type {
    return if (comptime has(name)) @import(name) else null;
}

However, because of this accepted proposal: #2206, all @import calls will require string literals, so this won't work once that is implemented. I'm am a bit dissapointed that we won't be able to expose a tryImport because it means the user must specify the same package name twice, which provides more opportunity for errors (espcially copy/paste one):

if (comptime @import("buildpkgs").has("foo")) {
    const foo = @import("foo");
}

// VS

if (@import("buildpkgs").tryImport("foo")) |foo| {
}

As you can see, tryImport has 2 advantages, the first is you don't have to maintain 2 copies of the same package name in your code, and the second is you're no longer required to specify comptime when you call it because it returns a comptime value, ?type.

@Vexu Vexu added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Mar 7, 2021
@Vexu
Copy link
Member

Vexu commented Jun 10, 2021

I'll close this pr for now since we want to reserve the pull request queue for prs that are ready to be merged. I'll add a reminder to the issue about this implementation.

@Vexu Vexu closed this Jun 10, 2021
@Vexu Vexu mentioned this pull request Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants