Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build system refactoring phase 8: update translation system #238

Closed
wants to merge 6 commits into from

Conversation

pspacek
Copy link
Contributor

@pspacek pspacek commented Nov 14, 2016

This patch set moves IPA translation system towards standard Makefiles produced by gettextize framework.

Depends on #233 .

@MartinBasti
Copy link
Contributor

Lint failed

cd .; ./makeaci --validate
./makeaci: ipaserver/plugins/dogtag.py:244: ignoring ImportError: No module named backports_abc
cd .; ./makeapi --validate
./makeapi: ipaserver/plugins/dogtag.py:244: ignoring ImportError: No module named backports_abc
make -C ./po validate-src-strings
make[1]: Entering directory '/freeipa/po'
make[1]: Leaving directory '/freeipa/po'
make[1]: *** No rule to make target 'validate-src-strings'.  Stop.
make: *** [polint] Error 2
Makefile:1098: recipe for target 'polint' failed

@MartinBasti MartinBasti self-assigned this Nov 14, 2016
@lslebodn
Copy link
Contributor

Even after fixing lint issues there is a different issue

make[2]: Entering directory '/home/user/freeipa/po'
make[2]: *** No rule to make target 'distdir'.  Stop.
make[2]: Leaving directory '/home/user/freeipa/po'
make[1]: *** [Makefile:701: distdir] Error 1
make[1]: Leaving directory '/home/user/freeipa'
make: *** [Makefile:1068: rpms] Error 2

@lslebodn
Copy link
Contributor

and also

Making install in po
make[1]: Entering directory '/home/user/freeipa/po'
make[1]: *** No rule to make target 'install'.  Stop.
make[1]: Leaving directory '/home/user/freeipa/po'
make: *** [Makefile:595: install-recursive] Error 1

@lslebodn
Copy link
Contributor

The following files should be removed from git m4/gettext.m4 m4/iconv.m4 m4/lib-ld.m4 m4/lib-link.m4 m4/lib-prefix.m4 m4/nls.m4 m4/po.m4.

The latest versions should be used by autoreconf after installing build dependency gettext-devel.
Otherwise it's possible that tarball might contain outdated versions in future.

@pspacek pspacek force-pushed the build-phase-8 branch 2 times, most recently from 7e5038e to 410ed6b Compare November 16, 2016 12:37
@pspacek
Copy link
Contributor Author

pspacek commented Nov 16, 2016

This is rebased and fixed version. It should work including linters. Missing things:

  • use fresh gettext files generated by autoreconf

@pspacek
Copy link
Contributor Author

pspacek commented Nov 16, 2016

As far as I can tell all the nits mentioned above are addressed in the last version. Enjoy review :-)

@tiran
Copy link
Member

tiran commented Nov 17, 2016

Build is failing:

Can't exec "autopoint": No such file or directory at /usr/share/autoconf/Autom4te/FileUtils.pm line 345.
autoreconf: failed to run autopoint: No such file or directory
autoreconf: autopoint is needed because this package uses Gettext

The command is provided by gettext-devel:

BuildRequires: gettext-devel

@pspacek
Copy link
Contributor Author

pspacek commented Nov 18, 2016

Good catch, fixed & rebased on top of current master.

@tiran
Copy link
Member

tiran commented Nov 18, 2016

Your patch adds config.rpath. Is it necessary to include the file in source control? certmonger and sssd use the file but don't have it in git.

@lslebodn
Copy link
Contributor

Your patch adds config.rpath. Is it necessary to include the file in source control? certmonger and sssd use the file but don't have it in git.

Following patch fixes this issue
http://paste.fedoraproject.org/484212/47049414/

We now use standard framework generatedby "gettextize" utility.

It has two limitations which I do not consider sufficiently important
to invest into hand-made solution:

1. It can automatically gather strings only from files which have some
   file extension like .c or .py. Right now we do not have any
   translatable strings in Python files without extensions. Given that these
   files will be removed from source tree and replaced with entry points
   from setuptools I do not see a reason to invest into supporting this.

2. It does not automatically strip untranslated strings from po files.
   This is a manual step in mainteiner's in workflow anyway so I will
   add separate Makefile target for it later on.

This commit contains gettextize instrastructure + filled-in files
Makevars and POTFILES.in.

https://fedorahosted.org/freeipa/ticket/6418
The target was added to top-level Makefile.am as well so the maintainer
does not need to jump between directories when doing Zanata pull/push
and strip-po.

https://fedorahosted.org/freeipa/ticket/6418
Editing work is done in Zanata UI so there is no point in keeping all
versions around in SCM.

https://fedorahosted.org/freeipa/ticket/6418
…gure

configure is easiest option how to automatically generate POTFILES.in.
Attempts to add it to po/Makefile* have big potential to create cyclic
depedencies and cause other trouble.

Given how rare operation adding a source file is, I think it is sufficient
to document that configure needs to be run again after adding a source file
with translatable strings.

https://fedorahosted.org/freeipa/ticket/6418
The Makefile test targets were lost when gettextize infrastructure was
introduced. Now it is re-added in its modernized form which counts with
generated .pot files.

ipatests/i18n.py is now explicitly setting character encoding in files
it generates. According to gettext manual chapter "Filling in the Header Entry"
the Content-Type header is language-specific so it does not make sense
to fill it in in .pot file.

https://fedorahosted.org/freeipa/ticket/6418
All the source files are in the very same repo so there is no point
in keeping the file in Git.

https://fedorahosted.org/freeipa/ticket/6418
@pspacek
Copy link
Contributor Author

pspacek commented Nov 21, 2016

@tiran You are right, I forgot to remove the config.rpath when AM_GNU_GETTEXT_VERSION macro was introduced. This version fixes this problem by removing config.rpath from Git.

@lslebodn We can do this change in some later PR. I do not want to mix it with translation system changes.

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Nov 22, 2016
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Nov 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants