GNOME Bugzilla – Bug 760683
regex test: Check the expected PCRE exceptions at runtime
Last modified: 2016-01-18 18:44:05 UTC
We might be built against a different (newer) version that we're run against. We just hit this in Ubuntu's autopkgtest environment, which attempts to minimise the number of "new" packages that are installed. Also I found out while testing this that one other new case switched behvaiour at an earlier version than 8.38. It's somewhere between 8.31 (old) and 8.35 (new), but I didn't exhausively test all of the intermediate versions.
Created attachment 319136 [details] [review] regex test: Check the expected PCRE version at runtime We might be built against a newer version than we're run against.
Created attachment 319137 [details] [review] regex test: Assert /(?P<sub>foo)\\g<sub/ changed behaviour at 8.35, not 8.38
Review of attachment 319136 [details] [review]: Please don't assume that my views on coding style match those of core GLib people, but: ::: glib/Makefile.am @@ +345,3 @@ if USE_SYSTEM_PCRE +export pcre_lib = $(PCRE_LIBS) +export pcre_inc = $(PCRE_CFLAGS) Doing this export to pass the information into a sub-Make seems odd. I would personally do this by duplicating the "if USE_SYSTEM_PCRE" block in tests/. ::: glib/tests/Makefile.am @@ +1,3 @@ include $(top_srcdir)/glib-tap.mk +LDADD = $(top_builddir)/glib/libglib-2.0.la $(pcre_lib) -lm I think it would be better to override regex_LDADD individually: regex_LDADD = $(LDADD) $(pcre_lib) to avoid overlinking the rest. @@ +4,3 @@ AM_CPPFLAGS = -g $(glib_INCLUDES) $(GLIB_DEBUG_FLAGS) DEFS = -DG_LOG_DOMAIN=\"GLib\" -DEXEEXT=\"$(EXEEXT)\" +AM_CFLAGS = $(GLIB_WARN_CFLAGS) $(pcre_inc) This is fine though IMO. ::: glib/tests/regex.c @@ +2177,3 @@ + + pcre_major = g_ascii_strtoull (version, &ptr, 10); + /* ptr points to ".MINOR (release date)" */ Maybe assert that *ptr is '.'?
Review of attachment 319136 [details] [review]: ::: glib/tests/regex.c @@ +2279,3 @@ TEST_NEW_FAIL ("(?<=\\C)X", 0, G_REGEX_ERROR_SINGLE_BYTE_MATCH_IN_LOOKBEHIND); TEST_NEW_FAIL ("(?!\\w)(?R)", 0, G_REGEX_ERROR_INFINITE_LOOP); +if (pcre_ge (8, 38)) It's actually 8.37, as Chun-wei Fan pointed out on Bug #740573. Perhaps you could fix that at the same time?
Created attachment 319192 [details] [review] regex test: Check the expected PCRE version at runtime We might be built against a newer version than we're run against.
Created attachment 319193 [details] [review] regex test: Assert /(?P<sub>foo)\\g<sub/ changed behaviour at 8.35, not 8.38
Created attachment 319194 [details] [review] regex test: assert that /(?(?<ab))/ changed behaviour at 8.37, not 8.38
I now have no objection to these patches, and I would set accepted-commit-now if I thought I counted as a maintainer.
Review of attachment 319192 [details] [review]: ::: glib/tests/regex.c @@ +2290,3 @@ TEST_NEW_FAIL ("(?P<sub>foo)\\g<sub", 0, G_REGEX_ERROR_MISSING_BACK_REFERENCE); TEST_NEW_FAIL ("(?(?<ab))", 0, G_REGEX_ERROR_MISSING_SUBPATTERN_NAME_TERMINATOR); +} Now that this is no longer a preprocessor directive, but a runtime check, it should be indented like C code.
Review of attachment 319193 [details] [review]: ok
Review of attachment 319194 [details] [review]: please squash this into the first patch
Created attachment 319253 [details] [review] regex test: Check the expected PCRE version at runtime We might be built against a newer version than we're run against. (407a4e9e4e02c82a2e6371958487cd0a7ad704d3 changes the version to 8.37 already, so attachment 319194 [details] [review] is now obsolete)
Created attachment 319254 [details] [review] regex test: Assert /(?P<sub>foo)\\g<sub/ changed behaviour at 8.35, not 8.38
Review of attachment 319253 [details] [review]: looks good now
Review of attachment 319254 [details] [review]: ok
Committed. Thanks.
This breaks the Continuous build: http://build.gnome.org/continuous/buildmaster/builds/2016/01/18/51/build/log-glib.txt srcdir != builddir maybe?