Skip to content

Conversation

@lacc97
Copy link
Contributor

@lacc97 lacc97 commented Sep 27, 2023

This MR adds an optional field in the blank Style to allow for overriding the default header guard that is generated from the output file path.

i.e. now you can do

b.addConfigHeader(.{
        .style = .{ .blank = .{ .include_guard_override = "MY_PREFIXED_CONFIG_H" } },
        .include_path = "config.h",
    }, .{});

to get

/* This file was generated by ConfigHeader using the Zig Build System. */
#ifndef MY_PREFIXED_CONFIG_H
#define MY_PREFIXED_CONFIG_H

// ...

#endif /* MY_PREFIXED_CONFIG_H */

The motivation for this is the case when multiple dependencies generate the same config.h with the same include guard, which ends up causing problems when both headers are included in the same translation unit. With this option, there is an escape hatch to define an include guard with a name that is less likely to conflict.

A minor source incompatibility of this MR is that you can't specify the blank Style only with the enum literal, but must now coerce from the empty tuple (i.e. .{}, I'm not 100% familiar with the terminology yet 😅).
i.e. if you don't want the override, you now have to do

_ = b.addConfigHeader(.{.style = .{.blank = .{}}}, .{})

instead of

_ = b.addConfigHeader(.{ .style = .blank }, .{});

Not sure how to avoid this though. The alternative is putting this field in the main Options struct but that doesn't seem appropriate because the override only matters for the blank Style.

This commit adds an optional field in the blank Style to allow for
overriding the default header guard that is generated from the output
file path.
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.

Thanks for working on this. I'd like to counter propose adding the new option instead to Options so that it is backwards compatible with existing build scripts, and then it can be an option that only applies to blank currently.

As suggested by Andrew, this commit moves the guard override to the
Options struct such that existing build scripts remain source backwards
compatible.
@lacc97 lacc97 requested a review from andrewrk September 28, 2023 00:10
@lacc97
Copy link
Contributor Author

lacc97 commented Sep 28, 2023

Moved the include guard override to the Options struct and the Step.ConfigHeader struct instead of the Style union. This should make the changes backwards compatible now.

i.e.

b.addConfigHeader(.{
        .style = .blank = .{  },
        .include_path = "config.h",
        .include_guard_override = "MY_PREFIXED_CONFIG_H"
    }, .{});

@andrewrk
Copy link
Member

Looks great, thanks!

@andrewrk andrewrk merged commit acac685 into ziglang:master Sep 28, 2023
@lacc97 lacc97 deleted the config-header-include-guard-override branch September 28, 2023 23:34
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.

2 participants