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] Remove addRaylib and fix raygui builds when using raylib as dep #4475

Merged
merged 1 commit into from
Nov 10, 2024

Conversation

deathbeam
Copy link
Contributor

@deathbeam deathbeam commented Nov 10, 2024

  • addRaylib just duplicates what adding raylib as dependency already does
    so it do not needs to exist
  • move raygui build to standard build process when flag is enabled. this
    works correctly when using raylib as dependency and having raygui as
    dependency as well. problem with previous approach was that raygui was in
    options but it was doing nothing because you had to also use addRaygui for
    it to be effective

For how the dependency should be included normally in other projects using zig builds, its as simple as this (on top of including raylib in build.zig.zon)

const target = b.standardTargetOptions(.{});
const optimize = b.standardOptimizeOption(.{});

// This is the whole thing
const raylib_dep = b.dependency("raylib", .{ .target = target, .optimize = optimize, .shared = true });
const raylib = raylib_dep.artifact("raylib");
b.installArtifact(raylib)

…s dep

- addRaylib just duplicates what adding raylib as dependency already does
  so it do not needs to exist
- move raygui build to standard build process when flag is enabled. this
  works correctly when using raylib as dependency and having raygui as
  dependency as well. problem with previous approach was that raygui was in
  options but it was doing nothing because you had to also use addRaygui for
  it to be effective

Signed-off-by: Tomas Slusny <[email protected]>
@raysan5
Copy link
Owner

raysan5 commented Nov 10, 2024

@deathbeam thanks for the review! I like PRs that remove code lines!

@sagehane maybe you can also review those changes?

@raysan5 raysan5 changed the title build.zig: Remove addRaylib and fix raygui builds when using raylib as dep [build][Zig] Remove addRaylib and fix raygui builds when using raylib as dep Nov 10, 2024
@sagehane
Copy link
Contributor

I never used addRaylib because it's not idiomatic as far as the Zig package manager is supposed to be used, so I am in favour of removing it. I've not used raygui so I really can't comment on it, and the only other thing to note is the removal of the global flags array and moving around a bit of code. It looks good to me.

I will note that it is a breaking change in that addRaylib will no longer work, but the functionality exposed is the same and people will just be encouraged to do things the more idiomatic way.

@deathbeam
Copy link
Contributor Author

deathbeam commented Nov 10, 2024

Re: breaking change: this is list of projects that are using addRaylib in build.zig:

https://github.com/search?q=addRaylib+path%3Abuild.zig&type=code&p=1

A lot of it is coming from having raylib build.zig vendored so its not even being used. And outisde of <5 repos everything else is already not working on latest master (latest master addRaylib requires try before so everything that doesnt use it wont work, which is another side effect of having custom dependency function that only causes issues).

Also probably the biggest consumer of build.zig (i assume) is using the dependency properly (+ some boilerplate for raygui which should be unnecessary after this change too):

https://github.com/Not-Nik/raylib-zig/blob/devel/build.zig#L103

So I think impact of the change is minimal

@raysan5 raysan5 merged commit ce3b121 into raysan5:master Nov 10, 2024
2 checks passed
@raysan5
Copy link
Owner

raysan5 commented Nov 10, 2024

@sagehane @deathbeam thanks for the further review!

@deathbeam deathbeam deleted the remove-add-raylib branch November 10, 2024 12:53
Sign up for free to 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.

3 participants