Switch default regex engine to PCRE2 #12978
Conversation
straight-shoota
force-pushed
the
feature/pcre2-default
branch
from
d2a06b1 to
957bb12
Compare
|
So it looks like something with PCRE2 JIT support is broken on Windows and leads to invalid memory access (#13013). All other platforms seem to be working good, though. I have disabled JIT compilation for |
|
After #13056 is merged we can undo the last commits. |
.github/workflows/win.yml
Outdated
| @@ -15,7 +15,7 @@ jobs: | |||
| make deps | |||
| - name: Cross-compile Crystal | |||
| run: | | |||
| LLVM_TARGETS=X86 bin/crystal build --cross-compile --target x86_64-pc-windows-msvc src/compiler/crystal.cr -Dwithout_playground -Dwithout_iconv -Dwithout_interpreter | |||
| LLVM_TARGETS=X86 bin/crystal build --cross-compile --target x86_64-pc-windows-msvc src/compiler/crystal.cr -Dwithout_playground -Dwithout_iconv -Dwithout_interpreter -Duse_pcre2 | |||
There was a problem hiding this comment.
This first-generation compiler is still a compiler, so shouldn't it use PCRE1? Or are we intentionally detecting potential PCRE2 errors here? Same for the linker command below
There was a problem hiding this comment.
The reason to use PCRE2 directly also for the compiler is to drop PCRE completely from the Windows workflow. I guess we could also keep it there, at least for building the compiler. Wouldn't be too hard, but still extra effort.
My rationale is that Windows is still an unsupported preview platform and thus breaking PCRE compatibility isn't that much of a problem.
Of course it's not ideal to have different compiler behaviour on different platforms either, though 🤔
There was a problem hiding this comment.
what's the extra effort required?
There was a problem hiding this comment.
I guess it's just build both libraries, make sure to build the compiler with PCRE and distribute only PCRE2.
There was a problem hiding this comment.
Then wouldn't it break all users who try to build the compiler with make -fMakefile.win FLAGS=-Duse_pcre? The only compelling reason not to distribute PCRE1 right now is if the mirror dies, otherwise let's just try to keep things consistent between Windows and other platforms
There was a problem hiding this comment.
Yes, either that or manually install libpcre.
There was a problem hiding this comment.
That's rather poor UX. In that case please continue to omit -Duse_pcre in Makefile.win.
Back to this change, I don't think we need -Duse_pcre2 here as it is already the new default (and indeed the second-generation compiler uses PCRE2 because -Duse_pcre isn't present).
There was a problem hiding this comment.
Why? The normal packages as for using crystal to build your (or someone else's) software. Not for building the compiler. We're not shipping a copy of libllvm either - which you need for building the compiler even more than PCRE.
There was a problem hiding this comment.
The reason we don't ship LLVM is because the static libraries are too large and we haven't figured out DLL distribution, not that we shouldn't ship it.
There was a problem hiding this comment.
Windows packaging is another rabbit hole we shouldn't get too deep into with this.
So I was actually wrong, we're still building and shipping PCRE on windows.
I changed it back to build the compiler with PCRE as well.
| # and if none is given it tries to check for availability of `libpcre2` with `pkg-config`. | ||
| # If `pkg-config` is unavailable, the default is PCRE2. If `pkg-config` is available but | ||
| # has no information about a `libpcre2` package, it falls back to PCRE. | ||
| {% if flag?(:use_pcre2) || (!flag?(:use_pcre) && (flag?(:win32) || `hash pkg-config 2> /dev/null && (pkg-config --silence-errors --modversion libpcre2-8 || printf %s false) || true` != "false")) %} |
There was a problem hiding this comment.
If you run make crystal FLAGS=-Duse_pcre2 then both flags will be present and it is PCRE2 that will be chosen. It seems this is by design, contrary to my suggestion here. Documenting that behavior would be nice even if these flags are temporary
There was a problem hiding this comment.
The two steps to remedy this is to err if the two flags are present, and fix the makefile to not add the flag if there's one of these there, right?
There was a problem hiding this comment.
You'd have to test for both -Duse_pcre2 and -D use_pcre2 with the space inside the Makefile, which sounds rather complex for such a small task, and that's not counting CRYSTAL_OPTS, so perhaps this behavior is fine. It is not like those flags would exist for more than 3 minor releases or something
This is the next step for the migration towards PCRE2 (#12790).
With this change PCRE2 becomes the default regex engine.
There's still an automatic fallback when
pkg-configcan't findlibpcre2, so you should still be able to build Crystal programs iflibpcre2is missing on the build system. This will change in a future release, though.It's always possible to actively choose one or the other with
-Duse_pcreand-Duse_pcre2.The compiler continues to be built with PCRE for now to ensure consistency for regex literals in the language (see #12857).