Add build option to provide an additional header file to the Lua compilation#159
Add build option to provide an additional header file to the Lua compilation#159robbielyman merged 2 commits intonatecraddock:mainfrom
Conversation
robbielyman
left a comment
robbielyman
left a comment
There was a problem hiding this comment.
i'm a bit concerned about the user_h.getPath3 call—could you make sure that this is an acceptable method for consumers of the Zig build system to be using? in particular, i'm concerned about whether this build script works correctly in the case that the lua_user_h file is generated at build time.
also the orelse null should be deleted, since it does nothing.
build.zig
Outdated
| const lang = b.option(Language, "lang", "Lua language version to build") orelse .lua54; | ||
| const shared = b.option(bool, "shared", "Build shared library instead of static") orelse false; | ||
| const luau_use_4_vector = b.option(bool, "luau_use_4_vector", "Build Luau to use 4-vectors instead of the default 3-vector.") orelse false; | ||
| var lua_user_h = b.option(Build.LazyPath, "lua_user_h", "Lazy path to user supplied c header file") orelse null; |
There was a problem hiding this comment.
both var and orelse null are code smells.
build/lua.zig
Outdated
| // Build as DLL for windows if shared | ||
| if (target.result.os.tag == .windows and shared) "-DLUA_BUILD_AS_DLL" else "", | ||
|
|
||
| if (lua_user_h) |user_h| b.fmt("-DLUA_USER_H=\"{}\"", .{user_h.getPath3(b, null)}) else "", |
There was a problem hiding this comment.
i'm not sure, but are authors of build scripts supposed to use getPath at all?
There was a problem hiding this comment.
Probably not, that was rather sloppy. Actually using the provided addIncludePath and installHeader seems to be the way to do it properly. This also solves the var code smell from above.
This allows overriding Lua specific funtions like lua_[un]lock.
|
At this point, maybe an options struct being passed to the configure calls could help with extensibility. |
robbielyman
left a comment
robbielyman
left a comment
There was a problem hiding this comment.
looks great! thanks for cleaning that up.
2 participants
resolves #143
Supplying the option as a
std.Builld.LazyPathand expanding relative paths might not be the most elegant option, but it works for both the examples and when used as a module.Alternatively to this solution, ziglua could provide its own header file and expose some functions a user would have to implement (like lua_zlock). But this would require additional flags or some structure containing all the options. Delegating the header to the user seems simpler and more flexible to me. (I am not sure how much this feature is going to be used anyways :D)