Conversation
WalkthroughReplaced PCRE1 packages ( Changes
Sequence Diagram(s)(omitted — changes do not alter runtime control flow) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/Dockerfile-test (2)
17-17: You can drop the explicit libpcre2-posix3; libpcre2-dev already depends on it.Trimming the package list slightly reduces image size with no loss of functionality. On Debian/Ubuntu,
libpcre2-devpulls inlibpcre2-posix3.
Citations: Debian Bookwormlibpcre2-devdepends onlibpcre2-posix3. (packages.debian.org, linux-packages.com)Suggested change:
- libpcre2-posix3 libpcre2-dev \ + libpcre2-dev \
38-52: Add a build-time sanity check to guarantee we actually linked NGINX against PCRE2.Since NGINX uses PCRE2 by default if found starting with 1.21.5, add a quick assertion after building to fail fast if it ever falls back.
Citations: NGINX added PCRE2 and prefers it by default in 1.21.5. (mailman.nginx.org)Apply:
@@ && auto/configure \ --with-http_auth_request_module \ --with-compat \ --with-debug \ # enables grpc --with-http_v2_module \ --add-dynamic-module=/opentracing \ - && make && make install + && make && make install \ + && ldd /usr/local/nginx/sbin/nginx | grep -qi 'pcre2' \ + && echo "Verified: nginx is linked with PCRE2"Alternatively (more robust), assert the PCRE2 define produced by configure:
- && make && make install + && make && make install \ + && grep -q 'NGX_PCRE2' objs/ngx_auto_config.hbuild/Dockerfile (2)
16-16: Minor trim:libpcre2-posix3is redundant whenlibpcre2-devis installed.
libpcre2-devdepends onlibpcre2-posix3. You can omit the explicit runtime package to keep the image leaner.
Citations:libpcre2-devdependency list. (packages.debian.org)Suggested change:
- libpcre2-posix3 libpcre2-dev \ + libpcre2-dev \
58-61: Optionally assert PCRE2 detection at build time.Add a simple check after configure to ensure the build used PCRE2 (guards against accidental regressions if base images change).
RUN ./auto/configure \ --with-compat \ --add-dynamic-module=/opentracing \ - && make modules + && grep -q 'NGX_PCRE2' objs/ngx_auto_config.h \ + && make modulesCitations: NGINX defines PCRE2 support in configure since 1.21.5; PCRE2 is preferred when found. (mailman.nginx.org)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
build/Dockerfile(1 hunks)test/Dockerfile-test(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Build Binaries (1.27.2)
- GitHub Check: Build Binaries (1.27.1)
- GitHub Check: Build Binaries (1.27.5)
- GitHub Check: Build Binaries (1.16.1)
- GitHub Check: Build Binaries (1.27.3)
- GitHub Check: Build Binaries (1.27.0)
- GitHub Check: Build Binaries (1.25.5)
- GitHub Check: Build Binaries (1.26.3)
- GitHub Check: Build Binaries (1.19.10)
- GitHub Check: Build Binaries (1.24.0)
- GitHub Check: Build Binaries (1.26.0)
- GitHub Check: Build Binaries (1.23.4)
- GitHub Check: Build Binaries (1.21.6)
- GitHub Check: Build Binaries (1.26.1)
- GitHub Check: System Testing
- GitHub Check: Build Docker Image
- GitHub Check: Build Docker Image (debian)
- GitHub Check: Analyze (go)
- GitHub Check: Build Docker Image (alpine)
🔇 Additional comments (4)
test/Dockerfile-test (2)
17-17: PCRE2 packages swap looks right for Ubuntu 24.04.
libpcre2-devandlibpcre2-posix3exist on Ubuntu 24.04, and installing them is appropriate for moving off PCRE (libpcre3).
Citations: Ubuntu package info for libpcre2-posix3 confirms availability. (linux-packages.com)
39-45: Ensure NGINX_VERSION >= 1.21.5 in CI.Older NGINX won’t understand PCRE2; 1.21.5+ is required. If this ARG is supplied by CI, validate it to avoid surprise build breaks.
Quick guard you can embed before configure:
@@ RUN cd / \ + && bash -euo pipefail -c 'v_req=1.21.5; v_cur="${NGINX_VERSION}"; \ + [ "$(printf "%s\n" "$v_req" "$v_cur" | sort -V | head -n1)" = "$v_req" ] || \ + { echo "NGINX_VERSION ($v_cur) must be >= $v_req for PCRE2"; exit 1; }' \ && wget -O nginx-release-${NGINX_VERSION}.tar.gz https://github.com/nginx/nginx/archive/release-${NGINX_VERSION}.tar.gz \Citations: PCRE2 supported and preferred since NGINX 1.21.5. (mailman.nginx.org)
build/Dockerfile (2)
16-16: Migrating to PCRE2 packages in the base stage looks good for Debian 12.
libpcre2-dev(andlibpcre2-posix3) are correct in Debian 12 and align with the project’s PCRE2 move.
Citations: Debian Bookworm pages forlibpcre2-devandlibpcre2-posix3. (packages.debian.org)
42-48: Confirm the NGINX version used in this stage is >= 1.21.5.Same rationale as the test image: older NGINX won’t select PCRE2. If
NGINX_VERSIONis injected by CI, validate before building.Example guard (just before
./auto/configure):@@ WORKDIR /nginx-release-${NGINX_VERSION} +RUN bash -euo pipefail -c 'v_req=1.21.5; v_cur="${NGINX_VERSION}"; \ + [ "$(printf "%s\n" "$v_req" "$v_cur" | sort -V | head -n1)" = "$v_req" ] || \ + { echo "NGINX_VERSION ($v_cur) must be >= $v_req for PCRE2"; exit 1; }'Citations: PCRE2 supported by NGINX starting in 1.21.5 and preferred when present. (mailman.nginx.org)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
build/Dockerfile (1)
3-17: Tiny image-size win (safe to skip for build stage).Since this is a build stage, it’s already low impact, but you can drop apt lists to keep layers lean.
RUN apt-get update && \ apt-get install --no-install-recommends --no-install-suggests -y \ build-essential \ cmake \ pkg-config \ git \ ca-certificates \ automake \ autogen \ autoconf \ libtool \ ssh \ wget \ libpcre2-dev \ - zlib1g-dev + zlib1g-dev && \ + rm -rf /var/lib/apt/lists/*
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.github/workflows/ci.yml(0 hunks)build/Dockerfile(1 hunks)test/Dockerfile-test(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/Dockerfile-test
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: Build Binaries (1.28.0)
- GitHub Check: Build Binaries (1.27.2)
- GitHub Check: Build Binaries (1.27.1)
- GitHub Check: Build Binaries (1.27.5)
- GitHub Check: Build Binaries (1.29.0)
- GitHub Check: Build Binaries (1.27.0)
- GitHub Check: Build Binaries (1.27.3)
- GitHub Check: Build Binaries (1.27.4)
- GitHub Check: Build Binaries (1.25.5)
- GitHub Check: Build Binaries (1.24.0)
- GitHub Check: Build Binaries (1.21.6)
- GitHub Check: Build Docker Image
- GitHub Check: System Testing
- GitHub Check: Build Docker Image (alpine)
- GitHub Check: Build Docker Image (debian)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
build/Dockerfile (2)
16-16: PR summary vs. implementation mismatch (libpcre2-posix3).The PR description mentions adding
libpcre2-posix3, but this Dockerfile only installslibpcre2-dev. Either addlibpcre2-posix3here (if required) or update the PR text to match reality.Optional diff if POSIX wrapper is actually needed:
- libpcre2-dev \ + libpcre2-dev \ + libpcre2-posix3 \
16-16: Enable PCRE2 explicitly in Nginx build and confirm matrix coverage
Add--with-pcre2to the./auto/configureinvocation inbuild/Dockerfileto force linking against PCRE2:RUN ./auto/configure \ --with-compat \ + --with-pcre2 \ --add-dynamic-module=/opentracing \ && make modulesCI matrix starts at Nginx 1.21.6 (PCRE2 support was introduced in 1.21.5)(lists.freebsd.org), so all tested versions already support PCRE2.
Likely an incorrect or invalid review comment.
1 participant
This pull request updates the Docker build and test environments to use the newer PCRE2 library instead of the legacy PCRE3 library. This change improves compatibility and security for regular expression handling in the system.
Dependency upgrades:
libpcre3andlibpcre3-devwithlibpcre2-posix3andlibpcre2-devinbuild/Dockerfileto use the latest PCRE2 libraries.test/Dockerfile-testfor consistency in the test environment.Summary by CodeRabbit
Chores
Tests