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

Do not fail to build if 'zig build-lib' etc. arguments exceed OS limits #10950

Merged
merged 8 commits into from
Feb 23, 2022
Merged

Do not fail to build if 'zig build-lib' etc. arguments exceed OS limits #10950

merged 8 commits into from
Feb 23, 2022

Conversation

slimsag
Copy link
Sponsor Contributor

@slimsag slimsag commented Feb 20, 2022

These patches fix a major issue which prevents using Zig's build system from being viable in larger projects with lots of source files on Windows (and macOS + Linux, to a lesser degree.)

In Mach engine we're seeing command line arguments to zig build-lib exceed the 32 KiB limit that Windows imposes in the CreateProcess API, due to the number of source files and compiler flags we need in order to build WebGPU/Dawn.

I've fixed this issue in three steps (best reviewed as individual commits):

  1. Refactored zig build-lib [args] args handling to use an iterator, in anticipation of response file support.
  2. Added response file support to zig build-lib @args.rsp (and build-exe, etc. friends) so you can pass them a text file of arguments instead.
  3. Changed the Builder to check if the arguments it would pass to any zig build command are >30 KiB and, if so, use e.g. a zig build-lib @zig-cache/args/<SHA2 of args> response file instead.

Fixes #10693
Fixes hexops/mach#167

This change refactors the `zig` argument handling (for `build-lib`, etc.
commands) to use a `process.argsWithAllocator` iterator instead of
directly accessing arguments via array indices. This supports the next
commit which will enable us to use a response file argument iterator
here seamlessly.

Helps #10693

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
This change enables `zig build-lib` and friends to take a response file
of command line arguments, for example:

```sh
zig build-lib @args.rsp
```

Which effectively does the same thing as this in Bash:

```sh
zig build-lib $(cat args.rsp)
```

Being able to use a file for arguments is important as one can quickly
exceed the 32 KiB limit that Windows imposes on arguments to a process.

Helps #10693

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
In Mach engine we're seeing command line arguments to `zig build-lib`
exceed the 32 KiB limit that Windows imposes, due to the number of
sources and compiler flags we must pass in order to build gpu-dawn.

This change fixes the issue by having `Builder` check if the arguments
to a `zig build-*` command are >30 KiB and, if so, writes the arguments
to a file `zig-cache/args/<SHA2 of args>`. Then the command invocation
merely becomes `zig build-lib @<that file>`.

Fixes #10693

Fixes hexops/mach#167

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
slimsag added a commit to hexops/mach that referenced this pull request Feb 21, 2022
I've fixed the issue in Zig upstream: ziglang/zig#10950

Helps #86

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
slimsag added a commit to hexops/mach that referenced this pull request Feb 21, 2022
I've fixed the issue in Zig upstream: ziglang/zig#10950

Helps #86

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
slimsag added a commit to hexops-graveyard/mach-gpu-dawn that referenced this pull request Feb 21, 2022
I've fixed the issue in Zig upstream: ziglang/zig#10950

Helps hexops/mach#86

Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
@kubkon
Copy link
Member

kubkon commented Feb 21, 2022

CreateProcess is a Win32 function, correct? Perhaps you'd be willing to investigate if using NT directly would not impose such a limit in Windows?

src/main.zig Outdated Show resolved Hide resolved
src/main.zig Outdated Show resolved Hide resolved
@slimsag
Copy link
Sponsor Contributor Author

slimsag commented Feb 21, 2022

@kubkon

Perhaps you'd be willing to investigate if using NT directly would not impose such a limit in Windows?

I was not able to find such an API, maybe it'd be something undocumented?

In any case, I should note that macOS and Linux also have limitations here (256 KiB and 256 MiB, respectively): I think it would be advisable to have response file support like this in place, as clang and others use the same approach to solve this issue.

lib/std/build.zig Outdated Show resolved Hide resolved
lib/std/build.zig Outdated Show resolved Hide resolved
src/main.zig Outdated Show resolved Hide resolved
Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
@slimsag
Copy link
Sponsor Contributor Author

slimsag commented Feb 21, 2022

Addressed all the feedback :)

src/main.zig Outdated Show resolved Hide resolved
Signed-off-by: Stephen Gutekanst <stephen@hexops.com>
@slimsag
Copy link
Sponsor Contributor Author

slimsag commented Feb 23, 2022

friendly ping :) I have some quite cool tech demos that will be blocked on this fix landing

@kubkon kubkon merged commit 136a439 into ziglang:master Feb 23, 2022
@andrewrk
Copy link
Member

andrewrk commented Feb 24, 2022

Thanks for the enhancement!

Looks like this introduce a regression in zig run:

+ /Users/runner/work/1/s/build.host/release/bin/zig run ../doc/docgen.zig -- /Users/runner/work/1/s/build.host/release/bin/zig ../doc/langref.html.in release/docs/langref.html
thread 135918 panic: expected output arg
/Users/runner/work/1/s/doc/docgen.zig:36:49: 0x100c16d2f in main (docgen)
    const out_file_name = args_it.next() orelse @panic("expected output arg");
                                                ^
/Users/runner/work/1/s/build.host/release/lib/zig/std/start.zig:561:37: 0x100c88098 in std.start.callMain (docgen)
            const result = root.main() catch |err| {
                                    ^
/Users/runner/work/1/s/build.host/release/lib/zig/std/start.zig:495:12: 0x100c27577 in std.start.callMainWithArgs (docgen)
    return @call(.{ .modifier = .always_inline }, callMain, .{});
           ^
/Users/runner/work/1/s/build.host/release/lib/zig/std/start.zig:460:12: 0x100c274b5 in std.start.main (docgen)
    return @call(.{ .modifier = .always_inline }, callMainWithArgs, .{ @intCast(usize, c_argc), c_argv, envp });
           ^

andrewrk added a commit that referenced this pull request Feb 24, 2022
This reverts commit 136a439, reversing
changes made to 9dd839b.

This broke the behavior of `zig run`.
@andrewrk
Copy link
Member

Reverted in 5ab5e2e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants