| Message ID | 20211118084143.279174-2-someguy@effective-light.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] grep/pcre2: limit the instances in which UTF mode is enabled | expand |
On Thu, Nov 18 2021, Hamza Mahfooz wrote: > Since, git aspires to support many PCRE2 versions, it is only sensible to > test changes to git against versions of PCRE2 that are deemed to be > notable, to ensure those changes to git don't cause regressions when using > the aforementioned PCRE2 versions. This is underscored by the fact that, > commit ae39ba431a (grep/pcre2: fix an edge case concerning ascii patterns > and UTF-8 data, 2021-10-15) was able to make it's way to master while > causing an otherwise easy to catch regression when an older version of > PCRE2 is used. So, to address this issue, add a job for PCRE2 to build all > of the notable versions, compile all of them against git and only run the > tests that can possibly be impacted by PCRE2. > > Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Hamza Mahfooz <someguy@effective-light.com> > --- > .github/workflows/main.yml | 26 ++++++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml > index 6ed6a9e807..ae96fc0251 100644 > --- a/.github/workflows/main.yml > +++ b/.github/workflows/main.yml > @@ -319,3 +319,29 @@ jobs: > - uses: actions/checkout@v2 > - run: ci/install-dependencies.sh > - run: ci/test-documentation.sh > + pcre2: > + needs: ci-config > + if: needs.ci-config.outputs.enabled == 'yes' > + strategy: > + fail-fast: false > + matrix: > + jit: ['--enable-jit', '--disable-jit'] > + utf: ['--enable-utf', '--disable-utf'] > + version: [31, 34, 36, 39] > + runs-on: ubuntu-latest > + steps: > + - uses: actions/checkout@v2 > + - uses: actions/checkout@v2 > + with: > + repository: 'PhilipHazel/pcre2' > + path: 'compat/pcre2-repo' > + - run: ci/install-dependencies.sh > + - run: cd compat/pcre2-repo > + - run: git checkout pcre2-10.${{matrix.version}} > + - run: ./autogen.sh > + - run: ./configure ${{matrix.jit}} ${{matrix.utf}} --prefix="$PWD/inst" Thanks a lot for following-up on this. Do you have a link to a sample run of this to see how it looks? I think that the --enable-utf here is a bug, my fault, I suggested using that option. But on closer inspection I should have said --{enable,disable}-unicode. Eyeballing the configure.ac in pcre2.git now and checking if/how it passes our tests I think it might be a noop unless --enable-ebcdic is also in play, which we don't need to test. Any reason for picking those specific versions? I think we do need to test on older than 10.31 (released in early 2018). On the other hand PCREv2 wasn't released at all until 2015, and got up to 10.20 all in that one year, and I think git may have been one of the first popular packages to use it. I (optionally at first) us from PCRE v1 to v2, and IIRC something like only 1-2 obscure packages in Debian depended on it at the time, with hundreds depending on PCREv1. We should also add a "latest" in there, and then just map that ourselves to: git tag -l --sort=-version:refname | head -n 1 I.e. it's probably overkill to test the pcre2.git bleeding edge, and it might produce undue noise, but testing the latest release in addition to select older versions seems like a good thing to do. > + - run: make > + - run: sudo make install I don't think "sudo" here is needed (and I'm surprised it works in CI, presumably a noop or it's always root). Just a: make install Should do, it'll also take care of doing "make" > + - run: cd ../.. > + - run: make USE_LIBPCRE=Y CFLAGS=-O3 LIBPCREDIR="$PWD/compat/pcre2-repo/inst" I was going to say: "We should probably leave CFLAGS undefined and use whatever the default is, or is there a good reason for -O3?". But I see this is another thing copied from my throw-away one-liner in [1], sorry. We can just skip that, I just happened to be testing with CFLAGS=-O3 locally at the time & copied that into my E-Mail. > + - run: cd t && make *{grep,log,pickaxe}* 1. https://lore.kernel.org/git/211117.867dd67spq.gmgdl@evledraar.gmail.com/
On Thu, Nov 18 2021 at 11:32:50 AM +0100, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> > Thanks a lot for following-up on this. Do you have a link to a sample > run of this to see how it looks? https://github.com/effective-light/git/actions/runs/1492352516 (it looks like the disable unicode case isn't worth considering, since it never runs through the tests successfully). > But on closer inspection I should have said > --{enable,disable}-unicode. Eyeballing the configure.ac in pcre2.git > now > and checking if/how it passes our tests I think it might be a noop > unless --enable-ebcdic is also in play, which we don't need to test. Looks like ebcdic and unicode can't be enabled at the same time. > Any reason for picking those specific versions? I think we do need to > test on older than 10.31 (released in early 2018). I chose them primarily because they were brought up on the other thread.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 6ed6a9e807..ae96fc0251 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -319,3 +319,29 @@ jobs: - uses: actions/checkout@v2 - run: ci/install-dependencies.sh - run: ci/test-documentation.sh + pcre2: + needs: ci-config + if: needs.ci-config.outputs.enabled == 'yes' + strategy: + fail-fast: false + matrix: + jit: ['--enable-jit', '--disable-jit'] + utf: ['--enable-utf', '--disable-utf'] + version: [31, 34, 36, 39] + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions/checkout@v2 + with: + repository: 'PhilipHazel/pcre2' + path: 'compat/pcre2-repo' + - run: ci/install-dependencies.sh + - run: cd compat/pcre2-repo + - run: git checkout pcre2-10.${{matrix.version}} + - run: ./autogen.sh + - run: ./configure ${{matrix.jit}} ${{matrix.utf}} --prefix="$PWD/inst" + - run: make + - run: sudo make install + - run: cd ../.. + - run: make USE_LIBPCRE=Y CFLAGS=-O3 LIBPCREDIR="$PWD/compat/pcre2-repo/inst" + - run: cd t && make *{grep,log,pickaxe}*
Since, git aspires to support many PCRE2 versions, it is only sensible to test changes to git against versions of PCRE2 that are deemed to be notable, to ensure those changes to git don't cause regressions when using the aforementioned PCRE2 versions. This is underscored by the fact that, commit ae39ba431a (grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data, 2021-10-15) was able to make it's way to master while causing an otherwise easy to catch regression when an older version of PCRE2 is used. So, to address this issue, add a job for PCRE2 to build all of the notable versions, compile all of them against git and only run the tests that can possibly be impacted by PCRE2. Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Hamza Mahfooz <someguy@effective-light.com> --- .github/workflows/main.yml | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)