-
Notifications
You must be signed in to change notification settings - Fork 202
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
Install and test zlib, bzip2 in CI builds #582
Conversation
7265b20
to
2d7a23f
Compare
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.
maybe use editline in the alpine job for variety?
not sure why this is needed though (maybe something to add to the commit message on the rationale), since the test should work regardless and it is mostly useful when the tool is used interactively
.github/workflows/build.yml
Outdated
@@ -16,7 +21,7 @@ jobs: | |||
run: ./autogen.sh | |||
|
|||
- name: Configure | |||
run: ./configure --enable-jit --enable-pcre2-16 --enable-pcre2-32 | |||
run: ./configure --enable-jit --enable-pcre2-16 --enable-pcre2-32 --enable-pcre2grep-libz --enable-pcre2grep-libbz2 --enable-pcre2test-libreadline |
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.
we also have libeditline
support (which is preferred due to license in the BSD)
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.
OK, I can use that.
I really wanted the CI to be testing the zlib/bzip2 support; and when I added that, I thought it would be reasonable to throw in a test for readline too (although we're only testing it builds & links, and not really exercising the readline support).
The readline support is not really important, since it's just test-only. It would be OK if it broke. But the zlib support can't break.
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.
truth is that pcre2grep
was always treated as a more complicated version of pcre2demo
, and that is the only tool that needs the dependency (the library does not)
agree though we are better off testing and treating it more like a product, as it seems people seem to think it is almost as useful as GNU grep
of ripgrep
(which funny enough also uses PCRE2)
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.
It would be nicer if other people could maintain the frontend (like even GNU grep has -P
now for PCRE!)
But I guess if people are using pcre2grep, then it needs to be tested.
I'm happy with this PR. We only need a minimal effort (smoke test) to exercise these external library integrations.
2d7a23f
to
a3b4474
Compare
a3b4474
to
b921a29
Compare
Also, ensure that the libreadline and libeditline builds pass (although we can't really test whether these work).
b921a29
to
4fc784d
Compare
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.
LGTM
Also, ensure that the libreadline and libeditline builds pass (although we can't really test whether these work).