raylib icon indicating copy to clipboard operation
raylib copied to clipboard

build.zig: Fix various issues around `-Dconfig`

Open sagehane opened this issue 1 year ago • 5 comments

Here is a PR with issues 1, 2, and 3 from https://github.com/raysan5/raylib/pull/4393#issuecomment-2423777267 addressed.

I didn't bother implementing 4 because there seemed to be multiple possible solutions to it, the way I originally attempted it was quite hacky and caused breaking changes.

Breaking changes: Now, valid flags like "-x c++" will be treated as "-x" "c++" and thus behave incorrectly. I found this to be a non-issue as Zig currently doesn't accept those flags, and I believe "-xc++" is a valid alternative. I don't know enough C to know if there are other possibly useful flags that often come with spaces between.

sagehane avatar Oct 20 '24 02:10 sagehane

~~> Here is a PR with issues 1, 2, and 3 from #4393 (comment) addressed.~~

~~It's worth noting that issues 1 and 2 (the dependency problem) still occur when using @embedFile() over pathFromRootDir(). But since the goal of this commit is purely to resolve the -Dconfig problems, not the dependency problem, I think it's perfectly fine as-is. Not to mention, one can just uncomment those two lines should the need arise.~~

My bad was looking at the wrong comment, gotta reread before I actually respond to this

Also, if you don't mind explaining, could I ask how @embedFile() "bloats the size of the binary"? I'm reading the Zig documentation and it sounds like the bloat comes from creating a very long string, is that right?

Yerdun avatar Oct 20 '24 03:10 Yerdun

Wait, are you still experiencing breakage for your use-case?

Also, if you don't mind explaining, could I ask how @embedFile() "bloats the size of the binary"?

Yes, if you have any values defined at comptime and is used at runtime, it will be added to the binary. Since src/config.h is about 16 KiB, it means any program embedding it AND using the value at runtime should become 16 KiB larger. Note, the "program" here is the build system and it will have no result in the final binary people should care about.

Anyhow, I thought of something that seems okay to me. Should be far smaller than embedding the entire file; also probably easier to debug. I need to check if this actually does reduce the binary size compared to just using @embedFile though. Maybe debug mode might not be smart enough to optimise it out.

/// A list of all flags from `src/config.h` that one may override
const config_h_flags = outer: {
    // Set this value higher if compile errors happen as `src/config.h` gets larger
    @setEvalBranchQuota(1 << 20);

    const config_h = @embedFile("config.h");
    var flags: []const []const u8 = &.{};

    var lines = std.mem.tokenizeScalar(u8, config_h, '\n');
    while (lines.next()) |line| {
        if (!std.mem.containsAtLeast(u8, line, 1, "SUPPORT")) continue;
        if (std.mem.startsWith(u8, line, "//")) continue;
        if (std.mem.startsWith(u8, line, "#if")) continue;

        var flag = std.mem.trimLeft(u8, line, " \t"); // Trim whitespace
        flag = flag["#define ".len - 1 ..]; // Remove #define
        flag = std.mem.trimLeft(u8, flag, " \t"); // Trim whitespace
        flag = flag[0 .. std.mem.indexOf(u8, flag, " ") orelse continue]; // Flag is only one word, so capture till space
        flag = "-D" ++ flag; // Prepend with -D

        flags = @as([]const []const u8, flags ++ [1][]const u8{flag});
    }

    @compileLog(flags);
    break :outer flags;
};
$ zig build
/home/plumeus/repos/raylib/zig_patch_2/src/build.zig:98:5: error: found compile log statement
    @compileLog(flags);
    ^~~~~~~~~~~~~~~~~~

Compile Log Output:
@as([]const []const u8, &.{ "-DSUPPORT_MODULE_RSHAPES"[0..24], "-DSUPPORT_MODULE_RTEXTURES"[0..26], "-DSUPPORT_MODULE_RTEXT"[0..22], "-DSUPPORT_MODULE_RMODELS"[0..24], "-DSUPPORT_MODULE_RAUDIO"[0..23], "-DSUPPORT_CAMERA_SYSTEM"[0..23], "-DSUPPORT_GESTURES_SYSTEM"[0..25], "-DSUPPORT_RPRAND_GENERATOR"[0..26], "-DSUPPORT_MOUSE_GESTURES"[0..24], "-DSUPPORT_SSH_KEYBOARD_RPI"[0..26], "-DSUPPORT_WINMM_HIGHRES_TIMER"[0..29], "-DSUPPORT_PARTIALBUSY_WAIT_LOOP"[0..31], "-DSUPPORT_SCREEN_CAPTURE"[0..24], "-DSUPPORT_GIF_RECORDING"[0..23], "-DSUPPORT_COMPRESSION_API"[0..25], "-DSUPPORT_AUTOMATION_EVENTS"[0..27], "-DRL_SUPPORT_MESH_GPU_SKINNING"[0..30], "-DSUPPORT_QUADS_DRAW_MODE"[0..25], "-DSUPPORT_FILEFORMAT_PNG"[0..24], "-DSUPPORT_FILEFORMAT_GIF"[0..24], "-DSUPPORT_FILEFORMAT_QOI"[0..24], "-DSUPPORT_FILEFORMAT_DDS"[0..24], "-DSUPPORT_IMAGE_EXPORT"[0..22], "-DSUPPORT_IMAGE_GENERATION"[0..26], "-DSUPPORT_IMAGE_MANIPULATION"[0..28], "-DSUPPORT_DEFAULT_FONT"[0..22], "-DSUPPORT_FILEFORMAT_TTF"[0..24], "-DSUPPORT_FILEFORMAT_FNT"[0..24], "-DSUPPORT_TEXT_MANIPULATION"[0..27], "-DSUPPORT_FONT_ATLAS_WHITE_REC"[0..30], "-DSUPPORT_FILEFORMAT_OBJ"[0..24], "-DSUPPORT_FILEFORMAT_MTL"[0..24], "-DSUPPORT_FILEFORMAT_IQM"[0..24], "-DSUPPORT_FILEFORMAT_GLTF"[0..25], "-DSUPPORT_FILEFORMAT_VOX"[0..24], "-DSUPPORT_FILEFORMAT_M3D"[0..24], "-DSUPPORT_MESH_GENERATION"[0..25], "-DSUPPORT_FILEFORMAT_WAV"[0..24], "-DSUPPORT_FILEFORMAT_OGG"[0..24], "-DSUPPORT_FILEFORMAT_MP3"[0..24], "-DSUPPORT_FILEFORMAT_QOA"[0..24], "-DSUPPORT_FILEFORMAT_XM"[0..23], "-DSUPPORT_FILEFORMAT_MOD"[0..24], "-DSUPPORT_STANDARD_FILEIO"[0..25], "-DSUPPORT_TRACELOG"[0..18] }[0..45])

sagehane avatar Oct 20 '24 03:10 sagehane

Wait, are you still experiencing breakage for your use-case?

When using @embedFile() instead of pathFromRoot(), yes. It still duplicates the submodule's file path:

/tmp/raylib-zig-dep-bug % zig build
install
└─ install raylib-zig-dep-bug
   └─ zig build-exe raylib-zig-dep-bug Debug native
      └─ WriteFile raylib.h failure
error: FileNotFound: /tmp/raylib-zig-dep-bug/raylib/raylib/src/raylib.h

But it's easily solvable by uncommenting the two pathFromRoot() lines, so I think it's perfectly fine. Not to mention, should src/build.zig ever be moved to the root directory in the future, that would also solve the problem.

Note, the "program" here is the build system and it will have no result in the final binary people should care about.

Oh, I thought it would affect the actual library binary that was outputted. That would be perfectly fine, then.

Don't know enough to comment on your other proposal regarding reading config.h, but if it makes the build script easier to debug, that sounds good to me.

Yerdun avatar Oct 20 '24 03:10 Yerdun

When using @embedFile() instead of pathFromRoot(), yes. It still duplicates the submodule's file path:

Sorry to hear that. I'll try your repo with this again. This is why merging src/build.zig to build.zig simplifies a lot of things and makes it easier to debug too.

File size differences with usage of @embedFile:

test1.zig:

const std = @import("std");

pub fn main() void {
    var x = @embedFile("config.h");
    _ = &x;
}

test2.zig:

const std = @import("std");

/// A list of all flags from `src/config.h` that one may override
const config_h_flags = outer: {
    // Set this value higher if compile errors happen as `src/config.h` gets larger
    @setEvalBranchQuota(1 << 20);

    const config_h = @embedFile("config.h");
    var flags: []const []const u8 = &.{};

    var lines = std.mem.tokenizeScalar(u8, config_h, '\n');
    while (lines.next()) |line| {
        if (!std.mem.containsAtLeast(u8, line, 1, "SUPPORT")) continue;
        if (std.mem.startsWith(u8, line, "//")) continue;
        if (std.mem.startsWith(u8, line, "#if")) continue;

        var flag = std.mem.trimLeft(u8, line, " \t"); // Trim whitespace
        flag = flag["#define ".len - 1 ..]; // Remove #define
        flag = std.mem.trimLeft(u8, flag, " \t"); // Trim whitespace
        flag = flag[0 .. std.mem.indexOf(u8, flag, " ") orelse continue]; // Flag is only one word, so capture till space
        flag = "-D" ++ flag; // Prepend with -D

        flags = @as([]const []const u8, flags ++ [1][]const u8{flag});
    }

    break :outer flags;
};

pub fn main() void {
    var x = config_h_flags;
    _ = &x;
}
$ zig build-exe test1.zig && zig build-exe test2.zig && du -b test1 test2
2454400	test1
2444984	test2

$ zig build-exe test1.zig -fstrip && zig build-exe test2.zig -fstrip && du -b test1 test2
201056	test1
186992	test2

$ strings test1 | rg RTEXT
#define SUPPORT_MODULE_RTEXTURES        1
#define SUPPORT_MODULE_RTEXT            1       // WARNING: It requires SUPPORT_MODULE_RTEXTURES to load sprite font textures

$ strings test2 | rg RTEXT
-DSUPPORT_MODULE_RTEXTURES
-DSUPPORT_MODULE_RTEXT

Okay, the difference is negligible here. But I still think the latter is easier to debug so I would prefer it that way.

sagehane avatar Oct 20 '24 04:10 sagehane

Sorry to hear that. I'll try your repo with this again.

Oh, really, don't worry about it. Your code already has an opt-in fix: like I mentioned earlier, it works perfectly fine when uncommenting the pathFromRoot() lines.

This is why merging src/build.zig to build.zig simplifies a lot of things and makes it easier to debug too.

Yup. So I wouldn't worry about introducing temporary fixes to the dependency problem right now, since just moving src/build.zig to root is a far more graceful way of handling it.

Yerdun avatar Oct 20 '24 04:10 Yerdun

Weird, when trying to use your repo on Zig 0.12.1 and 0.13.0, I get the following:

$ zig build
install
└─ install raylib
   └─ zig build-lib raylib Debug native
      └─ run wayland-scanner (xdg-shell-client-protocol-code.h) failure
error: FileNotFound: /tmp/raylib-zig-dep-bug/raylib/raylib/src/external/glfw/deps/wayland/xdg-shell.xml
<omitted>

I didn't notice this until now as I'm used to using the master branch of Zig, where everything compiles without any problems (including the @embedFile portion). And it's a different and more serious compile error that should be affecting more users than those passing -Dconfig... I wonder why you aren't encountering this... ugh, I really want to merge src/build.zig to build.zig. I'm getting headaches.

sagehane avatar Oct 20 '24 04:10 sagehane

Okay, fine, I'll just give up and call it a day and be satisfied that this repo successfully builds from the root and src dir for all versions of Zig from 0.12.0 and onwards. I also added the commit on parsing src/config.h at comptime.

I plan to use #4393 to actually bring the logic over from src/build.zig to build.zig as I'm not sure how to get this working for all intended use-cases otherwise.

sagehane avatar Oct 20 '24 05:10 sagehane

I wonder why you aren't encountering this...

Oh, I am actually. That's the same problem I mentioned earlier, where using pathFromRoot instead of @embedFile (or moving src/build.zig to build.zig`) makes it work.

I'm used to using the master branch of Zig, where everything compiles without any problems

Huh, didn't know that. I probably should have mentioned I was using 0.12, my bad.

And it's a different and more serious compile error that should be affecting more users than those passing -Dconfig ugh, I really want to merge src/build.zig to build.zig. I'm getting headaches.

Oh, I was thinking to just not worry about it until we try proposing the move to a single build.zig. As I was writing this, I just saw that you made a new commit. Sorry about that, didn't mean to make you work harder.

I'll go and test it with my repo in a bit.

Yerdun avatar Oct 20 '24 05:10 Yerdun

As I was writing this, I just saw that you made a new commit. Sorry about that, didn't mean to make you work harder.

Oh, that commit is totally unrelated so it won't fix your immediate problems.

Anyhow, @raysan5, I think this PR is ready to be merged and I'll fix the other existing problems with #4393.

sagehane avatar Oct 20 '24 05:10 sagehane

Oh, that commit is totally unrelated so it won't fix your immediate problems.

Ah, gotcha. No worries, you've accomplished your goals for this commit already. And the fix on my end is very easy, anyway.

Thanks for everything you've done.

Yerdun avatar Oct 20 '24 05:10 Yerdun

@sagehane Thanks for the review! Merged!

raysan5 avatar Oct 20 '24 22:10 raysan5