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

std.Build: add new functions to create artifacts/Step.Compile from existing module #20388

Merged
merged 16 commits into from
Dec 18, 2024

Conversation

BratishkaErik
Copy link
Contributor

Continued where @mlugg left in #18752. I will not duplicate that description here, because this PR is just a revival.

The first commit of this PR also makes a change (suggested by @andrewrk) to std.Build.Module: it now participates directly in the step graph. This allows us to do away with a bunch of dependency tracking logic there, since it's all handled by the build runner!

Unfortunately, I could not figure out how to resolve recursive and mutual dependencies between modules with this model, so I leaved this change out.

Also, since modules are now exposed to the users that run zig init, I added some small comments that explain to user what is a module and how to use it.
I wanted to add more comments that explain what is a package/artifact, how to use b.dependency() and etc., but this was going out of scope in this PR, I'm leaving it to the future. BTW I think zig init should be splitted again to zig init exe and zig init lib, it's easier to explain to users when you don't have this partial duplication.

@mlugg
Copy link
Member

mlugg commented Jun 23, 2024

Unfortunately, I could not figure out how to resolve recursive and mutual dependencies between modules with this model, so I leaved this change out.

I think that's the right move. Modeling modules as build steps would be convenient if it worked, but it's not really logically coherent, and if it causes issues (which, as you say, it does), that's our fault for misusing the abstraction!

@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch from fe3d357 to 870dcec Compare June 23, 2024 08:58
BratishkaErik added a commit to BratishkaErik/pg.zig that referenced this pull request Jun 23, 2024
Uses functions from ziglang/zig#20388 .
Also fix error with no longer existing `std.rand` namespace.

Signed-off-by: Eric Joldasov <[email protected]>
BratishkaErik added a commit to BratishkaErik/pg.zig that referenced this pull request Jun 23, 2024
Uses functions from ziglang/zig#20388 .
Also fix error with no longer existing `std.rand` namespace.

Signed-off-by: Eric Joldasov <[email protected]>
@matklad
Copy link
Contributor

matklad commented Jun 24, 2024

Nice, I needed something like this the other day for TigerBeetle --- we have a vsr module, which we'd love to use as a dependency for other modules, but which also includes a bunch of internal tests. So, ideally, we'd be able to pass the same vsr_module into other_modulue.addImport as well as b.addTest. Right now, we have to essentially duplicate vsr_module definition for the test_step.root_module module created by addTest.

@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch 3 times, most recently from 882431a to 3f052df Compare July 5, 2024 18:22
@jeffbdavenport
Copy link

This would be a convenient addition

@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch 4 times, most recently from e9fee04 to bf6d26f Compare July 15, 2024 14:28
@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch 4 times, most recently from bdbc671 to bc74600 Compare July 23, 2024 19:42
@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch 3 times, most recently from 65e8d80 to ba59cec Compare August 1, 2024 19:58
@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch 3 times, most recently from 9bcdd13 to be48c95 Compare August 12, 2024 11:20
@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch 5 times, most recently from 0c9c6ce to 1c5cae4 Compare August 20, 2024 11:26
@BratishkaErik BratishkaErik force-pushed the std.Build/accept-root-module-2 branch from eb64e26 to afc77f0 Compare December 17, 2024 20:49
@mlugg
Copy link
Member

mlugg commented Dec 18, 2024

@BratishkaErik, thanks for doing the finishing touches here.

@andrewrk, are you happy for me to merge this? I've thoroughly reviewed the work locally (refactoring and reworking some parts) and applied the changes we discussed, so I'm happy with it, but figured I should check with you.

@andrewrk
Copy link
Member

andrewrk commented Dec 18, 2024

I'm probably going to say "yes" but could you make it easier on me by typing up the corresponding release notes for this PR? That will make it very easy to evaluate, plus we need those if it is merged. (also make sure the "breaking" label is correctly applied, or not)

@mlugg
Copy link
Member

mlugg commented Dec 18, 2024

Sure thing -- here are the release notes. They're pretty similar to the ones I wrote up above, just modified for the API we ended up implementing.

Release Notes

Zig 0.14.0 modifies the build system APIs for creating Compile steps, allowing them to be created from existing std.Build.Module objects. This allows for module graphs to be defined in a more clear manner, and for components of these graphs to be reused more easily; for instance, a module which exists as a dependency of another can easily have a corresponding test step created. The new APIs can be used by modifying your calls to addExecutable, addTest, etc. Instead of passing options like root_source_file, target, and optimize directly to these functions, you should pass in the root_module field a *std.Build.Module created using these parameters. Zig 0.14.0 still permits the old, deprecated usages for these functions; the next release will remove them.

Users of the legacy APIs can upgrade with minimal effort by just moving the module-specific parts of their addExecutable (etc) call into a createModule call. For instance, here is the updated version of a simple build script:

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

    const exe = b.addExecutable(.{
        .name = "hello",
        .root_source_file = b.path("src/main.zig"),
        .target = target,
        .optimize = optimize,
    });
    b.installArtifact(exe);
}
const std = @import("std");

-->

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

    const exe = b.addExecutable(.{
        .name = "hello",
        .root_module = b.createModule(.{ // this line was added
            .root_source_file = b.path("src/main.zig"),
            .target = target,
            .optimize = optimize,
        }), // this line was added
    });
    b.installArtifact(exe);
}
const std = @import("std");

And, to demostrate the benefits of the new API, here is an example build script which elegantly constructs a complex build graph of multiple modules:

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

    // First, we create our 3 modules.

    const foo = b.createModule(.{
        .root_source_file = b.path("src/foo.zig"),
        .target = target,
        .optimize = optimize,
    });
    const bar = b.createModule(.{
        .root_source_file = b.path("src/bar.zig"),
        .target = target,
        .optimize = optimize,
    });
    const qux = b.createModule(.{
        .root_source_file = b.path("src/qux.zig"),
        .target = target,
        .optimize = optimize,
    });

    // Next, we set up all of their dependencies.

    foo.addImport("bar", bar);
    foo.addImport("qux", qux);
    bar.addImport("qux", qux);
    qux.addImport("bar", bar); // mutual recursion!

    // Finally, we will create all of our `Compile` steps.
    // `foo` will be the root of an executable, but all 3 modules also have unit tests we want to run.

    const foo_exe = b.addExecutable(.{
        .name = "foo",
        .root_module = foo,
    });

    b.installArtifact(foo_exe);

    const foo_test = b.addTest(.{
        .name = "foo",
        .root_module = foo,
    });
    const bar_test = b.addTest(.{
        .name = "bar",
        .root_module = bar,
    });
    const qux_test = b.addTest(.{
        .name = "qux",
        .root_module = qux,
    });

    const test_step = b.step("test", "Run all unit tests");
    test_step.dependOn(&b.addRunArtifact(foo_test).step);
    test_step.dependOn(&b.addRunArtifact(bar_test).step);
    test_step.dependOn(&b.addRunArtifact(qux_test).step);
}
const std = @import("std");

@andrewrk
Copy link
Member

Oops, sorry for the redundant request. I hate that "show hidden items" UI on GitHub

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

OK yeah I remember this now. Thanks for seeing it through!

@mlugg mlugg added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes. labels Dec 18, 2024
@mlugg
Copy link
Member

mlugg commented Dec 18, 2024

I've applied "breaking" because:

  • While this change is currently backwards-compatible, we will be removing this compatibility in future
  • There are some minor unavoidable API breakages in here, such as std.Build.Step.Compile.root_module now being a pointer

@mlugg mlugg merged commit 12d64c4 into ziglang:master Dec 18, 2024
10 checks passed
@BratishkaErik
Copy link
Contributor Author

Thanks! 🥳

@BratishkaErik BratishkaErik deleted the std.Build/accept-root-module-2 branch December 18, 2024 03:40
AndrewKraevskii added a commit to AndrewKraevskii/dcimgui that referenced this pull request Dec 18, 2024
b.host was deprecated in 0.13.0 and removed in ziglang/zig#20388
andrewrk added a commit to andrewrk/glslang-zig that referenced this pull request Dec 23, 2024
kassane added a commit to kassane/openssl-zig that referenced this pull request Dec 24, 2024
@andrewrk
Copy link
Member

Release notes neglect to mention defineCMacro being deleted

@mlugg
Copy link
Member

mlugg commented Jan 13, 2025

defineCMacro had been deprecated (and hence slated for removal) in a prior release cycle. My understanding is that we only mention these things in the release where they're initially deprecated.

@andrewrk
Copy link
Member

It should definitely be mentioned when it is deleted so that people can search for the missing function in the release notes and find out what to do. That's the main purpose of the release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior. release notes This PR should be mentioned in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants