Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zig build to CI #622

Merged
merged 6 commits into from
Dec 17, 2024
Merged

Add zig build to CI #622

merged 6 commits into from
Dec 17, 2024

Conversation

NWilson
Copy link
Member

@NWilson NWilson commented Dec 12, 2024

Since we ship a zip build file, we clearly should be testing it works.

And... it doesn't, really? Not very well, anyway. It's building with all config.h variables set to undefined, so it's pulling in stuff like the emulated memmove, and other stuff like that.

I've no idea how configure/feature-detection is supposed to work in a zig build file. But it's not being done at all currently. 😢

Anyway, I've made it build pcre2test so we can at least run the unit tests, and they thankfully pass. So something is working!

@NWilson
Copy link
Member Author

NWilson commented Dec 12, 2024

@player-two @RossComputerGuy @plajjan @star-tek-mb

You have all contributed to build.zig in the past.

Someone should check this PR. If there's anything wrong... I'd be grateful if one of you would fix it, please!

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files

📢 Thoughts on this report? Let us know!

@NWilson NWilson force-pushed the user/niwilson/zig-ci branch from f21e9fd to fc55df7 Compare December 12, 2024 14:33
Copy link
Contributor

@carenas carenas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most importantly, it doesn't seem to support the JIT.

but at least the previous syntax was backward compatible with old zig (not from the snap)

@NWilson
Copy link
Member Author

NWilson commented Dec 16, 2024

Thanks for looking at it @carenas

but at least the previous syntax was backward compatible with old zig (not from the snap)

I'm not sure if that's an advantage or not...? Using the latest documented syntax is probably an improvement, but I guess some users might want compatibility with older Zig releases. That seems less likely though - zig is a very new language, so users are likely to be updating frequently.

If none of the original Zig contributors reply, then I guess I should just merge this, so at least we have some CI testing.

@plajjan
Copy link
Contributor

plajjan commented Dec 16, 2024

While I might have contributed, we use our own version of pcre2 with build.zig because of issues in the past. I don't remember exactly but I suppose this file was outdated and perhaps generally not in working condition. So, I certainly welcome more testing of build.zig so we make sure it stays in good condition and I'd quickly ditch my own fork in case the upstream becomes usable for me. My view is that the whole zig community is pretty much on latest-and-greatest, so I'd say, target latest (v0.13 right now) and forget about older releases.

build.zig is more geared around inspecting the target CPU & platform (target triple) for feature detection. AFAIK there is no direct support for detecting things like HAVE_MEMMOVE, but if the ABI / libc is say musl libc, then you know what it is capable of or not and build logic accordingly.

There seems to be a cmake header file https://github.com/PCRE2Project/pcre2/blob/master/config-cmake.h.in that zig can use, see for example https://github.com/actonlang/libssh/blob/zig-build/build.zig#L18

In many cases the feature detection can be reduced, at least if we only aspire to support a few well known platforms, see for example https://github.com/actonlang/libxml2/blob/zig-build-v0.13/build.zig#L27

I support the PR as-is, it can be further improved in increments from here.

@NWilson
Copy link
Member Author

NWilson commented Dec 16, 2024

Thank you for your review! That link to usage of Zig addConfigHeader is useful. We should at least use that, even if the values of the detected features are hardcoded.

If you have an improved version of build.zig that you are using locally, please do contribute it upstream. There's no point having a file in our repo which we didn't write, don't understand, and don't use - if the users downstream aren't even making use of it!

Copy link
Contributor

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me.

@NWilson NWilson force-pushed the user/niwilson/zig-ci branch from 2cc91b0 to 7b643c8 Compare December 17, 2024 11:04
@NWilson
Copy link
Member Author

NWilson commented Dec 17, 2024

Thank you @RossComputerGuy and @plajjan.

I have added b.addConfigHeader() support, and I will merge this now.

@NWilson NWilson merged commit 413bd8a into master Dec 17, 2024
29 checks passed
@NWilson NWilson deleted the user/niwilson/zig-ci branch December 17, 2024 11:10
@carenas
Copy link
Contributor

carenas commented Dec 17, 2024

A little off-topic, but not everything we ship works and that is still ok.

AFAIK, you can't build PCRE2 in zOS using the release package, because it is missing at least a header and some of the code might had syntax errors, but still there is a zOS package for PCRE2 which is a derivative work of our, and might be now based on at least 10.33 (allthought I am not sure, as I don't have a zOS system to test, and the "porter" has gone silent for a while)

When 10.35 releases, it would be nice if it will support zOS better, but there is not much we can do ourselves about it.

@NWilson
Copy link
Member Author

NWilson commented Dec 17, 2024

Haha! z/OS is what I would call a seriously "exotic" system (same as VMS...).

I take no responsibility for: AIX (and POWER), Solaris on SPARC, HP-UX (and PA-RISC or Itanium), or VMS or z/OS... they're just out of scope, for me personally. I'll accept patches, but it's really not something the average open-source project can offer support for.

I think the list of platforms in our CI currently is pretty much what we want to claim we support in future:

  • GCC, Clang, MSVC. (Could also try Intel ICC, MinGW...) Support for Sun cc is a bonus. No testing with xlc
  • Windows / Mac / Linux / Solaris / FreeBSD. That's pretty broad already.
  • We optimistically expect (without testing) that it will work on Android, iOS, 32-bit i386, 32-bit ARM (... maybe we should test that actually)

@carenas
Copy link
Contributor

carenas commented Dec 18, 2024

they're just out of scope, for me personally

sure, but one of the most "valuable" things PCRE2 has done, is to make sure it is as portable as possible, because there are crazy people out there running it compiled into webassembly, or even running in CPUs you never heard about and that no longer have sljit support.

I am sure you have noticed that the library doesn't even use strtol, when that is part of ANSI C and POSIX.

Something I probably should had mentioned before in our meetting, is that to be completely fair Windows in ARM is definitely not supported, and the only one that has full support is 32-bit x86 ;)

@plajjan
Copy link
Contributor

plajjan commented Dec 18, 2024

Zig ships with libc for a number of different archs / ABIs which is really nice because we can do cross-compilation out-of-the-box without providing anything extra. This is possible because for example zig ships with macos headers or all versions of glibc headers (cleverly compacted). It is naturally possible to rely on the system libc as well, but it's much less convenient.

I think the zig CI testing should focus on the platforms that zig ships libc headers for, i.e. https://ziglang.org/download/0.11.0/release-notes.html#Support-Table , in order of prio (IMHO)

  • linux
  • macos
  • windows
  • wasi

We might not be able to run the compiled executables of all of these in CI, but at least we can compile and for the Boehm GC, I think a lot was learnt just by running such cross-compiles. Doubly so because zig enables a bunch of rather strict checks per default, so we get more errors, usually indicative of error prone code. Lots of things have improved in Boehm GC based on that.

I suppose x86_64 and aarch64 are the most relevant CPU architectures although we could imagine testing on more.

I think adding these cross-compilation tests to CI should be one of the next steps in order to improve things.

@PhilipHazel
Copy link
Collaborator

With regard to zOS, the "port" was done by Ze'ev Atlas (zatlas1@yahoo.com) and he has maintained it through a number of releases. The code needs some "massaging" for zOS so I think Carlo is right in that this is probably best described as a derivative rather than a port. The main thing for our consideration is to maintain the character conventions that enable PCRE2 to work in EBCDIC code.

@NWilson
Copy link
Member Author

NWilson commented Dec 18, 2024

because there are crazy people out there running it compiled into webassembly

That would be me 😄. WebAssembly is not "crazy" like z/OS! It's a modern target architecture, with an easy-to-obtain compiler (standard clang supports it fully). Disclaimer: I was one of the LLVM contributors who added C++ support to their/own WebAssembly backend.

Thankfully, I think we're all broadly in agreement about what we can and can't support as PCRE2 maintainers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants