New find_xgettext method to i18n module #13900

bruchar1 · 2024-11-12T13:45:08Z

This method return a PotExtractor object used to extract translatable
string from source files into a .pot translation template.

It differs from a plain CustomTarget in three ways:

It accepts build targets as sources, and automatically resolves source
files from those build targets;
It detects command lines that are too long, and writes, at config
time, the list of source files into a text file to be consumed by the
xgettext command;
It detects dependencies between pot extraction targets, based on the
dependencies between source targets.

This does the specific thing I need for my project, that I was trying to implement in a more generic way in #12272 and #11822.

docs/markdown/i18n-module.md

bonzini · 2024-12-06T13:54:43Z

docs/markdown/i18n-module.md

@@ -74,3 +74,72 @@ for normal keywords. In addition it accepts these keywords:
 * `mo_targets` *required*: mo file generation targets as returned by `i18n.gettext()`.

 *Added 0.62.0*
+
+
+### i18n.pot_extractor()


I'd prefer a more specific name like find_xgettext for this function and XgettextProgram for the object.

While I'm open changing the names, I'm not sure about find_xgettext, because I think it would suggest that the returned object is compatible with was is returned by find_program. Or should it be?

Actually, I think this is a neat idea. I modified it to make the returned object a subclass of ExternalProgram.

mesonbuild/modules/i18n.py

@@ -115,6 +134,161 @@
 }


+class XgettextProgram(ExternalProgram):


This method return a XgettextProgram object used to extract translatable string from source files into a .pot translation template. It differs from a plain CustomTarget in three ways: - It accepts build targets as sources, and automatically resolves source files from those build targets; - It detects command lines that are too long, and writes, at config time, the list of source files into a text file to be consumed by the xgettext command; - It detects dependencies between pot extraction targets, based on the dependencies between source targets.

dcbaker · 2024-12-10T18:34:15Z

mesonbuild/modules/i18n.py

+    @typed_kwargs(
+        'i18n.find_xgettext',
+        _ARGS,
+        KwargInfo('required', bool, default=True),


Why not use the shared option that also handles UserFeatureOptions? Or add the disabler argument. There may be something I'm missing here.

dcbaker · 2024-12-10T18:44:28Z

mesonbuild/modules/i18n.py

+        for dep in depends:
+            source_list += '\n' + path.join(dep.subdir, dep.get_filename())
+
+        estimated_cmdline_length = len(source_list) + len(arguments) + sum(len(arg) for arg in arguments) + 1


I think this code is correct, but it looks really odd because of the way your count the spaces in arguments, I think:

# count the space between each argument as well estimated_cmdline_length = len(source_list) + sum(len(arg) + 1 for arg in arguments) + 1

would be more clear

dcbaker · 2024-12-10T18:47:42Z

mesonbuild/modules/i18n.py

+
+        estimated_cmdline_length = len(source_list) + len(arguments) + sum(len(arg) for arg in arguments) + 1
+        if estimated_cmdline_length < 8000:
+            # Maximum command line length on Windows is 8191


We have a helper function to calculate an rsp threshold in the ninja backend, which includes an environment variable to tune whether we use an rsp file or not. I wonder if it would be appropriate to use that function here (maybe moved to utils.universal)? (or possibly it would make more sense to have a way for custom_targets to specify that they can use an RSP file, but that seems like a large out of scope project.

dcbaker · 2024-12-10T18:48:59Z

mesonbuild/modules/i18n.py

+class XgettextProgram(ExternalProgram):
+
+    def __init__(self, xgettext: ExternalProgram, args: T.List[str]):
+        self.__dict__.update(xgettext.__dict__)


I don't love this, but I understand why you did this.

eli-schwartz · 2024-12-10T19:03:00Z

docs/markdown/i18n-module.md

+- merge `bool`:
+        if `true`, will merge the resulting pot file with extracted pot files
+        related to dependencies of the given source targets. For instance,
+        if you build an executable, then you may want to merge the executable
+        translations with the translations from the dependent libraries.


This looks like it should be called "recursive" instead of "merge".

eli-schwartz · 2024-12-10T19:05:09Z

docs/markdown/i18n-module.md

+- alias `list[build_tgt]`:
+        a list of build targets using the same resulting pot file. For
+        instance, if you build both a static and a shared library from the
+        same sources, you may specify the static library target as an alias,
+        to use the generated pot file when that static library is used as a
+        dependency of another target.


I don't really understand what this is doing. But it seems backwards somehow.

eli-schwartz · 2024-12-10T19:09:00Z

docs/markdown/snippets/i18n_extract.md

+## i18n module find_xgettext
+
+There is a new `find_xgettext` function in `i18n` module that acts as a
+wrapper around `xgettext`. It allows to extract strings to translate from
+source files.
+
+This function is convenient, because:
+- It can find the sources files from a build target;
+- It will use an intermediate file when the number of source files is too
+  big to be handled directly from the command line;
+- It is able to get strings to translate from the dependencies of the given
+  targets.


Ultimately the goal here is just to avoid the i18n.gettext() assumption that POTFILES is the source of truth for xgettext, right?

It feels like this function adds complexity which doesn't directly serve that. In particular, it seems odd to have a function which finds the program, and then have that yield an object with a single method. We could instead directly provide an i18n.xgettext() function with the same API contract which internally looks up xgettext, no?

eli-schwartz · 2024-12-10T19:15:08Z

Since the underlying goal as I understand it is essentially "i18n.gettext() is not suitable for my use case because it depends on POTFILES but I would like to calculate the list from meson.build", it feels a bit like a more appropriate solution here could be to teach i18n.gettext() to take an optional override of files to extract from (and e.g. resolve build targets by recursing into their source filelist, if desirable) -- and then generate a POTFILES for you, basically.

bonzini · 2024-12-10T19:35:36Z

and then generate a POTFILES for you, basically.

I understood that he also wants to generate multiple .pot files, so that would not be a single POTFILES.

bruchar1 requested a review from jpakkane as a code owner November 12, 2024 13:45

bruchar1 force-pushed the i18nextract branch 4 times, most recently from 9ebc09e to 4f5283b Compare November 12, 2024 16:23

This was referenced Nov 14, 2024

New dict.map() method #13198

Closed

New unique() method on list #13195

Closed

bonzini suggested changes Dec 6, 2024

View reviewed changes

bruchar1 force-pushed the i18nextract branch 2 times, most recently from b35d6b7 to 9347ef2 Compare December 6, 2024 16:51

bruchar1 added the module:i18n label Dec 6, 2024

github-advanced-security bot found potential problems Dec 6, 2024

View reviewed changes

bruchar1 changed the title ~~New pot_extrator method to i18n module~~ New find_xgettext method to i18n module Dec 6, 2024

bruchar1 added 2 commits December 6, 2024 15:25

fixes for mypy

bf64973

bruchar1 force-pushed the i18nextract branch from 9347ef2 to 03d8165 Compare December 6, 2024 20:25

dcbaker requested changes Dec 10, 2024

View reviewed changes

eli-schwartz reviewed Dec 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New find_xgettext method to i18n module #13900

New find_xgettext method to i18n module #13900

bruchar1 commented Nov 12, 2024

bonzini Dec 6, 2024

bruchar1 Dec 6, 2024

bruchar1 Dec 6, 2024

dcbaker Dec 10, 2024

dcbaker Dec 10, 2024 •

edited

Loading

dcbaker Dec 10, 2024 •

edited

Loading

dcbaker Dec 10, 2024

eli-schwartz Dec 10, 2024

eli-schwartz Dec 10, 2024

eli-schwartz Dec 10, 2024

eli-schwartz commented Dec 10, 2024

bonzini commented Dec 10, 2024

		@@ -115,6 +134,161 @@
		}


		class XgettextProgram(ExternalProgram):

New find_xgettext method to i18n module #13900

Are you sure you want to change the base?

New find_xgettext method to i18n module #13900

Conversation

bruchar1 commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

dcbaker Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz commented Dec 10, 2024

bonzini commented Dec 10, 2024

dcbaker Dec 10, 2024 •

edited

Loading

dcbaker Dec 10, 2024 •

edited

Loading