Discussion:
Please test pre-commit hook for po files
(too old to reply)
u
2018-05-29 09:38:00 UTC
Permalink
Raw Message
Hi!

in order to integrate our web translation platform with our
infrastructure, we are trying to create more rules for PO files, used
for the translation of our website. At some point, all PO files should
follow these rules and the easiest way to ensure this is to install a
pre-commit hook. We will also verify this upon push, so in the future
it'll be necessary to ensure already upon commit that things work the
way they should.

But before enforcing this policy, we'd like you to test a pre-commit
hook. The hook verifies that no PO file headers are missing and that
they are set to a specific team address, wraps the files to 79 chars per
line, checks for whitespace errors. This is necessary in order to avoid
merge conflicts on these headers, something which we've seen occur very
often but that we want to automate without humans having to intervene.

In order to make this happen please:

git fetch tails translation_platform_hooks (assuming tails.git is
called tails as a remote)
git submodule update --init
cd .git/hooks
ln -s ../../wiki/src/contribute/l10n_tricks/pre-commit .

Now, everytime you try to commit a PO file, check_po will run. We've
modified check_po to only check the files you're trying to commit, not
run on the entire repository.

We have not automatically cleaned the PO files, but we will do that at
some point, upon merging these changes into master. This means that if
your files are currently not wrapped at 79 chars, you'll have to do that
:) We've chosen this wrapping because that's Poedit's default setting
and we want to make sure all other tools wrap at the same width.

But: hefee has prepared a script available under
wiki/src/contribute/l10n_tricks/unify_po-headers.sh which can help to
clean the files before trying to commit them :)

It's also "easy" to rewrap your po files using
msgcat -w 79 $FILENAME -o "$FILENAME"

Please report back here if you encounter problems or have ideas for
improvement.

Corresponding tickets if you want to comment there:
- https://labs.riseup.net/code/issues/15605 Make check_po.sh optionally
accept a list of files
- https://labs.riseup.net/code/issues/15408 Consider forcing wrapping of
po files at 79 chars per line
- https://labs.riseup.net/code/issues/15362 Run check_po whenever we try
to commit a po file in all Git repositories

Cheers!
hefee & u.
intrigeri
2018-05-29 10:26:19 UTC
Permalink
Raw Message
Post by u
ln -s ../../wiki/src/contribute/l10n_tricks/pre-commit .
This caught my eye before I could test this.

I'd rather not ask all Tails contributors to run code, on every
commit, that lives in a section of our website that's publicly
writable. Please consider moving this script to bin/ :)
u
2018-05-29 13:38:00 UTC
Permalink
Raw Message
Hi!
Post by intrigeri
Post by u
ln -s ../../wiki/src/contribute/l10n_tricks/pre-commit .
This caught my eye before I could test this.
I'd rather not ask all Tails contributors to run code, on every
commit, that lives in a section of our website that's publicly
writable. Please consider moving this script to bin/ :)
With a notion of 'public' that allows only some people to write here, right?

I moved it to bin/ and will resend the email now.

Cheers!
u.
intrigeri
2018-05-29 14:43:03 UTC
Permalink
Raw Message
Post by u
Post by intrigeri
Post by u
ln -s ../../wiki/src/contribute/l10n_tricks/pre-commit .
This caught my eye before I could test this.
I'd rather not ask all Tails contributors to run code, on every
commit, that lives in a section of our website that's publicly
writable. Please consider moving this script to bin/ :)
With a notion of 'public' that allows only some people to write here, right?
In theory, yes. I was definitely over-simplifying things above.
You seem to be interested in the longer version of my reasoning so
I'll write it up:

- I believe that the only thing that prevent ikiwiki.cgi from
allowing anyone with an Internet connection to edit arbitrary files
under wiki/src/ is our lockedit plugin configuration.
There's already been security issues in this part of the ikiwiki
code so I'd rather not rely on it when we can cheaply avoid it.

- For various reasons we tend to review changes under wiki/src/ less
carefully than other changes so if someone exploited an ikiwiki bug
and modified that pre-commit hook, chances are their code would run
on a number of our systems before someone notices the problem.

So yeah, in theory, assuming no software bugs, it's safe to put such
code under wiki/src/; but it increases attack surface a fair bit, with
no substantial benefit I can think of, so let's err on the safe side,
as you did already, thanks!

Now, this hook runs wiki/src/contribute/l10n_tricks/check_po.sh so the
problem I'm describing above is still there. This could not fixed in
pre-commit hook by calling submodules/jenkins-tools/slaves/check_po
directly instead of going through the symlink.
Post by u
I moved it to bin/ and will resend the email now.
\o/

Cheers!
--
intrigeri
u
2018-05-30 07:40:00 UTC
Permalink
Raw Message
Hi!
Post by intrigeri
Post by u
Post by intrigeri
Post by u
ln -s ../../wiki/src/contribute/l10n_tricks/pre-commit .
This caught my eye before I could test this.
I'd rather not ask all Tails contributors to run code, on every
commit, that lives in a section of our website that's publicly
writable. Please consider moving this script to bin/ :)
With a notion of 'public' that allows only some people to write here, right?
- I believe that the only thing that prevent ikiwiki.cgi from
allowing anyone with an Internet connection to edit arbitrary files
under wiki/src/ is our lockedit plugin configuration.
There's already been security issues in this part of the ikiwiki
code so I'd rather not rely on it when we can cheaply avoid it.
Oops. I was not aware of that.
Post by intrigeri
So yeah, in theory, assuming no software bugs, it's safe to put such
code under wiki/src/; but it increases attack surface a fair bit, with
no substantial benefit I can think of, so let's err on the safe side,
as you did already, thanks!
If there's a place for such scripts, let's put them there :)
Post by intrigeri
Now, this hook runs wiki/src/contribute/l10n_tricks/check_po.sh so the
problem I'm describing above is still there. This could not fixed in
pre-commit hook by calling submodules/jenkins-tools/slaves/check_po
directly instead of going through the symlink.
Agreed. I'll modify this, this will be transparent for the testers.

Cheers!
u.
intrigeri
2018-06-19 16:31:13 UTC
Permalink
Raw Message
Post by intrigeri
Now, this hook runs wiki/src/contribute/l10n_tricks/check_po.sh so the
problem I'm describing above is still there. This could not fixed in
pre-commit hook by calling submodules/jenkins-tools/slaves/check_po
directly instead of going through the symlink.
Was this fixed?

Also, I'd like to do a code review before we merge this.
Happy to do that once I have a confirmation that the last problem I've
mentioned was fixed!
u
2018-06-22 10:25:00 UTC
Permalink
Raw Message
Hi!
Post by intrigeri
Post by intrigeri
Now, this hook runs wiki/src/contribute/l10n_tricks/check_po.sh so the
problem I'm describing above is still there. This could not fixed in
pre-commit hook by calling submodules/jenkins-tools/slaves/check_po
directly instead of going through the symlink.
Was this fixed?
Also, I'd like to do a code review before we merge this.
Happy to do that once I have a confirmation that the last problem I've
mentioned was fixed!
It was now.

A code review is welcome.

Cheers!
u.

u
2018-05-29 13:41:00 UTC
Permalink
Raw Message
Hi!

Upon a technical remark, I moved the pre-commit file to
bin/pre-commit-translation instead. See updated instructions below.

git fetch tails translation_platform_hooks
git submodule update --init
cd .git/hooks
ln -s ../../bin/pre-commit-translation pre-commit

Cheers!
u.
sajolida
2018-05-30 14:28:00 UTC
Permalink
Raw Message
Post by u
Upon a technical remark, I moved the pre-commit file to
bin/pre-commit-translation instead. See updated instructions below.
git fetch tails translation_platform_hooks
git submodule update --init
cd .git/hooks
ln -s ../../bin/pre-commit-translation pre-commit
Doing only `git fetch` doesn't change my checkout and nor creates
../../bin/pre-commit-translation locally.

So instead I did:

git checkout -t origin/translation_platform_hooks
git submodule update --init
cd .git/hooks
ln -s ../../bin/pre-commit-translation pre-commit

Then I did a dummy change to index.es.po and tried to commit it.

I got the following:

Project-Id-Version header is not empty for following files:
wiki/src/index.es.po

The po files you're trying to commit contain errors. Please fix them and try again.

Which make me believe that you didn't run your new PO file validator on all the current PO files.
If you're adding validity checks to our current PO files it would be nice to also make them comply
with them, otherwise:

- Translators (and mergers) will have to fix them one by one
- Ikiwiki on the production server might not be able to do the automatic "Update PO files"

I'm also curious about what `git submodule update --init` does. Are you using submodules for that?
I'm asking out of curiosity because I don't know much about submodules but I couldn't find spot what
is specific to your work under .git/modules/submodules.
--
sajolida
u
2018-05-30 15:58:00 UTC
Permalink
Raw Message
Hi!
Post by sajolida
Post by u
Upon a technical remark, I moved the pre-commit file to
bin/pre-commit-translation instead. See updated instructions below.
git fetch tails translation_platform_hooks
git submodule update --init
cd .git/hooks
ln -s ../../bin/pre-commit-translation pre-commit
Doing only `git fetch` doesn't change my checkout and nor creates
../../bin/pre-commit-translation locally.
Indeed!
Post by sajolida
git checkout -t origin/translation_platform_hooks
git submodule update --init
cd .git/hooks
ln -s ../../bin/pre-commit-translation pre-commit
Thanks for the updated version of the instructions.
Post by sajolida
Then I did a dummy change to index.es.po and tried to commit it.
wiki/src/index.es.po
The po files you're trying to commit contain errors. Please fix them and try again.
Which make me believe that you didn't run your new PO file validator on all the current PO files.
Absolutely, as said in my email, we did not do that yet and we'll do it
once people have tested it and we are ready to merge. At least that was
the initial plan.
Post by sajolida
If you're adding validity checks to our current PO files it would be nice to also make them comply
- Translators (and mergers) will have to fix them one by one
So, would you suggest we do that on master right now? Or should we
instead wait for more comments before making such a modification?
Post by sajolida
- Ikiwiki on the production server might not be able to do the automatic "Update PO files"
ikiwiki on the production server is currently not impacted by this hook
and we are aware that we will have to fix this, but it'll happen at a
later stage.
Post by sajolida
I'm also curious about what `git submodule update --init` does. Are you using submodules for that?
I'm asking out of curiosity because I don't know much about submodules but I couldn't find spot what
is specific to your work under .git/modules/submodules.
check_po.sh lives in a submodule and we have updated this script in a
branch of the same name. So people need to update - at least in case
they've never checked out the repository. I'm not 100% sure it's needed
in the case you have checked out the submodules, but I've put it in the
instructions to be sure.

Cheers!
u.
sajolida
2018-05-31 10:30:00 UTC
Permalink
Raw Message
Post by u
Post by sajolida
Then I did a dummy change to index.es.po and tried to commit it.
wiki/src/index.es.po
The po files you're trying to commit contain errors. Please fix them and try again.
Which make me believe that you didn't run your new PO file validator on all the current PO files.
Absolutely, as said in my email, we did not do that yet and we'll do it
once people have tested it and we are ready to merge. At least that was
the initial plan.
Indeed I missed that in your original email.
Post by u
Post by sajolida
If you're adding validity checks to our current PO files it would be nice to also make them comply
- Translators (and mergers) will have to fix them one by one
So, would you suggest we do that on master right now? Or should we
instead wait for more comments before making such a modification?
Another option would be to fix all the PO files in
translation_platform_hooks. This way people who test your branch will
work on a checkout that's already compatible with your new validation rules.
Post by u
Post by sajolida
- Ikiwiki on the production server might not be able to do the automatic "Update PO files"
ikiwiki on the production server is currently not impacted by this hook
and we are aware that we will have to fix this, but it'll happen at a
later stage.
Ok.

And I guess you also checked (or have a plan so) that the "Update PO
files" of ikiwiki on the production server are compatible with your new
validation rules, right?
Post by u
Post by sajolida
I'm also curious about what `git submodule update --init` does. Are you using submodules for that?
I'm asking out of curiosity because I don't know much about submodules but I couldn't find spot what
is specific to your work under .git/modules/submodules.
check_po.sh lives in a submodule and we have updated this script in a
branch of the same name. So people need to update - at least in case
they've never checked out the repository. I'm not 100% sure it's needed
in the case you have checked out the submodules, but I've put it in the
instructions to be sure.
Ah! Now I see this commit:

commit 53203d6f06
Author: intrigeri <***@boum.org>
Date: Fri Jun 12 20:58:51 2015 +0000

Replace check_po.sh with a symlink pointing to the same script in
the jenkins-tools submodule.

Refs: #9567

I didn't realize that earlier :)

Learning things every day...
--
sajolida
u
2018-06-12 12:23:00 UTC
Permalink
Raw Message
Hi!
Post by sajolida
Post by sajolida
If you're adding validity checks to our current PO files it would be nice to also make them comply
- Translators (and mergers) will have to fix them one by one
Actually, the cool thing is that we have a script for it :) See

./wiki/src/contribute/l10n_tricks/unify_po-headers.sh de

Where "de" is the language code of the files you want to fix.

I've prepared a branch with clean files nevertheless:
tails.gittranslation_platform_clean_po
Post by sajolida
And I guess you also checked (or have a plan so) that the "Update PO
files" of ikiwiki on the production server are compatible with your new
validation rules, right?
This will happen.

Please update again branch tails.git:translation_platform_hooks to get
the latest version of our scripts and git hooks.

Cheers!
u.
u
2018-06-12 12:32:00 UTC
Permalink
Raw Message
Hi!

I've prepared a branch that unifies all the PO headers of our PO files
and makes them follow our new rules.

I'd like to see this branch merged in to master after review:

tails.git:feature/10034+unify_po_files

It will likely create conflicts on Weblate, but I can help with this if
necessary.

If we manage to merge this now, people can test our git commit hooks for
real :) What do you think?

Cheers!
u.
intrigeri
2018-06-13 09:05:31 UTC
Permalink
Raw Message
Post by u
tails.git:feature/10034+unify_po_files
I think your branch includes unintended changes such as:

--- a/submodules/aufs4-standalone
+++ b/submodules/aufs4-standalone
@@ -1 +1 @@
-Subproject commit 84295fc49a4778dd60a4a3e9b3075e1b341a356d
+Subproject commit caea84a33dce187ad77f9ee524d7ec46acc69c63

Once this has been fixed, I'm happy to review any code changes
introduced by this branch via submodules :)

Cheers!
--
intrigeri
u
2018-06-19 08:53:00 UTC
Permalink
Raw Message
Hi!
Post by intrigeri
Post by u
tails.git:feature/10034+unify_po_files
--- a/submodules/aufs4-standalone
+++ b/submodules/aufs4-standalone
@@ -1 +1 @@
-Subproject commit 84295fc49a4778dd60a4a3e9b3075e1b341a356d
+Subproject commit caea84a33dce187ad77f9ee524d7ec46acc69c63
Once this has been fixed, I'm happy to review any code changes
introduced by this branch via submodules :)
I think this should be fixed. I've force pushed there because I needed
to update all files again.

Cheers!
u.
u
2018-06-19 09:35:00 UTC
Permalink
Raw Message
Fellow translators and developers,

three weeks ago I've asked on your lists to test a Git hook we've
implemented. We've improved this hook and created a script to make your
po files comply with this hook. Before we merge this and enable it by
default please test this. Deadline Friday 23:59 Berlin time, June 29th 2018.

Git hook for PO files
=====================

(Assuming tails.git is called `tails` as a remote.)
In order to enable the git hook (& assuming you have none enabled yet):

git checkout -t tails/translation_platform_hooks
git submodule update --init
cd .git/hooks
ln -s ../../bin/pre-commit-translation pre-commit

The hook will only run when you try to commit a PO file, not when
working on other files.

Expected format
===============

Now, running it as it, will likely result in errors, because the hook
assumes a certain format of PO files and its headers:

- PO files are supposed to be wrapped at 79 chars per line, this is
POedit's default.
- empty Project-Id-Version
- Last-Translator: Tails translators <tails-***@boum.org>
- Language-Team: Tails translators <tails-***@boum.org>

This is because we want to avoid merge conflicts when automatically
merging files on and from the translation platform.

Your name is still in the Git commit, so there is no need to worry about
attribution.

Make your files comply: unify PO headers
========================================

In order to make all files in your language comply you can run our
script in the same branch, where $LANG is your language code, for
example portuguese:

LANG=pt
git checkout -t origin/translation_platform_hooks
./wiki/src/contribute/l10n_tricks/unify_po-headers.sh $LANG

Now, look at the `git diff`.
Commit all modified files locally.

Then try to edit a PO file like you usually do and see if you can
commit. If you can't:
- try to understand the error message,
- fix it or report back here if it's impossible or you do not understand
the error.

I've also asked for a merge into tails/master of PO files in all
languages cleaned with this unification script but this did not yet
happen. When it will, I'll let you know.

In the future, you could decide to run the unify_po-headers script every
time you want to commit PO files and it'll take care of reformatting
your files.

Cheers!
u.
Loading...