Skip to content

Report error on missing values for addConfigHeader #20234

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

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

MrDmitry
Copy link
Contributor

@MrDmitry MrDmitry commented Jun 8, 2024

Observed behavior:

> ls
build.zig  foo.h.in

> cat foo.h.in
#define a @b@

> cat build.zig
const std = @import("std");

pub fn build(b: *std.Build) void {
    const out_h = b.addConfigHeader(.{
        .style = .{ .cmake = b.path("foo.h.in") },
    }, .{});
    const install_file = b.addInstallFileWithDir(out_h.getOutput(), .header, "foo.h");
    b.getInstallStep().dependOn(&install_file.step);
}

> zig build
install
└─ install generated to foo.h
   └─ configure cmake header foo.h.in to foo.h failure
error: /home/dmitry/zig/20230/foo.h.in:1: unable to substitute variable: error: MissingValue
error: HeaderConfigFailed
Build Summary: 0/3 steps succeeded; 1 failed (disable with --summary none)
install transitive failure
└─ install generated to foo.h transitive failure
   └─ configure cmake header foo.h.in to foo.h failure
error: the following build command failed with exit code 1:
...

The output does not exactly match the expected behavior from #20230, but it is consistent with how other ConfigHeader functions propagate errors.

Tests are updated to match the new functionality.
Standalone tests are modified to stay consistent with CMake behavior. Since this new behavior is different, I removed the conflicting cases.

Closes #20230

@MrDmitry MrDmitry marked this pull request as ready for review June 8, 2024 15:27
@andrewrk
Copy link
Member

andrewrk commented Jun 8, 2024

Well, it's certainly an improvement. It would be a lot more useful if it would tell you the names of the missing names though. Thanks!

@andrewrk andrewrk merged commit 84d1580 into ziglang:master Jun 8, 2024
10 checks passed
@andrewrk andrewrk added zig build system std.Build, the build runner, `zig build` subcommand, package management release notes This PR should be mentioned in the release notes. labels Jun 8, 2024
@MrDmitry
Copy link
Contributor Author

MrDmitry commented Jun 8, 2024

Yeah, I'm experimenting with reporting diagnostics. I should have something in the coming weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes This PR should be mentioned in the release notes. zig build system std.Build, the build runner, `zig build` subcommand, package management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addConfigHeader stopped reporting error for missing property names
2 participants