update build.zig for release of 0.14.0 breaking language changes #724
Conversation
There was a problem hiding this comment.
You or I will also need to update the CI build to validate this against 0.14. +My mistake - the CI is already using Zig "latest" so it has started testing against 0.14 already.+
I don't know Zig at all, so I'll just leave this PR open for a few days (or a week maybe?) in case anyone else from the Zig community comes by to comment.
Then I'll merge it, if it all builds and the tests pass.
build.zig
Outdated
| const config_header = b.addConfigHeader( | ||
| .{ | ||
| .style = .{ .cmake = b.path("src/config-cmake.h.in") }, | ||
| .style = .{ .cmake = b.path("config-cmake.h.in") }, |
There was a problem hiding this comment.
There are a couple of regressions, here and below "pcre2_compile_cgroup.c".
There was a problem hiding this comment.
oh! i see what you mean now. took me a while to understand this (namely figuring out that I wasn't on master!)
There was a problem hiding this comment.
Note that we won't target PRs at old release branches like 10.45. Aiming for master is correct, since the next release will come from there.
build.zig
Outdated
| const pcre2test = b.addExecutable(.{ | ||
| .name = "pcre2test", | ||
| .root_module = pcre2test_mod, | ||
| .linkage = linkage, |
There was a problem hiding this comment.
What does "linkage" mean for an executable? Altering the linkage to choose between libpcre2-8.{so,a} shouldn't force a property change on the pcre2test binary.
There was a problem hiding this comment.
another great question. This was likely added in an effort to fix some error messages when I hadn't yet removed the previous linkLibrary() steps.
I inferred the necessity by the executeable simply having the field at all, and the fact that we were 'linking libraries'. But once I switched to Importing the libraries into the pcre2test module, that likely was no longer a meaningful inference. I'll also test this now.
There was a problem hiding this comment.
yes it does seem like this is a redundant line, likely because we are importing now, instead of linking. likely whatever error it initially suppressed was a red herring.
|
@NWilson Definitely pause on merging. I naively made this pull request assuming the build worked on master, but I forgot that the repo I was using to rewrite the build.zig file was a It looks like the build.zig is failing now on master because of a missing so. this PR should be for the 10.45 branch. or just be halted until I can figure out how to do without those files. EDIT: It IS building now on master, so I'll push those changes, as well as the removal of the |
qaptoR
force-pushed
the
update_zig_build_0.14
branch
from
be9f687 to
89b5f95
Compare
NWilson
changed the title
|
Thank you @qaptoR for these changes! I will merge them. Enough time has passed and no-one else has said anything. I can see a few small things that could be cleaned up, now that we only support Zig 0.14 (this PR drops support for Zig 0.13, which is probably OK). I'll do those in a follow-up PR. |
2 participants
fixes #722
notable changes are that
.root_modulefields needs dedicated*Modulestructs created (cannot be anonymous.additionally, those modules are now how we add the C macros to the files being compiled.
also, for the pcre2test executable that we build, instead of having
linkLibrarysteps, I've directly added thepcre2andposixmodules as imports into the .root_module of the executable.lastly, two new default values were necessary to give values:
PCRE2GREP_BUFSIZEand_MAX_BUFSIZEfor theconfig_header.None of this was explicitly outlined in the zig documentation. It was inferred from the std.Build, std.Build.Compile, and std.Build.Module listings in the zig std Library reference.