-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Build the compiler with PCRE2 #13084
Build the compiler with PCRE2 #13084
Conversation
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.
The first generation cross-compilation command to Windows also needs an explicit -Duse_pcre2
Converted to draft. There are a couple of CI issues with PCRE2 that need to be resolved first. |
8be84d6
to
cdf9edc
Compare
f8ba190
to
c3a2949
Compare
CI is all green. This is ready for review now. |
.github/workflows/linux.yml
Outdated
@@ -15,13 +15,18 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
crystal_bootstrap_version: [1.2.2, 1.3.2, 1.4.1, 1.5.1, 1.6.2, 1.7.2] | |||
crystal_bootstrap_version: [1.2.2, 1.3.2, 1.4.1, 1.5.1, 1.6.2] | |||
env: ["USE_PCRE1=true"] |
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.
Hash notation seems more natural:
env: ["USE_PCRE1=true"] | |
env: | |
USE_PCRE1: true |
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 shit, I've noticed just now that's not jobs.<job_id>.env
but rather a matrix variable called env
. In this case using hash notation rather won't work, sorry for the mixup. btw, I'd suggest renaming this variable to something more descriptive, like flags
, so it gives better understanding what is it for and won't mixup with the env
key, which is a thing for GHA.
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.
@straight-shoota This most likely broke the CI :(
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 shit. Thanks for noticing.
What's really, really bad about this is that the GitHub Actions summary on the PR and commit doesn't mention this. The workflow file failed to parse but that's not reported as a failure? WTF.
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
The explicit selection of PCRE2 via
-D use_pcre2
is intentional. We want the compiler to built with PCRE2 and not silently fall back to PCRE.