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.
| 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.
| 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 |
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.