WIP: glfw: make the glfw package more friendly for projects outside of the mach project #23

2021-08-13T20:49:23Z

Avokadoen commented

2021-08-13 20:49:23 +00:00

(Migrated from github.com)

By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

This PR enables executables to link with GLFW and expose some previously missing functions

- [x] By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right. This PR enables executables to link with GLFW and expose some previously missing functions

emidoots (Migrated from github.com) reviewed

2021-08-14 19:30:32 +00:00

glfw/build.zig

					
				@ -39,1 +39,3 @@

				pub fn link(b: *Builder, step: *std.build.LibExeObjStep, options: Options) void {

				/// Extend a library by statically linking glfw with it

				pub fn linkStaticLibrary(b: *Builder, other_lib: *std.build.LibExeObjStep, options: Options) void {

				    const engine_lib = b.addStaticLibrary("engine", "src/main.zig");

Hi there @Avokadoen ! Really cool stuff here, thanks a ton for the PR - I'm quite excited to try out zig_vulkan now :)

I'd like to understand more what the changes to this build.zig do exactly so I can learn from you - my expectation was that anyone outside of the mach repo could already do this:

Import const glfw = @import("glfw/build.zig")
glfw.link(b, myLibOrExe, .{})

Based on these changes, though, it seems like either (1) that did not work for you or (2) there was an issue with "calling glfw.link(...) leads to the entire library rebuilding"? Is my guess for either of those right?

As an aside, I'd like to keep the external name just glfw.link instead of glfw.linkStaticLibrary for a few reasons:

I think static linking should be the default.
I think if dynamic linking is ever an option (I am not particularly interested in it), that it could be an option under the Options struct.

What do you think?

Hi there @Avokadoen ! Really cool stuff here, thanks a ton for the PR - I'm quite excited to try out zig_vulkan now :) I'd like to understand more what the changes to this build.zig do exactly so I can learn from you - my expectation was that anyone outside of the `mach` repo could already do this: 1. Import `const glfw = @import("glfw/build.zig")` 2. `glfw.link(b, myLibOrExe, .{})` Based on these changes, though, it seems like either (1) that did not work for you or (2) there was an issue with "calling `glfw.link(...)` leads to the entire library rebuilding"? Is my guess for either of those right? As an aside, I'd like to keep the external name just `glfw.link` instead of `glfw.linkStaticLibrary` for a few reasons: 1. I think static linking should be the default. 2. I think if dynamic linking is ever an option (I am not particularly interested in it), that it could be an option under the `Options` struct. What do you think?

Avokadoen (Migrated from github.com) reviewed

2021-08-14 20:22:14 +00:00

glfw/build.zig

					
				@ -39,1 +39,3 @@

				pub fn link(b: *Builder, step: *std.build.LibExeObjStep, options: Options) void {

				/// Extend a library by statically linking glfw with it

				pub fn linkStaticLibrary(b: *Builder, other_lib: *std.build.LibExeObjStep, options: Options) void {

				    const engine_lib = b.addStaticLibrary("engine", "src/main.zig");

Hi!
Let me just start by thanking you for the amazing work on the glfw building script and wrapper API.
It seems like calling step.linkLibrary(lib) when lib has been initialized through var lib = b.addStaticLibrary("foo", "baz") makes the final build command do zig build-lib even when the desired command is zig build-exe. This means that anything that is not a library step can not utilize the link function defined in the glfw build script in master.

The naming of the functions could be improved as the linkStaticLibrary function is referencing linking the glfw step with a another static library step i.e mach engine (linking with mach is done statically if i'm note mistaken?).

I agree dynamic linking is not very useful in the context of this project!

So to summarize, this PR aims to enable third party project to simply link using the glfw.linkStep(b, third_party_step, .{}), and then a third_party_step.addPackagePath("glfw", "deps/mach/glfw/src/main.zig").

PS: vulkan_zig is not very interesting at the moment from any perspective, but I working on changing that!

Hi! Let me just start by thanking you for the amazing work on the glfw building script and wrapper API. It seems like calling ``step.linkLibrary(lib)`` when ``lib`` has been initialized through ``var lib = b.addStaticLibrary("foo", "baz")`` makes the final build command do ``zig build-lib`` even when the desired command is ``zig build-exe``. This means that anything that is not a library step can not utilize the link function defined in the glfw build script in master. The naming of the functions could be improved as the linkStaticLibrary function is referencing linking the glfw step with a another static library step i.e mach engine (linking with mach is done statically if i'm note mistaken?). I agree dynamic linking is not very useful in the context of this project! So to summarize, this PR aims to enable third party project to simply link using the ``glfw.linkStep(b, third_party_step, .{})``, and then a ``third_party_step.addPackagePath("glfw", "deps/mach/glfw/src/main.zig")``. PS: vulkan_zig is not very interesting at the moment from any perspective, but I working on changing that!

Avokadoen (Migrated from github.com) reviewed

2021-08-14 23:39:49 +00:00

glfw/build.zig

					
				@ -39,1 +39,3 @@

				pub fn link(b: *Builder, step: *std.build.LibExeObjStep, options: Options) void {

				/// Extend a library by statically linking glfw with it

				pub fn linkStaticLibrary(b: *Builder, other_lib: *std.build.LibExeObjStep, options: Options) void {

				    const engine_lib = b.addStaticLibrary("engine", "src/main.zig");

After trying to create a minimum requirement to reproduce the error i discovered that linking seems to work with a simple hello world linked using the existing link function. I'm converting the PR to a draft until I can submit an issue explaining things. I'll look into it tomorrow :^)

Avokadoen (Migrated from github.com) reviewed

2021-08-15 10:51:04 +00:00

glfw/build.zig

					
				@ -39,1 +39,3 @@

				pub fn link(b: *Builder, step: *std.build.LibExeObjStep, options: Options) void {

				/// Extend a library by statically linking glfw with it

				pub fn linkStaticLibrary(b: *Builder, other_lib: *std.build.LibExeObjStep, options: Options) void {

				    const engine_lib = b.addStaticLibrary("engine", "src/main.zig");

This is more likely an issue debate rather than a PR debate at this point. It seems like a fresh zig project created with init-exedoes not suffer from the side effect of being ran with build-lib like the zig_vulkan project. Currently unsure if this is a mach, zig_vulkan, or a zig compiler bug and so I'm hesitant to submit a issue on this project at this point. I will not prioritize this issue as my fork works for now. I'll submit an issue at a later point when I eventually get back to this.

This is more likely an issue debate rather than a PR debate at this point. It seems like a fresh zig project created with ``init-exe``does not suffer from the side effect of being ran with ``build-lib`` like the zig_vulkan project. Currently unsure if this is a mach, zig_vulkan, or a zig compiler bug and so I'm hesitant to submit a issue on this project at this point. I will not prioritize this issue as my fork works for now. I'll submit an issue at a later point when I eventually get back to this.