- Notifications
You must be signed in to change notification settings - Fork239
update build.zig for release of 0.14.0 breaking language changes#724
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
NWilson left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
| constconfig_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There are a couple of regressions, here and below "pcre2_compile_cgroup.c".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
build.zig Outdated
| constpcre2test=b.addExecutable(.{ | ||
| .name="pcre2test", | ||
| .root_module=pcre2test_mod, | ||
| .linkage=linkage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What does "linkage" mean for an executable? Altering the linkage to choose betweenlibpcre2-8.{so,a} shouldn't force a property change on the pcre2test binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
another great question. This was likely added in an effort to fix some error messages when I hadn't yet removed the previouslinkLibrary() 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
qaptoR commentedMar 7, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
NWilson commentedMar 17, 2025
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. |
fa68936 intoPCRE2Project:masterUh oh!
There was an error while loading.Please reload this page.
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.