The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
Greetings,
I took a look at the v2 patch for the PGXS update-po bug, and it looks solid.
I built PostgreSQL from source with --enable-nls and set up a test installation. To actually see the bug in action and
verifythe fix, I put together a simple external extension with NLS support - just a basic C file with some
errmsg/errhintcalls, a PGXS Makefile with the usual CATALOG_NAME and AVAIL_LANGUAGES settings, and a po/ directory with
French,German, and Japanese translations. I also added an nls.mk file and a po/LINGUAS file, since they are required
forthe NLS machinery to work.
Before the patch, running make update-po in my extension directory did absolutely nothing—no .po.new files were
created.The problem is pretty straightforward once you look at it: nls-global.mk searches $(top_srcdir) for .po files,
butin PGXS mode, $ (top_srcdir) points to the PostgreSQL install directory, not the extension's source tree, where the
actual.po files live.
I applied the patch and everything works as expected. The patch adds the PGXS conditional to nls-global.mk so it
searchesthe current directory instead, and make update-po successfully created all three .po.new files (fr, de, ja)
usingmsgmerge. The generated files look good—proper msgid/msgstr entries and valid translation content. The extension
buildsfine afterward as well.
I also double-checked that regular in-tree builds still work correctly with contrib modules, and they do.
The fix is straightforward and does exactly what it needs to—using ‘find .’ for PGXS mode instead of ‘find
$(top_srcdir)’for regular builds. Both ALL_LANGUAGES and all_compendia get the same treatment, and the ifdef nesting
looksright to me.
The patch is clean, and I didn't encounter any issues during testing. There is no documentation, but for this BUG FIX,
Idid not expect any.
Looks good to me— I think it is ready for commit.
Bryan Green
