Skip to content

add zig build support for -static - #8248

Closed
elerch wants to merge 1 commit into
ziglang:masterfrom
elerch:master
Closed

add zig build support for -static#8248
elerch wants to merge 1 commit into
ziglang:masterfrom
elerch:master

Conversation

@elerch

Copy link
Copy Markdown
Contributor

This allows forcing exe output to be statically linked, similar to the pre-existing is_dynamic flag. Setting the flag will pass -static to the zig build-exe command. Usage example:

// const std = @import("std");constBuilder=@import("std").build.Builder;
pubfnbuild(b: *Builder) void {
// Standard target options allows the person running `zig build` to choose// what target to build for. Here we do not override the defaults, which// means any target is allowed, and the default is native. Other options// for restricting supported target set are available.consttarget=b.standardTargetOptions(.{});
// Standard release options allow the person running `zig build` to select// between Debug, ReleaseSafe, ReleaseFast, and ReleaseSmall.constmode=b.standardReleaseOptions();
constexe=b.addExecutable("hello", "src/main.zig");
exe.is_static=true;
exe.install();
construn_cmd=exe.run();
run_cmd.step.dependOn(b.getInstallStep());
if (b.args) |args| {
run_cmd.addArgs(args);
}
construn_step=b.step("run", "Run the app");
run_step.dependOn(&run_cmd.step);
}

@data-man

Copy link
Copy Markdown
Contributor
...exe.is_static=true;
exe.is_dynamic=true;
...

What will happen here?

@elerch

Copy link
Copy Markdown
ContributorAuthor

Good question. I tried setting -static and -dynamic on zig build-exe so I could match the behavior. However, zig build-exe doesn't have a lot of smarts in this regard.

zig build-exe -static -dynamic: results in a dynamic executable
zig build-exe -dynamic -static: results in a static executable

Since the code sets -static last, we should see a static executable produced.

I'm open to whatever behavior we think is correct here, but I thought to open the conversation, getting the flag at least accessible would be preferred. I had considered making is_dynamic optional, so setting to false would indicate static, null would be normal behavior, or true would be dynamic linking, but is_dynamic is used in a few other places and given the zig build-exe behavior I felt this was more consistent with the CLI.

@tauoverpi

Copy link
Copy Markdown
Contributor

If both are true it should just throw an error and request the user removes one of them.

I had considered making is_dynamic optional

In this case it would be better to just have an enum ?enum { static, dynamic } as it expresses the intent better.

@elerch

Copy link
Copy Markdown
ContributorAuthor

If both are true it should just throw an error and request the user removes one of them.

I think this is the crux of the question. zig build-exe does not currently throw an error if both -dynamic and -static are specified. If an error were to be thrown, it should probably do so on zig build-exe. I assume errors are bubbled up, so maybe that should be in separate PR/issue?

Alternatively, there could be a check in build.zig, but then there's (admittedly minimal) duplicated logic...

@elerch

Copy link
Copy Markdown
ContributorAuthor

I created #8258 to disallow -dynamic and -static at the CLI when calling zig build-exe. This appears to bubble up appropriately through the build mechanism as well, so I'm not sure we need a second check in std/lib/build.zig for this case.

@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.

As noted by others, this flag works more like an enum than bools so build.zig should be changed to have linkage: ?enum { static, dynamic } = null instead. Let me know if you are interested in implementing this chage.

Also sorry for taking so long with reviewing this, I'm trying to put more effort into keeping the review queue more manageable.

@elerch

Copy link
Copy Markdown
ContributorAuthor

ok - new changes @Vexu. I rebased to 0.8.0 and removed is_dynamic/is_static in favor of link_mode, which seemed more consistent. I was also able to leverage the existing std.builtin.LinkMode. The change is more involved and is now a breaking
change as it touches a public function and removes the existing is_dynamic bool. Looks like there's also a minor conflict with master (as this is based on 0.8.0 tag). Please take a look.

@Vexu

Vexu commented Jun 11, 2021

Copy link
Copy Markdown
Member

It turns that #7959 already mostly implemented having linkage as an enum making this pr unnecessary.
Once again sorry about losing your pr in the review queue, we'll try to keep the amount of open prs more manageable in the future. Hopefully this doesn't discourage you from contributing in the future.

@VexuVexu closed this Jun 11, 2021
@elerch

Copy link
Copy Markdown
ContributorAuthor

I see the changes in #7959 - I agree they look like they'll solve this issue too. Definitely not discouraged - happy to help, even if it turns out only to further the discussion along.

Sign up for freeto 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.

4 participants

@elerch@data-man@tauoverpi@Vexu