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

use a source build of libpcre #20

Merged
merged 8 commits into from
Jul 1, 2024
Merged

Conversation

lun-4
Copy link
Contributor

@lun-4 lun-4 commented Jun 30, 2024

removes the need to link against system libpcre

exposed pcre-8.45 as zigpkg which was used for the zigmod version of this library: nektro/pcre-8.45#2 unknown if it'll be merged so libpcre.zig would depend on my fork of it

drawback is that macos won't compile with source build, it'll still use system libpcre by default such that nothing breaks fixed

(this update should be carried out to koino post-merge such that koino doesn't depend on my fork of libpcre.zig, so I didn't open a PR for that yet)

@kivikakk
Copy link
Owner

kivikakk commented Jul 1, 2024

I like this very much! Can you clarify the bit about the macOS build? I tried it locally (with -Dsystem_library=false) and it tested fine, and it looks OK in CI too. Similarly, using that commit with koino seems fine on my system.

@lun-4
Copy link
Contributor Author

lun-4 commented Jul 1, 2024

had a temporary hack to make pcre work on my system and that broke macos (as well as ubuntu, it seems), but now it works out without issues so I'll leave it as false by default

looks like windows cross-compiles as well: tested with zig build -Dtarget=x86_64-windows-gnu -Doptimize=ReleaseSafe -Dcpu=x86_64_v2 -Dsystem_library=false, I wonder if that would close #17 by accident 🤔

@kivikakk
Copy link
Owner

kivikakk commented Jul 1, 2024

but now it works out without issues so I'll leave it as false by default

SGTM! I pushed a commit with a typo fix so you might need to pull.

I wonder if that would close #17 by accident 🤔

!!! Heyooo. I don't have a Windows dev machine right now but I'll try it in CI

@lun-4
Copy link
Contributor Author

lun-4 commented Jul 1, 2024

I think this would also kind of remove the need to vendor the library at this rate. if someone wants to use linkPcre they would be able to add .{ .system_library = "true" } to b.dependency()'s args. if you agree with this I can update the readme accordingly (and signal that windows is not supported in that mode)

@kivikakk
Copy link
Owner

kivikakk commented Jul 1, 2024

if you agree with this I can update the readme accordingly (and signal that windows is not supported in that mode)

Yes, absolutely! I'm really pleased with this, thanks so much for pushing all your work up!

@kivikakk
Copy link
Owner

kivikakk commented Jul 1, 2024

Windows success! :)

@kivikakk kivikakk linked an issue Jul 1, 2024 that may be closed by this pull request
@lun-4
Copy link
Contributor Author

lun-4 commented Jul 1, 2024

ready to merge as soon as CI passes again!

@kivikakk
Copy link
Owner

kivikakk commented Jul 1, 2024

Tysm! <3

@kivikakk kivikakk merged commit 0f3e120 into kivikakk:main Jul 1, 2024
4 checks passed
@kivikakk
Copy link
Owner

kivikakk commented Jul 1, 2024

I'll make the koino update now and save you the extra work!

@lun-4 lun-4 deleted the static-linking branch July 1, 2024 19:49
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.

Fix Windows CI
2 participants