Buf fix: update-po for PGXS does not work

Started by Ryo Matsumura (Fujitsu)about 2 years ago4 messages

matsumura.ryo@fujitsu.com

about 2 years ago

2 attachment(s)

Hi hackers,

I found that 'make update-po' for PGXS does not work.
Even if execute 'make update-po', but xx.po.new is not generated.
I don't test and check for meson build system, but I post it tentatively.

I attached patch and test set.
'update-po' tries to find *.po files $top_srcdir, but there is no po-file in PGXS system because $top_srcdir is install directory.

Please check it.

Thank you.

Best Regards
Ryo Matsumura

Attachments:

update-po-test.tar.gzapplication/x-gzip; name=update-po-test.tar.gzDownload

fix-update-pgxs-po.patchapplication/octet-stream; name=fix-update-pgxs-po.patchDownload

diff --git a/src/nls-global.mk b/src/nls-global.mk
index f21bd5e..5a2778d 100644
--- a/src/nls-global.mk
+++ b/src/nls-global.mk
@@ -141,6 +141,10 @@ init-po: po/$(CATALOG_NAME).pot
 
 # For performance reasons, only calculate these when the user actually
 # requested update-po or a specific file.
+ifdef PGXS
+ALL_LANGUAGES := $(shell find . -name '*.po' -print | sed 's,^.*/\([^/]*\).po$$,\1,' | LC_ALL=C sort -u)
+all_compendia := $(shell find . -name '*.po' -print | LC_ALL=C sort)
+else
 ifneq (,$(filter update-po %.po.new,$(MAKECMDGOALS)))
 ALL_LANGUAGES := $(shell find $(top_srcdir) -name '*.po' -print | sed 's,^.*/\([^/]*\).po$$,\1,' | LC_ALL=C sort -u)
 all_compendia := $(shell find $(top_srcdir) -name '*.po' -print | LC_ALL=C sort)
@@ -149,6 +153,7 @@ ALL_LANGUAGES = $(AVAIL_LANGUAGES)
 all_compendia = FORCE
 FORCE:
 endif
+endif
 
 ifdef WANTED_LANGUAGES
 ALL_LANGUAGES := $(filter $(WANTED_LANGUAGES), $(ALL_LANGUAGES))

Alvaro Herrera

alvherre@alvh.no-ip.org

over 1 year ago

In reply to: Ryo Matsumura (Fujitsu) (#1)

1 attachment(s)

Re: Buf fix: update-po for PGXS does not work

On 2023-Oct-27, Ryo Matsumura (Fujitsu) wrote:

Hi hackers,

I found that 'make update-po' for PGXS does not work.
Even if execute 'make update-po', but xx.po.new is not generated.
I don't test and check for meson build system, but I post it tentatively.

I attached patch and test set.
'update-po' tries to find *.po files $top_srcdir, but there is no po-file in PGXS system because $top_srcdir is install directory.

Thanks. I think you have the order of the ifdef nest backwards; even in
the PGXS case we should have "ALL_LANGUAGES = $(AVAIL_LANGUAGES)" unless
we're making update-po. Here I present it the other way around.

Regards

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"World domination is proceeding according to plan" (Andrew Morton)

Attachments:

v2-0001-fix-update-po-for-the-PGXS-case.patchtext/x-diff; charset=utf-8Download

From 7c1e3d06f4ad09500cc233c91fc4108ef58ae638 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Tue, 20 Aug 2024 20:38:15 -0400
Subject: [PATCH v2] fix update-po for the PGXS case

Author: Ryo Matsumura <matsumura.ryo@fujitsu.com>
Discussion: https://postgr.es/m/TYCPR01MB113164770FB0B0BE6ED21E68EE8DCA@TYCPR01MB11316.jpnprd01.prod.outlook.com
---
 src/nls-global.mk | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/nls-global.mk b/src/nls-global.mk
index dfff472cb3..73a6db10a1 100644
--- a/src/nls-global.mk
+++ b/src/nls-global.mk
@@ -142,8 +142,13 @@ init-po: po/$(CATALOG_NAME).pot
 # For performance reasons, only calculate these when the user actually
 # requested update-po or a specific file.
 ifneq (,$(filter update-po %.po.new,$(MAKECMDGOALS)))
+ifdef PGXS
+ALL_LANGUAGES := $(shell find . -name '*.po' -print | sed 's,^.*/\([^/]*\).po$$,\1,' | LC_ALL=C sort -u)
+all_compendia := $(shell find . -name '*.po' -print | LC_ALL=C sort)
+else
 ALL_LANGUAGES := $(shell find $(top_srcdir) -name '*.po' -print | sed 's,^.*/\([^/]*\).po$$,\1,' | LC_ALL=C sort -u)
 all_compendia := $(shell find $(top_srcdir) -name '*.po' -print | LC_ALL=C sort)
+endif
 else
 ALL_LANGUAGES = $(AVAIL_LANGUAGES)
 all_compendia = FORCE
-- 
2.39.2

Bryan Green

dbryan.green@gmail.com

3 months ago

In reply to: Alvaro Herrera (#2)

Re: Buf fix: update-po for PGXS does not work

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 verify the fix, I put together a simple external extension with NLS support - just a basic C file with some errmsg/errhint calls, 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 for the 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, but in 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 searches the current directory instead, and make update-po successfully created all three .po.new files (fr, de, ja) using msgmerge. The generated files look good—proper msgid/msgstr entries and valid translation content. The extension builds fine 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 looks right to me.

The patch is clean, and I didn't encounter any issues during testing. There is no documentation, but for this BUG FIX, I did not expect any.

Looks good to me— I think it is ready for commit.

Bryan Green

Álvaro Herrera

alvherre@kurilemu.de

3 months ago

In reply to: Bryan Green (#3)

Re: Buf fix: update-po for PGXS does not work

On 2025-Oct-10, Bryan Green wrote:

I took a look at the v2 patch for the PGXS update-po bug, and it looks solid.

Many thanks for the dedication to testing it! I have pushed this now to
all branches, though I'm unsure if Matsumura-san still needs it.

Regards

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)