Support for direct installation via zig fetch#907
Support for direct installation via zig fetch#907mrdimidium wants to merge 5 commits intoPCRE2Project:mainfrom
zig fetch#907Conversation
NWilson
left a comment
NWilson
left a comment
There was a problem hiding this comment.
This looks fine, thank you. All the zig support is simply as submitted by other zig users, so if this is useful, then I'm happy to host it.
I will test it works on my machine and in the co jobs before merging. I like to be able to build the different platforms myself.
| "src/", "doc/", | ||
| "HACKING", "INSTALL", | ||
| "README.md", "LICENCE.md", | ||
| "AUTHORS.md", "SECURITY.md", |
There was a problem hiding this comment.
I think you want to include some additional directories here:
Possibly testdata and RunTest(.bat)
Probably deps, which includes source code for the JIT compiler, which is a supported option in the Zig build.
There was a problem hiding this comment.
It looks like zig fetch supports Git submodules, but I don't know how exactly the hash would be calculated based on the hashed contents here.
If you included the deps/ directory (ie the Git submodules) in the package hash, then the package importing pcre2 would calculate a different hash depending on whether or not the submodules were specified to be fetched or not.
There was a problem hiding this comment.
Interesting. I didn't include deps, expecting that if the user needed jit, they would install it separately and link it into Zig, but now I see that deps is included as files in the release archives.
I expect zig to calculate the hash based on the contents, but this will still lead to different results when installing from a release and from a repository.
There was a problem hiding this comment.
The most idiomatic way to specify juajit is through build.zig.zon . I'll try to suggest build.zig to the upstream author if they're willing to accept it.
There was a problem hiding this comment.
@NWilson, Please take a look at this commit. I explicitly installed the dependency from the same commit so it would work with git, HTTP, and any commit archive.
The file copying trick is necessary because the dependency is installed somewhere in zig-pkg, and we copy the file so it finds ../deps/.... This could be done more carefully by passing the path to the preprocessor, but I was hesitant to change the code for the sake of a zig build.
The downside is that you need to update the commit in the submodule and in build.zig.zon simultaneously.
There was a problem hiding this comment.
I would suggest that perhaps you don't want to use sljit as a Zig dependency, since it's not a Zig library. It really is part of PCRE2 (effectively), and it's just stored as a Git submodule for convenience.
If you want to use tarballs, then use a release tarball from GitHub Releases. If you want to use Git to checkout a tag, that's also fine. But I would dissuade you from using GitHub's per-commit tarballs, because they are not signed or reproducible (unlike our signed release tarballs and signed Git release tags).
There was a problem hiding this comment.
That makes sense. I replaced the dependency with an explicit inclusion of deps. I'm not sure the library user needs the test data and should depend on it. Am I wrong?
There was a problem hiding this comment.
Personally, I would always recommend running a library's test suite after building it from source. Maybe I'm paranoid! The pcre2test binary is built by Zig and can be used to verify the library functions, so I'd suggest you include the tests.
But I don't mind either way. I don't think the tests are hooked up to run automatically after the Zig build, so a Zig user could delete the testdata and not notice anything.
There was a problem hiding this comment.
Frankly, I'd be surprised by the tests running during builds, especially with cross-compilation.
I'd be happy to offer more build options in CI (at least cross-compilation and multiple compiler versions) if you're open to the extra workload.
There was a problem hiding this comment.
I don't need it - if you're happy for the tests not to be run by Zig, that's fine by me.
Don't do anything extra for me.
2 participants
Modern versions of zig (at least 0.15 and above) support a small package manager. This PR adds a manifest to the root directory and updates the build so that the manifest is included in the release archives.
I also took the liberty of updating the zig installation in CI to a reusable action from one of the zig core team members. This will prevent compiler version desynchronization and allow you to add builds to master versions if you wish. Let me know if this is unacceptable -- I'll return the snap or offer installation in ziglang.org with a small bash script (but with worse caching).
P.S. Thanks for supporting Zig here, it really made my job easier.