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

Allow packages to access build_options #9623

Merged
merged 6 commits into from Aug 26, 2021
Merged

Allow packages to access build_options #9623

merged 6 commits into from Aug 26, 2021

Conversation

leecannon
Copy link
Contributor

@leecannon leecannon commented Aug 25, 2021

I'm currently getting around this limitation by using an ugly hack

const build_option_file = std.fmt.allocPrint(exe.builder.allocator, "zig-cache/{s}_build_options.zig", .{exe.name}) catch unreachable;

const git_pkg = std.build.Pkg{
    .name = "git",
    .path = .{ .path = prefix_path ++ "src/git.zig" },
    .dependencies = &[_]std.build.Pkg{
        .{ .name = "build_options", .path = .{ .path = build_option_file } },
    },
};

this will fix #5375

@MasterQ32
Copy link
Contributor

Imho this is a anti-pattern and build_options should be removed from the LibExeObjStep completly.

The nicer solution here is to create a std.build.BuildOptions which provides a std.build.FileSource that you can pull into any package like this:

pub fn build(b: *Builder) !void {
    const build_options = b.createBuildOptions();
    build_options.add([]const u8, string, "This is a string");
    
    const git_pkg = std.build.Pkg{
        .name = "git",
        .path = .{ .path = prefix_path ++ "src/git.zig" },
        .dependencies = &[_]std.build.Pkg{
            .{ .name = "build_options", .path = build_options.getSource() },
        },
    };
}

With this, you can make as many build_options packages as you want, share them between packages or even whole compilations, and you are not dependent on hacks or the enforcement of all packages sharing build_options (which would not work as soon as two build options conflict)

@leecannon
Copy link
Contributor Author

I've mostly implemented the suggested change, however ive run into something im not 100% happy with.

Status quo outputs build options to a file called {executable_name}_build_options.zig, decoupling build options from the LibExeObjStep means that the caller must pass a name i.e. const build_options = b.addBuildOptions("build_options");

I will push the changes soon for review.

@MasterQ32
Copy link
Contributor

I've mostly implemented the suggested change, however ive run into something im not 100% happy with.

Status quo outputs build options to a file called {executable_name}_build_options.zig, decoupling build options from the LibExeObjStep means that the caller must pass a name i.e. const build_options = b.addBuildOptions("build_options");

I will push the changes soon for review.

Just don't care. Output the build options into zig-cache/build_options/${shasum}/build_options.zig where shasum is the sum over your file content (similar to how the zig-cache does it).

This allows you to have guaranteed non-conflicting build option files

@leecannon
Copy link
Contributor Author

leecannon commented Aug 25, 2021

While this is an improvement in terms of flexibility, using the newer version of the API in the use case I needed this for has made the code much more messy.

Is it actually worth the breaking change for everyone currently using build_options? whereas the initial commit in this PR would not impact anyone except the people already working around the issue that it fixes.

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.

Is it actually worth the breaking change for everyone currently using build_options?

Yes - the way I originally coded build_options meant that at some point we would have to break things in a messy way like this. I apologize for that. But the sooner the better.

Although I have some requested changes, I'm really glad that you're making these long overdue changes. Thank you for taking the time to do this.

Side note-

using the newer version of the API in the use case I needed this for has made the code much more messy.

may I see?

build.zig Outdated Show resolved Hide resolved
build.zig Outdated Show resolved Hide resolved
lib/std/build.zig Outdated Show resolved Hide resolved
lib/std/build.zig Outdated Show resolved Hide resolved
lib/std/build.zig Outdated Show resolved Hide resolved
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.

Brilliant, thank you!

@andrewrk andrewrk merged commit 311797f into ziglang:master Aug 26, 2021
@andrewrk andrewrk 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 Aug 26, 2021
@andrewrk andrewrk added this to the 0.9.0 milestone Apr 18, 2024
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.

addBuildOption results in error: unable to find 'build_options'
3 participants