build.zig: Merge `src/build.zig` to `build.zig`

[sagehane]

I see no reason to have to keep multiple build.zig files in the same project unless the project is a monorepo with multiple independent components. Redundant with #4387.

• [x] Moved src/build.zig to build.zig
• [ ] Should work with https://github.com/rstefanic/raylib-zig-build-bug @rstefanic
• [ ] Should work with https://github.com/Yerdun/raylib-zig-dep-bug @Yerdurn
• [ ] Consider rethinking Raygui support? Could simplify lots of things
• [x] Consider if it really is worth applying the default values of src/config.h when -Dconfig is specified
  • Conclusion: Yes, it is worth applying the default values.

I think it should work. Would appreciate if you two could test if it works with the lines using Path(.{ .cwd_relative = commented out. I'm on NixOS and I could successfully compile without those lines except for when targeting wasm32-emscripten or wasm64-emscripten.

---

Breaking changes:

• RaylibOptions introduced in https://github.com/raysan5/raylib/pull/4375 will become Options
• No-longer tries to look into src/config.h to apply possible default configs when passing -Dconfig. This comes with the benefit of being able to supply more than one space-separated flags now

[Yerdun]

Hi, still have yet to test with your commits or read up on your changes, but I did want to address something you mentioned:

• Consider if it really is worth applying the default values of src/config.h when -Dconfig is specified

I think it should. To my understanding, this is done to maintain identical functionality with the other build systems: when provided with custom cflags, they keep all the default values, but overwrite them with any changes should the user supply any. That said, (a) I have not actually looked through the other build scripts and thus cannot confirm, and (b) I do not know if doing this in build.zig is especially cumbersome.

• No-longer tries to look into src/config.h to apply possible default configs when passing -Dconfig. This comes with the benefit of being able to supply more than one space-separated flags now

In 4a6a7511d1c2cfff19b506735d9f6a207ca9eea2, would replacing the original line 94 with new lines 71-74 work? I'm thinking your idea of adding each flag individually to raylib_flags_arr could integrate into the current way it's set up, thus fixing the problem while not completely rewriting its structure. Haven't tested it yet, but just an idea.

I tried adding a comment to your commit to hopefully demonstrate the idea more clearly.

[sagehane]

@Yerdun, care checking https://github.com/raysan5/raylib/pull/4393/commits/3e7b5a0bf40096acc12aca794a576e7af5e60dde?

I "fixed" the situation by making -Dconfig take a []const []const u8 instead of a []const u8. One of the motivations being that it's super annoying to have one long string that doesn't even accept newlines as a parameter.

Here's roughly how I'm testing stuff:

    const raylib = b.dependency("raylib", .{
        .target = target,
        .optimize = optimize,
        .config = @as([]const []const u8, &.{"-Wall, -Wextra"}),
    });

    const exe = <omitted>
    exe.linkLibrary(raylib.artifact("raylib"));

The code itself is quite ugly. I'm assuming there is no way to properly pass []const []const u8 with a length over 1 using std.Build.option, but I would more than love to be wrong.

---

I wish Zig's type-resolution was better such that @as isn't required to coerce a pointer into a const slice. Oh well.

[raysan5]

@sagehane In my experience, PRs with a big list of TODOs and changes do not end-up well... They usually generate more issues and more PRs and breaking things... If this PR is just trying to solve the two build.zig files, I'm ok with keeping both.

[sagehane]

Okay so, the main motivation for moving src/build.zig was to simplify the logic such that it would hopefully be easier to maintain the file. I am strongly in favour of doing this, but I'm not sure everyone agrees.

And, sort of as a result, I did find out that there's a bug related to -Dconfig and a way to sort of address it. I think this part is more important.

So should I:

1. Fix the -Dconfig issue first with a different PR and postpone the decision of whether or not src/build.zig should be merged to root?
2. Just merge this (after addressing conflicts) and get both?

Regarding the code comments with TODOs, they are (bar two locations where I questioned the necessity of existing code, mostly as a reminder for myself) about documenting unfortunate hacks that would probably be better without it. All functionality should work as intended for the end user and it should only be a consideration for future maintainers of the file.

[Yerdun]

@Yerdun, care checking 3e7b5a0?

Sorry to get back late, but can confirm my sample repo compiles just fine now. The raylib dependency takes custom cflags into account. I also tried the other repo configured for 0.13.0 and that one compiles too.

Also, for what it's worth, I briefly looked over your changes and I think your code makes sense. The only thing I'd do differently is use NOTEs instead of TODOs, since I think that tag more accurately describes your comments. Then again, I'm just an end user who's really bad at Zig, not a maintainer.

Thanks a lot for your hard work!

I did find out that there's a bug related to -Dconfig and a way to sort of address it. I think this part is more important.

So should I:

1. Fix the `-Dconfig` issue first with a different PR and postpone the decision of whether or not `src/build.zig` should be merged to root?

2. Just merge this (after addressing conflicts) and get both?

May I ask what the problem with -Dconfig is? Also, I would personally be in favor of the first option, as I prefer to split steps whenever possible. But again, I'm not a maintainer.

[sagehane]

The only thing I'd do differently is use NOTEs instead of TODOs, since I think that tag more accurately describes your comments.

Sure, my coding conventions might look weird to some people and I'm perfectly willing to conform if it's an issue.

May I ask what the problem with -Dconfig is?

1. Broken when defining the .config field of std.build.dependency

I introduced this. The biggest issue.

Before: https://github.com/raysan5/raylib/blob/c4be01329493d0e772a04de08639b4da32905d2a/src/build.zig#L67-L71 Now: https://github.com/raysan5/raylib/blob/dc5e6e0ad089f8eda932efb4646d6bbafe0713cc/src/build.zig#L88-L90

Currently, the wrong path is passed to readFileAlloc. The fix is probably to revert to the old code or use the std.build.pathFromRoot like in this PR.

2. The code for checking what flags are user-specified is faulty

Was like this when I got here.

https://github.com/raysan5/raylib/blob/dc5e6e0ad089f8eda932efb4646d6bbafe0713cc/src/build.zig#L99-L109

Say config.h defines options foo and foobar, and I pass in -Dfoobar. Then std.mem.containsAtLeast will return true for both foo and foobar, thus unsetting both.

A real example might be if I passed a value for -DSUPPORT_MODULE_RTEXTURES. As-is, the current code would also unset -DSUPPORT_MODULE_RTEXT as both share the exact same prefix:

std.mem.containsAtLeast(u8, "-DSUPPORT_MODULE_RTEXTURES", 1, "-DSUPPORT_MODULE_RTEXTURES") == true;
std.mem.containsAtLeast(u8, "-DSUPPORT_MODULE_RTEXTURES", 1, "-DSUPPORT_MODULE_RTEXT") == true;

This PR solves this by checking if the user-specified flag is equal to the flag or if it's immediately followed by an equal sign.

3. No way to pass more than one user-specified flag

Was like this when I got here. The solution isn't completely pretty but I'm convinced it's worthwhile.

https://github.com/raysan5/raylib/blob/dc5e6e0ad089f8eda932efb4646d6bbafe0713cc/src/build.zig#L112-L113

Say I pass "-Dfoo -Dbar" as a user-specified config. Then the build system literally tries to apply that instead of applying "-Dfoo" and "-Dbar" separately, causing the flag to be ineffective. One can easily test this by seeing if compiling the project with -Dconfig="-Werror -Wall" will successfully result in a build error, for example.

This PR solves this by splitting on the value when encountering a whitespace, and converting it to []const []const u8 (slices of strings). This is technically problematic too because now flags like -x c++ wouldn't be passed properly. But Zig currently doesn't support that flag and even if it did, -xc++ is also valid afaik. I don't know enough about C to know what other flags could be problematic.

4. It's tedious to pass multiple user-specified flags through std.build.dependency

Related to 3, not a bug but ergonomics issue. This has weird implications and it could be better.

This PR solves this by changing the type of Options.config from []const u8 (strings) to []const []const u8 (slices of strings).

From the dependency side of stuff, I can now do:

    const raylib = b.dependency("raylib", .{
        .target = target,
        .optimize = optimize,
        .config = @as([]const []const u8, &.{
            "-DSUPPORT_MODULE_RTEXTURES=1",
            "-DSUPPORT_MODULE_RTEXT=1",
        }),
    });

Instead of having to do:

    const raylib = b.dependency("raylib", .{
        .target = target,
        .optimize = optimize,
        .config = @as([]const u8, "-DSUPPORT_MODULE_RTEXTURES=1 -DSUPPORT_MODULE_RTEXT=1"),
    });

It looks much better as the number of flags get longer. But I could also just expose a function that converts a list of strings into one big space-separated string. Would also have the same -x c++-like issues. (Perhaps this is better, but would likely be more confusing to those unwilling to read the documentation of the build.zig... But that's already kind of the case because how else would people know to have to use @as([]const []const u8, ...) to satisfy the type of the .config field?)

The implication is that passing -Dconfig="-Dfoo -Dbar" through the command-line will now be treated as @as([]const []const u8, &.{ "-Dfoo -Dbar" }). I'm not aware of any way to pass it as a non-singleton value. I hope upstream addresses this; might ask them later. Anyhow, I have a logic that checks if the length of the slice is 1 and converts it into a singleton list of space-separated flags. I think this is quite hacky so now I'm really reconsidering the "expose a helper function" method.

[Yerdun]

Thanks for the detailed rundown. Speaking as a layman end-user, I stand behind your proposed solutions. Even though you consider some hacky, they do solve the immediate issues at hand, and I can't think of any additional problems they would inadvertently introduce.

So should I:

1. Fix the `-Dconfig` issue first with a different PR and postpone the decision of whether or not `src/build.zig` should be merged to root?

2. Just merge this (after addressing conflicts) and get both?

~~I'm also going to change my stance on this, I would go with #2. You've already gotten things working, and retrofitting your changes to the old setup would just be unnecessary labor. Your current setup also has the added benefit of fixing the build.zig.zon dependency problem.~~

Scratch that, literally copying your /build.zig to /src/build.zig, then checking out /build.zig from master works just fine. Since Option #1 is possible without any additional effort, it might be more ideal so as to introduce changes gradually.

That said, given my inexperience, others' input here would have more weight than mine. And ultimately, the final decision on how to handle this is up to Ray.

[ngynkvn]

+1, it'll be easier to follow the commit history if this was 2 PRs, one for the compile logic changes and one for relocating src/build.zig. Trying to see what actually changed inside the build script is not trivial when it's coupled with the commit moving it into the project root

[raysan5]

@sagehane Should this PR remain open conseidering #4398 has been merged?

[sagehane]

@raysan5, yes, please keep it open. I do plan to merge src/build.zig as some use-cases are still broken. Unless you prefer me making a new PR just for the sake of it.

[sagehane]

I think this is ready for review. I got all the bug-reproduction repos working with Zig versions 0.12.1, 0.13.0, and the master release of Zig.

---

In the meanwhile, I also noticed another strange thing... I am thinking of making a PR for https://github.com/sagehane/raylib/commit/f89c637b9e3d957c850746b9e2262985fe9bd51d and I would like testing from some testing on Linux users that aren't on NixOS.

The lines where I removed a bunch of linkSystemLibrary are insignificant as Zig's linker is apparently smart enough to unlink libraries if they're not being used. If those lines are needed for some people's setup, it can trivially be added back. I just wanted to see how much I can remove whilst getting everything to build.

The interesting parts are:

• Trying to add /usr/include and /usr/lib to system paths when Zig should be doing that by default
• Trying to link drm instead of libdrm and hacking through it by adding /usr/include/libdrm
  • This breaks builds with -Dplatform=drm for systems where the path doesn't exist (NixOS, Guix, people installing to /opt, etc)
• Not properly documenting the fact that pkg-config is a dependency for some of these libraries
  • Maybe that's just a packaging quirk on NixOS and it works fine elsewhere

Anyhow, I'll worry about that PR if/when this one gets merged.

---

And another TODO: Now that the logic for parsing src/config.h has become simplified, it'll be trivial to configure all definitions instead of just the ones that start with SUPPORT.

[Yerdun]

In the meanwhile, I also noticed another strange thing... I am thinking of making a PR for sagehane@f89c637 and I would like testing from some testing on Linux users that aren't on NixOS.

With this commit, I confirm I was able to successfully build raylib and the example projects on Zig 0.12, 0.13, and master. For the record, I used the "Both" and "X11" windowing backend options, as well as building on the drm platform.

I also briefly tested this commit on my bug reproduction repo, on 0.12. That worked fine, too.

I'm willing to do more extensive testing should you ever open a PR for it.

[raysan5]

@sagehane Thank you very much for all the work put into this improvement/redesign! Merging!