Skip to content

Make build.zig ready for generated files - #7959

Merged
Vexu merged 9 commits into
ziglang:masterfrom
ikskuh:build_rewrite
Jun 11, 2021
Merged

Make build.zig ready for generated files#7959
Vexu merged 9 commits into
ziglang:masterfrom
ikskuh:build_rewrite

Conversation

@ikskuh

@ikskuhikskuh commented Feb 5, 2021

Copy link
Copy Markdown
Contributor

First of all: This is a breaking PR which will break all projects using std.build.Pkg. This PR is a draft as well.

Goal: This PR tries to improve the experience with generated files by various build steps.

Right now, std.build.FileSource is used in some places to allow either a std.build.WriteFileStep or std.build.TranslateC to generate files and use them to create executables and libraries. This PR replaces the special cases with a generic std.build.GeneratedFile which is a @fieldParentPtr style interface that allows to retrieve the path of a generated file after its make() invocation in a general purpose manner.

Usage example:

conststd=@import("std");
constTemplateStep=@import("src/TemplateStep.zig");
pubfnbuild(b: *std.build.Builder) void {
consttarget=b.standardTargetOptions(.{});
constmode=b.standardReleaseOptions();
// An arbitrary step that generates a output fileconsttemplate_step=TemplateStep.create(b, std.build.FileSource{
.path="example/layout.ztt",
});
constexe=b.addExecutable("demo", "example/main.zig");
exe.addPackage(std.build.Pkg{
.name="template",
.path=template_step.getFileSource(), // we can now add the generated file also as a package source
});
exe.setTarget(target);
exe.setBuildMode(mode);
exe.install();
}

Right now, this PR is unfinished as i first want some feedback on the changes i did and if we want to pursue this change.

Waiting for a response!
– xq

@ikskuhikskuh changed the title Make build.zigMake build.zig ready for generated filesFeb 5, 2021
@daurnimatordaurnimator added the zig build system std.Build, the build runner, `zig build` subcommand, package management label Feb 5, 2021
@VexuVexu added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Feb 5, 2021
@ikskuh

Copy link
Copy Markdown
ContributorAuthor

I get the following error in the CI:

Install the project...
-- Install configuration: "Release"
-- Installing: /usr/home/build/zig/build/release/bin/zig
-- Installing: /usr/home/build/zig/build/release/lib
./build.zig:301:8: error: expected type 'std.build.FileSource', found '[]u8'
}) catch unreachable);
^
./lib/std/build.zig:1222:24: note: std.build.FileSource declared here
pub const FileSource = union(enum) {
^
./build.zig:124:13: note: referenced here
try addCmakeCfgOptionsToExe(b, cfg, tracy, exe);
^
./lib/std/special/build_runner.zig:157:24: note: referenced here
.ErrorUnion => try root.build(builder),
^

How can i reproduce this locally?

@ikskuh

Copy link
Copy Markdown
ContributorAuthor

Okay, so the command failing was:

./build/zig build --override-lib-dir ./lib -Dlib-files-only --prefix ./release -Dconfig_h=./build/config.h install

I fixed the bugs happening with this

@ikskuh
ikskuh marked this pull request as ready for review February 23, 2021 20:41
Comment threadlib/std/build.zig Outdated
Comment threadlib/std/build.zig
@SpexGuy

Copy link
Copy Markdown
Contributor

Overall these are good changes, but I think they are missing a major use case:
Some file generation processes are expensive, or invalidate downstream results that would otherwise be cached. It would be nice if generated files could declare their dependencies and check if the generation process needs to be re-done. This is more of a wishlist thing for me, I don't have specific ideas on how this interface should look. But I think build system support for generated files would be incomplete without this feature.

One thing to note, that might help with this feature, is that for most processes you don't need to check if any of the current dependencies have changed, it is sufficient to only check if any of the previous dependencies have changed. So you could have the list of dependencies be an output of the make step, and save that list of files in the cache to be checked on the next run. This eliminates needing to write separate pieces of code for doing the build vs examining dependencies.

@andrewrk

Copy link
Copy Markdown
Member

I agree with @SpexGuy but I think that the build system supporting a general purpose caching system is something we need regardless, and orthogonal to this PR. It is certainly a glaring missing feature, however it is something that can be plugged into these changes later, and also will need to be plugged into the other build steps that do not rely on the internal caching of the zig compiler.

@ikskuh

Copy link
Copy Markdown
ContributorAuthor

Another thing that i left out (for now) was that LibExeObjStep does not provide a file source. But that's kinda not right. I think i should change that as well, so we can just use the output of a compile step as well, for example to push it directly into objdump or objcopy

@ikskuh
ikskuhforce-pushed the build_rewrite branch 2 times, most recently from ae0fbda to fabe2daCompareFebruary 26, 2021 18:51
@ikskuh

Copy link
Copy Markdown
ContributorAuthor

Another thing that i left out (for now) was that LibExeObjStep does not provide a file source. But that's kinda not right. I think i should change that as well, so we can just use the output of a compile step as well, for example to push it directly into objdump or objcopy

I tried to change the LibExeObjStep to use the GeneratedFile instead, but i can't figure out the right points where to actually set the .path property for those files. @andrewrk i talked to you on Discord about this already, you might need to take a look at it. Apart from that, i'm ready for merging i think

@ikskuh

Copy link
Copy Markdown
ContributorAuthor

I am confused. On my local machine, zig build --override-lib-dir lib test-toolchain runs clean.

Note here: I haven't built zig with the changes in the build system, but to my understanding this should not change anything.

@ikskuh

Copy link
Copy Markdown
ContributorAuthor

I found some hints, will investigate. Fixed a bug already 😁

@ikskuh

Copy link
Copy Markdown
ContributorAuthor

CI is green (latest commit just deletes a file that appeared during the rebase), so i'm ready to merge. @SpexGuy, can you review the latest state again? 🤔

@SpexGuySpexGuy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall this looks good. Does build.zig in the root of the repository also need to be updated?

Comment threadlib/std/build.zig Outdated
Comment threadlib/std/build.zig Outdated
Comment threadlib/std/build.zig Outdated
@ikskuh

Copy link
Copy Markdown
ContributorAuthor

Overall this looks good. Does build.zig in the root of the repository also need to be updated?

No, the CI already tested this including all build scripts in the tests folder 😃 It's less breaking than i expected, which is good

@ikskuh
ikskuhforce-pushed the build_rewrite branch 2 times, most recently from f6359f9 to d545f08CompareMay 24, 2021 17:39
@ikskuh

Copy link
Copy Markdown
ContributorAuthor

Rebased once again

@VexuVexu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, made all the remaining enums snake_case while we're breaking everything.

@Vexu
Vexu merged commit c5d4122 into ziglang:masterJun 11, 2021
@ikskuh
ikskuh deleted the build_rewrite branch June 11, 2021 16:17
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breakingImplementing this issue could cause existing code to no longer compile or have different behavior.zig build systemstd.Build, the build runner, `zig build` subcommand, package management

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ikskuh@SpexGuy@andrewrk@Vexu@daurnimator