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

Switch default regex engine to PCRE2 #12978

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/regex-engine.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ jobs:
- name: Install PCRE2
run: apk add pcre2-dev
- name: Assert using PCRE2
run: bin/crystal eval -Duse_pcre2 'abort unless Regex::Engine == Regex::PCRE2'
run: bin/crystal eval 'abort unless Regex::Engine == Regex::PCRE2'
- name: Assert select PCRE
run: bin/crystal eval -Duse_pcre 'abort unless Regex::Engine == Regex::PCRE'
- name: Run Regex specs
run: bin/crystal spec -Duse_pcre2 --order=random spec/std/regex*
run: bin/crystal spec --order=random spec/std/regex*
2 changes: 1 addition & 1 deletion .github/workflows/win.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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_pcre

- name: Upload Crystal object file
uses: actions/upload-artifact@v3
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ $(O)/primitives_spec: $(O)/crystal $(DEPS) $(SOURCES) $(SPEC_SOURCES)
$(O)/crystal: $(DEPS) $(SOURCES)
$(call check_llvm_config)
@mkdir -p $(O)
$(EXPORTS) $(EXPORTS_BUILD) ./bin/crystal build $(FLAGS) -o $@ src/compiler/crystal.cr -D without_openssl -D without_zlib
$(EXPORTS) $(EXPORTS_BUILD) ./bin/crystal build $(FLAGS) -o $@ src/compiler/crystal.cr -D without_openssl -D without_zlib -D use_pcre

$(LLVM_EXT_OBJ): $(LLVM_EXT_DIR)/llvm_ext.cc
$(call check_llvm_config)
Expand Down
2 changes: 1 addition & 1 deletion Makefile.win
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ $(O)\crystal.exe: $(DEPS) $(SOURCES) $(O)\crystal.res
@$(call MKDIR,"$(O)")
$(call export_vars)
$(call export_build_vars)
.\bin\crystal build $(FLAGS) -o "$(O)\crystal-next.exe" src\compiler\crystal.cr -D without_openssl -D without_zlib -D without_playground --link-flags=/PDBALTPATH:crystal.pdb "--link-flags=$(realpath $(O)\crystal.res)"
.\bin\crystal build $(FLAGS) -o "$(O)\crystal-next.exe" src\compiler\crystal.cr -D without_openssl -D without_zlib -D without_playground -D use_pcre --link-flags=/PDBALTPATH:crystal.pdb "--link-flags=$(realpath $(O)\crystal.res)"
$(call MV,"$(O)\crystal-next.exe","$@")
$(call MV,"$(O)\crystal-next.pdb","$(O)\crystal.pdb")

Expand Down
6 changes: 5 additions & 1 deletion src/regex/engine.cr
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
{% if flag?(:use_pcre2) %}
# The following condition ensures that the engine selection respects `-Duse_pcre2`/`-Duse_pcre`,
# 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")) %}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

require "./pcre2"

# :nodoc:
Expand Down