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

[build] Zig emscripten build broken? #4010

Closed
3 of 4 tasks
fjebaker opened this issue May 28, 2024 · 5 comments · Fixed by #4012
Closed
3 of 4 tasks

[build] Zig emscripten build broken? #4010

fjebaker opened this issue May 28, 2024 · 5 comments · Fixed by #4012
Labels
help needed - please! I need help with this issue

Comments

@fjebaker
Copy link

fjebaker commented May 28, 2024

  • I tested it on latest raylib version from master branch
  • I checked there is no similar issue already reported
  • I checked the documentation on the wiki
  • My code has no errors or misuse of raylib

Issue description

I think #3983 broke the Zig 0.12.0 build. If I pass --sysroot with an absolute path I now get

thread 637865 panic: sub_path is expected to be relative to the build root, but was this absolute path: '/home/lilith/Developer/emsdk/upstream/emscripten/cache/sysroot/include'. It is best avoid absolute paths, but if you must, it is supported by LazyPath.cwd_relative

If I pass --sysroot with relative paths

thread 637149 panic: reached unreachable code
/home/lilith/.zigup/cache/0.12.0/files/lib/std/debug.zig:403:14: 0x10b5bfd in assert (build)
    if (!ok) unreachable; // assertion failure
             ^
/home/lilith/.zigup/cache/0.12.0/files/lib/std/fs.zig:280:11: 0x10f0b3a in openDirAbsolute (build)
    assert(path.isAbsolute(absolute_path));
          ^

due to the std.fs.openAbsoluteDir verifying that a path is absolute or not. I think the changes in #3983 are well intended but are maybe too soon. Upgrading to Zig 0.13.0 when it is released will likely require more work than just changing one line...

I have a commit in a fork I am happy to PR that just reverts the change in #3983. I have tested it and it seems to work fine.

Edit: to suggest a compromise, one could also use something other than openAbsoluteDir to keep some version compat with newer 0.13.0-dev releases.

Environment

Cloned master branch and built with Zig version 0.12.0 with

zig build -Dtarget=wasm32-emscripten --sysroot "/home/lilith/Developer/emsdk/upstream/emscripten"
@raysan5
Copy link
Owner

raysan5 commented May 29, 2024

@fjebaker Thanks for reporting! Probably @CosmicBagel should comment on this issue.

@raysan5 raysan5 changed the title [build.zig] emscripten build broken? [build] Zig emscripten build broken? May 29, 2024
@raysan5 raysan5 added the help needed - please! I need help with this issue label May 29, 2024
@dylanlangston
Copy link
Contributor

I'm also seeing this issue in my project and agree with @fjebaker assessment of what's happening. The problem arises due to changes in the Zig master branch where the path to the emsdk headers must be a relative path, as mentioned here. Previously, in versions >= 0.12, this path was absolute.

This code in the src/build.zig is checking that the path is absolute:

var dir = std.fs.openDirAbsolute(cache_include, std.fs.Dir.OpenDirOptions{ .access_sub_paths = true, .no_follow = true }) catch @panic("No emscripten cache. Generate it!");
dir.close();

However, we need to pass a relative path to b.path(cache_include) to avoid the error sub_path is expected to be relative to the build root, but was this absolute path:

raylib.addIncludePath(b.path(cache_include));

Ideally, I'd like to be able to use raylib's master branch with Zig's master branch. To resolve this in a way that doesn't break existing code but enables new development also I propose something like this:

if (comptime builtin.zig_version.minor > 12) {
    var dir = std.fs.cwd().openDir(cache_include, std.fs.Dir.OpenDirOptions{ .access_sub_paths = true, .no_follow = true }) catch @panic("No emscripten cache. Generate it!");
    dir.close();
    raylib.addIncludePath(b.path(cache_include));
} else {
    var dir = std.fs.openDirAbsolute(cache_include, std.fs.Dir.OpenDirOptions{ .access_sub_paths = true, .no_follow = true }) catch @panic("No emscripten cache. Generate it!");
    dir.close();
    raylib.addIncludePath(.{ .path = cache_include });
}

I've made those changes in a fork here, happy to submit a pull request if others agree this change makes sense.

raysan5 pushed a commit that referenced this issue May 29, 2024
* Fix for issue #4010

Split the code for Zig's master branch and >= 0.12.0 due to changes in ziglang/zig#19623

* Restore the cache_include path which was removed in error

Accidently removed a couple lines I didn't mean to 🙈
@CosmicBagel
Copy link
Contributor

My apologies for breaking this for you, I should have tested this code path properly with 0.12.0 before submitting the PR, that's on me.

Indeed, zig is making a push to eliminate all absolute paths in the build system it seems ziglang/zig#18450
Context: andrewrk is the BDFL for zig, so if he's pushing to have it happen, it will most likely happen.

I like the approach dylan proposed here, the only thing I would like to add is maybe a warning that gets printed when using zig 0.12.0 so that people will be aware of future breakage when upgrading to zig 0.13.0. I'm assuming people have little scripts and shell aliases, shell variables, ect, that point to this.

@CosmicBagel
Copy link
Contributor

Ah dang, I was too slow

@fjebaker
Copy link
Author

Thanks @dylanlangston and @CosmicBagel, the changes merged work for me. Thanks for addressing this so quickly!

Context: andrewrk is the BDFL for zig, so if he's pushing to have it happen, it will most likely happen.

I absolutely love the direction Andrew is taking Zig in! Until the language has arrived at its stable API, these are just the little hacks we'll have to put up with (~;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help needed - please! I need help with this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants