i18n: Introduce i18n.files() metedata wrapper #1757

sardemff7 · 2017-05-08T14:22:06Z

This would “fix” #1733, #1739 and replace #1630.

It is still RFC as the code is not completely clean and maybe not up to Meson standards (at least the test part).

Would that be an acceptable design?

ignatenkobrain · 2017-05-08T14:22:13Z

mesonbuild/modules/i18n.py


 from os import path
 from .. import coredata, mesonlib, build
 from ..mesonlib import MesonException
 from . import ModuleReturnValue
 from . import ExtensionModule
+from ..interpreterbase import InterpreterObject, MutableInterpreterObject


[flake8]

[F401] '..interpreterbase.MutableInterpreterObject' imported but unused

ignatenkobrain · 2017-05-08T14:22:13Z

mesonbuild/modules/i18n.py

-        '--add-comments',
+    'none': {
+        'args': [],
+        'keywords' : [],


[flake8]

[E203] whitespace before ':'

ignatenkobrain · 2017-05-08T14:22:14Z

mesonbuild/modules/i18n.py

-        '--flag=g_error_new:3:c-format',
-        '--flag=g_set_error:4:c-format',
-    ]
+        'keywords' : [


[flake8]

[E203] whitespace before ':'

ignatenkobrain · 2017-05-08T14:22:14Z

mesonbuild/modules/i18n.py

+
+    def _get_preset(self, kwargs):
+        preset = kwargs.pop('preset', self.preset)
+        if not preset in PRESET_ARGS:


[flake8]

[E713] test for membership should be 'not in'

ignatenkobrain · 2017-05-08T14:22:15Z

mesonbuild/modules/i18n.py

+        self.datadirs = I18nModule._get_data_dirs(self.environment, self.subdir, mesonlib.stringlistify(kwargs.get('data_dirs', [])))
+
+        self.preset = kwargs.pop('preset', 'none')
+        if not self.preset in PRESET_ARGS:


[flake8]

[E713] test for membership should be 'not in'

ignatenkobrain · 2017-05-08T14:22:15Z

mesonbuild/modules/i18n.py

+        preset = self._get_preset(kwargs)
+
+        files = {
+            'files': [ path.join(self.interpreter.subdir, f) for f in args ],


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

ignatenkobrain · 2017-05-08T14:22:16Z

mesonbuild/modules/i18n.py

+        datadirs = '--datadirs=' + ':'.join(self.datadirs) if self.datadirs else None
+        pot_data_file = None
+        extra_args = None
+        preset = self._get_preset(kwargs)


[flake8]

[F841] local variable 'preset' is assigned to but never used

ignatenkobrain · 2017-05-08T14:22:16Z

mesonbuild/modules/i18n.py

-            extra_args = set(preset_args + extra_args)
+            preset = PRESET_ARGS.get(preset)
+            extra_args = set(preset['args'] +
+                             [ '--keyword=' + k for k in preset['keywords'] ] +


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

ignatenkobrain · 2017-05-08T14:22:16Z

mesonbuild/modules/i18n.py

+            preset = PRESET_ARGS.get(preset)
+            extra_args = set(preset['args'] +
+                             [ '--keyword=' + k for k in preset['keywords'] ] +
+                             [ '--flag=' + f for f in preset['flags'] ] +


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

ignatenkobrain · 2017-05-08T14:22:17Z

mesonbuild/scripts/gettext.py

+    if datafilename:
+        with open(datafilename, 'rb') as ifile:
+            files = pickle.load(ifile)
+        tmp_pot = os.path.join(os.path.dirname(datafilename), 'i18n-' + pkgname  + '.pot')


[flake8]

[E221] multiple spaces before operator

ignatenkobrain · 2017-05-08T14:22:17Z

mesonbuild/scripts/gettext.py

+            os.remove(tmp_pot)
+        join_arg = []
+        for f in files:
+            language = [ '--language=' + f['language'] ] if 'language' in f else []


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

ignatenkobrain · 2017-05-08T14:22:18Z

mesonbuild/scripts/gettext.py

+        join_arg = []
+        for f in files:
+            language = [ '--language=' + f['language'] ] if 'language' in f else []
+            keywords = [ '--keyword=' + k for k in f['keywords'] ]


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

ignatenkobrain · 2017-05-08T14:22:18Z

mesonbuild/scripts/gettext.py

+        for f in files:
+            language = [ '--language=' + f['language'] ] if 'language' in f else []
+            keywords = [ '--keyword=' + k for k in f['keywords'] ]
+            flags = [ '--flag=' + f for f in f['flags'] ]


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

ignatenkobrain · 2017-05-08T14:22:19Z

mesonbuild/scripts/gettext.py

+            keywords = [ '--keyword=' + k for k in f['keywords'] ]
+            flags = [ '--flag=' + f for f in f['flags'] ]
+            subprocess.check_call(['xgettext', '--package-name=' + pkgname, '-p', src_sub,
+                                '-D', os.environ['MESON_SOURCE_ROOT'], '-o', tmp_pot] + join_arg


[flake8]

[E128] continuation line under-indented for visual indent

ignatenkobrain · 2017-05-08T14:22:19Z

mesonbuild/scripts/gettext.py

+            flags = [ '--flag=' + f for f in f['flags'] ]
+            subprocess.check_call(['xgettext', '--package-name=' + pkgname, '-p', src_sub,
+                                '-D', os.environ['MESON_SOURCE_ROOT'], '-o', tmp_pot] + join_arg
+                                + language + keywords + flags + f['extra_args'] + f['files'],


[flake8]

[W503] line break before binary operator

[E128] continuation line under-indented for visual indent

ignatenkobrain · 2017-05-08T14:22:20Z

mesonbuild/scripts/gettext.py

+            subprocess.check_call(['xgettext', '--package-name=' + pkgname, '-p', src_sub,
+                                '-D', os.environ['MESON_SOURCE_ROOT'], '-o', tmp_pot] + join_arg
+                                + language + keywords + flags + f['extra_args'] + f['files'],
+                               env=child_env)


[flake8]

[E128] continuation line under-indented for visual indent

ignatenkobrain · 2017-05-08T14:22:20Z

mesonbuild/scripts/gettext.py

+                                + language + keywords + flags + f['extra_args'] + f['files'],
+                               env=child_env)
+            if os.path.exists(tmp_pot):
+                join_arg = [ '-j' ]


[flake8]

[E201] whitespace after '['

[E202] whitespace before ']'

jpakkane · 2017-05-21T18:43:32Z

It's unfortunate that the user has to write pot.process() to get things running and if they forget it then nothing happens. It the very least it should guard against being called multiple times.

ignatenkobrain · 2017-05-23T16:23:39Z

mesonbuild/modules/i18n.py

+        datadirs = '--datadirs=' + ':'.join(self.datadirs) if self.datadirs else None
+        pot_data_file = None
+        extra_args = None
+        preset = self._get_preset(kwargs)


[flake8]

[F841] local variable 'preset' is assigned to but never used

ignatenkobrain · 2017-05-23T16:23:39Z

mesonbuild/scripts/gettext.py

+            flags = ['--flag=' + f for f in f['flags']]
+            subprocess.check_call(['xgettext', '--package-name=' + pkgname, '-p', src_sub,
+                                   '-D', os.environ['MESON_SOURCE_ROOT'], '-o', tmp_pot] + join_arg +
+                                   language + keywords + flags + f['extra_args'] + f['files'],


[flake8]

[E127] continuation line over-indented for visual indent

sardemff7 · 2017-05-23T16:24:43Z

pot.process() is not needed any more, at the cost of a rewrite of the .dat file for each pot.add_files().

codecov-io · 2017-05-23T17:33:53Z

Codecov Report

Merging #1757 into master will decrease coverage by 0.01%.
The diff coverage is 59.52%.

@@            Coverage Diff             @@
##           master    #1757      +/-   ##
==========================================
- Coverage   63.84%   63.82%   -0.02%     
==========================================
  Files          71       71              
  Lines       17086    17238     +152     
  Branches     3517     3556      +39     
==========================================
+ Hits        10908    11002      +94     
- Misses       5098     5137      +39     
- Partials     1080     1099      +19

Impacted Files	Coverage Δ
mesonbuild/scripts/gettext.py	`0% <0%> (ø)`	⬆️
mesonbuild/scripts/msgfmthelper.py	`0% <0%> (ø)`	⬆️
mesonbuild/modules/i18n.py	`68.75% <70.42%> (+3.41%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b52955f...cd1802f. Read the comment docs.

TingPing · 2017-05-23T17:45:31Z

docs/markdown/i18n-module.md

+  pot.add_merge_files(*file1*, *file2*, ...)
+```
+
+This method is a shortcut to `pot.add_files()` then `pot.merge_files()` on a set of files.


Is there a case when you want merge_files() but never to add them?

Not me, but there is i18n.merge_file() and you’re not forced to call i18n.gettext() so I just wanted to keep that option.
Also, you cannot use extra_args iwth pot.add_merge_files() because there is no way to know which are for the xgettext call and which are for the msgfmt call. Since I wanted to keep extra_args as the standard name, and not having xgettext_extra_args and msgfmt_extra_args, this is the simplest way.

Just let me know what you prefer I’ll happily rework the code to fit. :-)

sardemff7 · 2017-05-23T23:22:25Z

Btw, should I write (a) new test(s) or add to the existing one?

jpakkane · 2017-05-29T18:26:51Z

Whatever feels most natural to you.

jpakkane · 2017-06-27T15:46:54Z

⚔️ conflicts. Also needs a mention in release notes.

jpakkane · 2017-06-30T15:32:53Z

This goes against the grain of current design of not using mutable objects. Mutating methods are not nice. Could this be converted behave like other parts do is to have Files() calls in various subdirs and then use all of them in the pot call invocation.

sardemff7 · 2017-07-01T09:24:05Z

That would require more than just files(), since we need to associate keywords, language (file language) and xgettext extra args with each set of files. An i18n.files() could work, as a files() wrapper with extra kwargs, then you pass them as a list to i18n.create_pot().
That would be a tiny bit less flexible as for merge_files() since it would not allow future extra_args to the msgfmt call (the reason I added separate pot.add_merge_files() and pot.merge_files() initially) but if you’re fine with a maybe future msgfmt_extra_args kwarg, that’ll do.

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>

ignatenkobrain · 2017-07-15T13:25:36Z

mesonbuild/modules/i18n.py

+            elif isinstance(f, mesonlib.File):
+                files.append(f)
+            elif isinstance(f, list):
+                files += self._flatten_files(f, source_dir, subdir)


[flake8]

[F821] undefined name 'self'

ignatenkobrain · 2017-07-15T13:25:36Z

mesonbuild/modules/i18n.py

+
+        MERGE_LANGUAGES = ('xml', 'desktop')
+        if self.language not in MERGE_LANGUAGES:
+            raise MesonException('i18n: "{}" is not a valid merge language {}'.format(language, MERGE_LANGUAGES))


[flake8]

[F821] undefined name 'language'

ignatenkobrain · 2017-07-15T13:25:37Z

mesonbuild/modules/i18n.py

+            elif isinstance(f, mesonlib.File):
+                files.append(f)
+            elif isinstance(f, list):
+                r = self._flatten_files(f)


[flake8]

[F821] undefined name 'self'

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>

sardemff7 · 2017-07-15T13:37:59Z

New design, you now create files object, containing a list of files and metadata, then pass all of them to .create_pot() to actually create all the needed targets.

As said on IRC, there is still an issue, I generate the .pot file in meson-private first (incrementally, with several xgettext calls) but Python fails to move it back to the source dir if they are on different filesystems. They are some possible fixes and workarounds, but if it is an acceptable issue, it’s probably the cleanest design and code.

sardemff7 · 2017-07-16T07:44:52Z

As a side note: i18n.files() does not install/merge files alone, it needs to be passed to i18n.create_pot(). I did not put that in the doc, as the files() does not do anything to the files either, and i18n.files() is advertise as a wrapper to add metadata.
Is that clear enough or should I be explicit about that? I was thinking about it in #837 since it could be done as a wrapper to i18n.files() with predefined values and a preset, but then the wrapper itself would also be “inactive” alone.

jpakkane · 2017-07-17T17:10:45Z

This is definitely better. But how about doing the encapsulation slightly differently:

pot_files = i18n.pot_file_list()
pot_files.add('foo.c', 'bar.c', ...)
... etc in other subdirs
# then finally do
foo = i18n.create_pot('projname', pot_files)

This would mirror the way configure_data works: first create an object to hold all data, then append stuff to it and finally use it.

sardemff7 · 2017-07-17T20:04:14Z

I thought objects where immutable? And that you didn’t like the “it needs final .process() to actually do something”. :-)
I don’t think mirroring configure_data() is meaningful here, we are not creating something, just associating metadata.
There are 3 needs that lead to this design:

Meson needs the list of files and associated keywords to create the pot file → you create objects to keep that, then pass them to .create_pot()
Meson needs the pot file directory to merge translations → the custom targets are generated in .create_pot()
you wanted immutable objects

FTR, the first design was: .create_pot() → add_files() → .process() and the second one was .create_pot() → add_files()

nirbheek · 2017-07-20T08:42:37Z

FWIW, I like the 'associate metadata with files' design. We might need this for more things in the future, so it is a good precedent. For instance, currently we guess the file type based on the extension throughout meson, but some day we may need to change that.

krader1961 · 2021-12-15T03:20:07Z

Christ on a Cracker. Please close ancient pull-requests like this one. Either it is still relevant and should be merged (possibly with changes) or it is irrelevant and should be rejected.

sardemff7 · 2021-12-20T09:43:28Z

IMHO, it is still relevant, even though I didn’t have time to keep it up to date
I just felt like there was no consensus about the macro design of this feature, and that I should wait before re-writing the whole thing again

jpakkane · 2022-02-14T11:12:53Z

Why does one need per-file gettext flags? Meson's design has been to not permit those in general, because they add a lot of complexity in both implementing the feature and to build files (i.e. the "where do these unexpected flags in my invocations come from, I never specify them" problem).

sardemff7 · 2022-02-17T09:20:07Z

It is less per-file as it is per-language
You may not want to use the same extraction keywords for C or Python files
However, you probably want only one .pot file, so you have to merge all the extractions as some point
You can see the description of #1739 for an example
Please note that some formats (like desktop files) are quite generic and you cannot guess all the extensions people would use (e.g. I use .action in some project)

ignatenkobrain reviewed May 8, 2017

View reviewed changes

nirbheek assigned TingPing May 9, 2017

sardemff7 force-pushed the wip/i18n-custom branch from 666a6c2 to 7ae132c Compare May 23, 2017 16:23

ignatenkobrain reviewed May 23, 2017

View reviewed changes

TingPing reviewed May 23, 2017

View reviewed changes

sardemff7 force-pushed the wip/i18n-custom branch from 7ae132c to f3ad0a5 Compare June 4, 2017 12:14

sardemff7 changed the title ~~[RFC] i18n: Add a pot file object~~ i18n: Add a pot file object Jun 4, 2017

sardemff7 force-pushed the wip/i18n-custom branch from f3ad0a5 to 578af85 Compare June 27, 2017 16:42

TingPing mentioned this pull request Jul 5, 2017

xdg module for common freedesktop tasks #837

Open

i18n: Fix permitted kwargs

c98ddf8

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>

sardemff7 force-pushed the wip/i18n-custom branch from 578af85 to 1d8f69e Compare July 15, 2017 13:25

sardemff7 changed the title ~~i18n: Add a pot file object~~ i18n: Introduce i18n.files() metedata wrapper Jul 15, 2017

ignatenkobrain reviewed Jul 15, 2017

View reviewed changes

i18n: Introduce i18n.files() metedata wrapper

cd1802f

Signed-off-by: Quentin Glidic <sardemff7+git@sardemff7.net>

sardemff7 force-pushed the wip/i18n-custom branch from 1d8f69e to cd1802f Compare July 15, 2017 13:32

TingPing mentioned this pull request Aug 22, 2017

i18n: install .mo files for immediate use in the builddir #2227

Closed

sardemff7 mentioned this pull request Aug 24, 2017

i18n: Allow to pass extra arguments to msgfmt #1630

Closed

TingPing mentioned this pull request Nov 13, 2017

i18n.merge_file does not generate a dependency on the .po files #2621

Open

TingPing removed their assignment Feb 28, 2022

i18n: Introduce i18n.files() metedata wrapper #1757

Are you sure you want to change the base?

i18n: Introduce i18n.files() metedata wrapper #1757

Conversation

sardemff7 commented May 8, 2017

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpakkane commented May 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sardemff7 commented May 23, 2017

codecov-io commented May 23, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sardemff7 commented May 23, 2017

jpakkane commented May 29, 2017

jpakkane commented Jun 27, 2017

jpakkane commented Jun 30, 2017

sardemff7 commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sardemff7 commented Jul 15, 2017

sardemff7 commented Jul 16, 2017

jpakkane commented Jul 17, 2017

sardemff7 commented Jul 17, 2017

nirbheek commented Jul 20, 2017

krader1961 commented Dec 15, 2021

sardemff7 commented Dec 20, 2021

jpakkane commented Feb 14, 2022

sardemff7 commented Feb 17, 2022 • edited

codecov-io commented May 23, 2017 •

edited

sardemff7 commented Feb 17, 2022 •

edited