Tails

#4 Updated by bertagaz 2018-05-10 11:09:12

• Target version changed from Tails_3.7 to Tails_3.8

#5 Updated by Anonymous 2018-05-17 14:08:37

• related to Feature #15364: Create .gitattributes with merge strategy for po files added

#6 Updated by Anonymous 2018-05-17 14:09:01

• Status changed from Confirmed to In Progress

#7 Updated by Anonymous 2018-05-17 14:13:04

• Feature Branch set to tails.git:translation_platform_hooks

#8 Updated by Anonymous 2018-05-17 14:15:10

• related to Bug #15605: Make check_po optionally accept a list of files added

#9 Updated by hefee 2018-05-17 16:47:14

• % Done changed from 0 to 60

Now available under wiki/src/contribute/l10n_tricks/unify_po-headers.sh
see my fork at:
https://0xacab.org/Hefee/tails/commit/cf132794b22763cc6fc82dae3934f70a3ace2bc8

#10 Updated by Anonymous 2018-06-19 09:09:29

• QA Check set to Pass

This file has been added to tails.git:translation_platform_hooks and improved upon (added 79 char limit).

# Rewrap po file to 79 chars
find -wholename ./tmp -prune -o \( -iname "$FILE_GLOB" -print0 \) \
        | xargs -0 --max-procs="$CPUS" --max-args=64 -I {} \
        msgcat -w 79 "{}" -o "{}"

I’ve tested it and it works well.

#12 Updated by Anonymous 2018-06-19 09:36:31

• Assignee deleted (hefee)

Let’s wait until Friday 29th 2018, 23:59h Berlin time.

#15 Updated by Anonymous 2018-06-21 14:42:07

• Assignee set to intrigeri
• QA Check changed from Pass to Ready for QA

Hi intrigeri, another code review is welcome. Please reassign to me when done.

#16 Updated by intrigeri 2018-06-22 21:48:48

• Target version changed from Tails_3.8 to Tails_3.9

#17 Updated by intrigeri 2018-06-23 07:33:41

• Target version changed from Tails_3.9 to Tails_3.8

I suppose it would smooth your work if this gets merged in time for 3.8. So I’ll try to review and hopefully merge this despite the tight timing.

#18 Updated by intrigeri 2018-06-23 07:58:20

• related to deleted (Bug #15605: Make check_po optionally accept a list of files)

#19 Updated by intrigeri 2018-06-23 07:58:22

• blocked by Bug #15605: Make check_po optionally accept a list of files added

#20 Updated by intrigeri 2018-06-23 08:36:49

• Assignee deleted (intrigeri)
• QA Check changed from Ready for QA to Dev Needed

If I merge this as-is, then the new checks added to check_po.sh will break various CI jobs because the PO files on this branch don’t satisfy the new requirements introduced by this branch. So I can’t merge this myself before merging into master you’ll need to:

• fix the PO files on the topic branch
• merge the topic branch of jenkins-tools once I’m done with my review on Bug #15605
• update the jenkins-tools submodule to its latest version (Bug #15605#note-14) on the topic branch; beware, I’ve pushed a trivial typo fix on top of your branch there

One related question: does ikiwiki itself generate PO files that satisfy the new requirements introduced by this branch? I hope so but if not, then we have a problem: every time ikiwiki on our live website updates/creates a PO file we will get test suite failures.

Code review:

• I don’t get why you pass --exit-code to git diff: the exit code seems to be ignored (as long as you don’t use set -e, see below).
• POLIST is a bit too generic a name. What about STAGED_PO_FILES?
• Please quote ${path}/./submodules/jenkins-tools/slaves/check_po: otherwise, if one has spaces in $path things will break. For the same reason, perhaps the {} placeholder in the sed commands should be quoted. Thankfully it’s easy to test if it causes trouble in its current state with a tails.git working copy in a directory that has spaces in its path.
• I’m not sure about the whitespace check: it’s run with exec so I don’t get how it can block the commit. Am I missing something? Anyway, it’s rather off-topic here so an option would be to skip this discussion now, remove this check for now so it does not block this ticket, and move that check to a new, dedicated ticket.
• It would be nice to use set -e and set -u consistently, as we do in basically every shell script of ours.
• DRY (“don’t repeat yourself”) in unify_po-headers.sh: there are 5 instances of the find -wholename … code
• I don’t speak sed, in particular when not used with its default regexp language. Generally we use --regexp-extended everywhere, which matches closely the regexp language supported by most programming languages. This is not a blocker but here we’re introducing code that will be hard to maintain. On the mid-term I’d rather see this script rewritten in Python/Perl/Ruby.
• Multiple typos in “>nother option would be to call python/perl to get a reex”.

#22 Updated by intrigeri 2018-06-26 16:27:48

• Target version changed from Tails_3.8 to Tails_3.9

#23 Updated by Anonymous 2018-08-19 17:39:57

• Assignee set to hefee

Hi! can you please look at the review above and then reassign to me? thanks!

#26 Updated by hefee 2018-08-28 22:41:34

• Assignee deleted (hefee)
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from tails.git:translation_platform_hooks to https://0xacab.org/Hefee/tails/tree/translation_platform_hooks

intrigeri wrote:

> * Please quote ${path}/./submodules/jenkins-tools/slaves/check_po: otherwise, if one has spaces in $path things will break. For the same reason, perhaps the {} placeholder in the sed commands should be quoted.

quoted {} placeholder in check_po

> * I don’t speak sed, in particular when not used with its default regexp language. Generally we use --regexp-extended everywhere, which matches closely the regexp language supported by most programming languages. This is not a blocker but here we’re introducing code that will be hard to maintain. On the mid-term I’d rather see this script rewritten in Python/Perl/Ruby.

rewrote unify_po in Python - I can simply rewrite the check_po also in Python, as I needed already the checking anyways for unify script.

> * Multiple typos in “>nother option would be to call python/perl to get a reex”.

@u: look for typos / improve text in python script and afterwards assign to intrigeri, if you are happy.

#27 Updated by Anonymous 2018-09-03 17:54:56

• Target version changed from Tails_3.9 to Tails_3.10.1

Won’t be able to review this in the next 48 hours (release of 3.9)

#28 Updated by intrigeri 2018-10-24 17:03:34

• Target version changed from Tails_3.10.1 to Tails_3.11

#30 Updated by Anonymous 2018-11-02 17:29:37

• Assignee set to hefee
• QA Check changed from Ready for QA to Dev Needed

#31 Updated by hefee 2018-11-05 16:22:03

• related to Feature #16102: List of potential checks we might want to do on PO files added

#32 Updated by hefee 2018-11-05 16:30:46

• Assignee deleted (hefee)
• QA Check changed from Dev Needed to Ready for QA
• Type of work changed from Sysadmin to Code

@u: Can you do another string check in wiki/src/contribute/l10n_tricks/unify_po-headers.py?
I added the check_po into same file, as we share a lot of code anyways.
So it make sense to keep this together in one file.

We also to find a solution:

• where to place the script(jenkins-tools or tails).
• how the script should be named/ if we want make it accessible with other names.

if this is decided:

• a QA in terms of program logic.

#33 Updated by Anonymous 2018-11-09 13:16:42

• Assignee set to hefee
• QA Check changed from Ready for QA to Pass

hefee wrote:
> @u: Can you do another string check in wiki/src/contribute/l10n_tricks/unify_po-headers.py?
> I added the check_po into same file, as we share a lot of code anyways.
> So it make sense to keep this together in one file.

Cool!

I checked and did some small improvements in tails.git:translation_platform_hooks.

> We also to find a solution:
> * where to place the script(jenkins-tools or tails).

I suggest to ask intrigeri for advice on this.

> * how the script should be named/ if we want make it accessible with other names.

Idem.

> if this is decided:
> * a QA in terms of program logic.

groente should officially be the reviewer of this, but intrigeri might also want to take a look.

#34 Updated by hefee 2018-11-12 11:14:48

• Assignee changed from hefee to groente
• QA Check changed from Pass to Info Needed

(the HEAD is at https://0xacab.org/Hefee/tails/tree/translation_platform_hooks)

infomation I need are:

• where to place the script(jenkins-tools or tails).
• is the script good enough to replace check_po.sh?
• where is check_po is used and what workflows I have to test?

Feel free to assign it to others, I you don’t know the anwsers.

#35 Updated by groente 2018-11-12 15:46:50

• Assignee changed from groente to hefee
• QA Check changed from Info Needed to Dev Needed

hey hefee,

if you want to replace the existing check_po, that should be done in jenkins-tools, which is where the isobuilders/testers get it from. see puppet-tails:manifests/tester/check_po.pp

as for replacing, it doesn’t look suitable as drop-in replacement: it takes different arguments and gives different output compared to check_po. did you plan on making some wrapper for backward compatibility?

in terms of code quality, the regexKey makes my head hurt, perhaps you’re better off with a review by someone who’s more fluent in python.

as for where check_po is actually used, i have no idea…

#43 Updated by hefee 2018-11-16 15:00:51

• Assignee deleted (hefee)
• QA Check changed from Dev Needed to Info Needed

groente wrote:
> if you want to replace the existing check_po, that should be done in jenkins-tools, which is where the isobuilders/testers get it from. see puppet-tails:manifests/tester/check_po.pp
>
> as for replacing, it doesn’t look suitable as drop-in replacement: it takes different arguments and gives different output compared to check_po. did you plan on making some wrapper for backward compatibility?
>
> in terms of code quality, the regexKey makes my head hurt, perhaps you’re better off with a review by someone who’s more fluent in python.
>
> as for where check_po is actually used, i have no idea…

Current issues:

• I’m missing information about what is the “backward compatibility”, that intrigeri and groente is talking about. Is the behaviour run i18nspector for all *.po files, when no argument given to the new script enough to forfill this requirment?
• Who can do the review? as groente feels himself not suitable to do the review…

@u can you help solving these issues?

#45 Updated by CyrilBrulebois 2018-12-16 13:58:30

• Target version changed from Tails_3.11 to Tails_3.12

#46 Updated by intrigeri 2019-01-08 16:17:25

• blocks Feature #15082: Have the Weblate Git communicate with our main Git repository added

#47 Updated by intrigeri 2019-01-08 17:05:07

• blocked by deleted (Feature #15082: Have the Weblate Git communicate with our main Git repository)

#48 Updated by Anonymous 2019-01-21 00:16:26

• Assignee set to hefee
• QA Check deleted (Info Needed)

@hefee: I think lamby will be able to do this review (still waiting for his confirmation though).

Concerning backward compatibility, I guess you’ve had a chance to talk with intrigeri?

#50 Updated by anonym 2019-01-30 11:59:12

• Target version changed from Tails_3.12 to Tails_3.13

#52 Updated by hefee 2019-02-17 15:14:32

• Description updated
• Assignee changed from hefee to enrico
• QA Check set to Ready for QA

I described the environment in where we want to run the sccript so a review should now be possible. Please ask if anything is not clear.

#53 Updated by intrigeri 2019-03-12 17:03:00

• Description updated

#54 Updated by intrigeri 2019-03-12 17:16:45

• related to Bug #15819: Make sure ikiwiki generated PO files satisfy the requirements we want check_po to set added

#55 Updated by CyrilBrulebois 2019-03-20 14:34:05

• Target version changed from Tails_3.13 to Tails_3.14

#56 Updated by enrico 2019-03-21 16:39:41

• File flake8.diff added
• Assignee changed from enrico to hefee

Hello, here is my review:

I suggest a round of flake8:

flake8 unify_po-headers.py

Besides cosmetic issues, you get things like:

unify_po-headers.py:68:11: W605 invalid escape sequence ‘\s’

which hint at a missing ‘r’ on regexp strings.

Since some of the pep8 habits make it easier for me to read the code, I’ve had
a go at fixing them. I’m attaching a patch.

I suggest to wrap the main code in a function, and just call the function at
module-level, like:

def main():
    ...

if __name__ == '__main__':
    main()

This way, variables used by that code don’t leak to module-level variables.

For example, this code prints 6 instead of refusing to compile because val
is mistyped inside foo:

def foo(value):
    return val + 1


if __name__ == '__main__':
    val = 5
    print(foo(val * 2))

        toplevel = subprocess.check_output(["git", "rev-parse", "--show-toplevel"])
        toplevel = toplevel.decode()[:-1]  # get rid of tailing \n

I’d use universal_newlines instead of decode, and rstrip() for cleanup:

        toplevel = subprocess.check_output(["git", "rev-parse", "--show-toplevel"], universal_newlines=True).rstrip()

Similarly for the following subprocess invocation.

    if not args.files:
        print("WARNING: no file to process :("
              " You may want to add files to operate on. See --help for further information.")

You probably want sys.exit or parser.error instead of print, so it goes to
stderr and the program terminates with an error status (and with
parser.error, you also get command line help).

    e = None

This doesn’t seem to be used, I guess it can be deleted.

                    print("Issues with {fname}:\n\t{issues}".format(fname=fname, issues="\n\t".join(issues)))

It looks like an error message, which makes me think it could go to stderr, but
I don’t know what are the required expectations on the output of the script.

    except FileNotFoundError as err:
        if err.filename in ("i18nspector", "msgcat"):
            sys.exit("{fname}: command not found\nYou need to install {fname} first. See /contribute/l10n_tricks."
                     .format(fname=err.filename))
        else:
            raise

I’d rather use shutil.which before starting processing, and error out if it
returns None. That way the check happens in a predictable place, and before
potential side effects the main code of the script can have.

Then that whole try/catch can be removed.

    def regexKey(self, key: str) -> Pattern:
        """returns a regex to match a key: value pair in po header"""
        return re.compile(r'^\s*"{key}\s*:\s*(?P<value>(.*\n)*?.*)\\\\n"\s*?$'.format(key=key), flags=re.M)

Please use key=re.escape(key). After adding ‘r’ to the regexp string, I
think there’s now one backslash too much when matching the newline.

I find it really hard to review that regular expression, which worries me from
a security standpoint. Instead of a hand-crafted parser, I would suggest using
an existing library to parse the files.

Polib seems good enough: https://polib.readthedocs.io/en/latest/api.html#polib.POFile
and there is also babel: http://babel.pocoo.org/en/latest/api/messages/pofile.html
both seem solid, widely used, and packaged in Debian since various releases.
Also, i18nspector already depends on python3-polib :)

For example, polib would give you all the metadata as a convenient dict:

from polib import pofile
pf = pofile("file.po")
print(repr(pf.metadata))

The rest of the code looks like it could be greatly simplified by something
like that, and I’d rather stop at suggesting polib than continue double
checking the use of the regexps.

class PoFile:
    ...

    def __init__(self, fname: str) -> None:
        self.fname = fname

    def __enter__(self) -> 'PoFile':
        """magic method for with statement. @returns self so it can be used with "as" """
        self.open()
        return self

    def __exit__(self, type, value, traceback) -> None:
        """magic method for with statement"""
        if type is None and value is None and traceback is None:
            self.write()

I don’t like that __exit__ always calls write, even if write only acts when self.__changed is True. I would prefer something like this:

@contextlib.contextmanager
def pofile_readonly(fname):
   pf = PoFile(fname)
   pf.open()
   yield pf

@contextlib.contextmanager
def pofile_writable(fname):
   pf = PoFile(fname)
   pf.open()
   try:
      yield pf
   else:
      pf.write()

so that if one wants to open a file read only, there is no chance that write
gets called.

Also, contextmanager makes it very straightforward to have context managers,
allowing to remove __enter__ / __exit__ methods from PoFile.

    def lang(self) -> str:
        """@returns: language of filename"""
        exts = self.fname.split(".")
        if len(exts) < 3:
            raise Exception("po file should have a language in his name.", self.fname)
        return exts[-2]

This would fail if any of the parent directories of the potfile contained a dot
in its name.

I suggest exts = os.path.basename(self.fname).split("."), and maybe a regexp
to be run on the basename instead of a split, to match exactly *.[lang].po.

I’ve seen potfiles named just with the language code, like de.po: if they are
supposed to be supported, that function would raise an exception on them.

    def write(self) -> None:
        """write file, if content was changed"""
        if self.__changed:
            with open(self.fname, 'w') as f:
                f.write(self.content)

Given that it’s writing over the source file, I’d be rather a fan of atomic
writes, like this:

def write(filename, content):
    with tempfile.NamedTemporaryFile(prefix=filename, delete=False) as fd:
        try:
            fd.write(content)
            fd.flush()
            os.fdatasync(fd.fileno())
        except Exception:
            os.unlink(fd.name)
            raise
        else:
            os.rename(fd.name, filename)

so that if anything goes wrong during the write, at least we don’t leave the
user with a truncated source file.

    def msgcat(self, modify=False) -> bool:
        ...

If this is only needed for word-wrapping, polib does it automatically.

    def i18nspector(self) -> List[str]:
        """@returns a list of issues raised by i18nspector removes allowed issues from @I18NINSPECTOR_ALLOWED_ISSUES.
        """

s/I18NINSPECTOR_ALLOWED_ISSUES/I18NSPECTOR_ACCEPT/ ?

This is all I can think of right now. I think a custom parser for .po files is a really hard thing to make secure. I’m happy to do another round of review if you manage to port it to polib, and I expect the code will be much easier to
check :)

#57 Updated by intrigeri 2019-03-22 16:58:01

• QA Check changed from Ready for QA to Dev Needed

#58 Updated by hefee 2019-04-10 15:21:48

• Assignee changed from hefee to enrico
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from https://0xacab.org/Hefee/tails/tree/translation_platform_hooks to https://salsa.debian.org/tails-team/tails/merge_requests/14

Thanks for your review. I included all things you suggested. And I added some tests to test the base functionality.
Meanwhile the Foundation Team started to use salsa to do reviews:

https://salsa.debian.org/tails-team/tails/merge_requests/14

I think it makes it a lot easier for you to comment directly on a line base. If it does not work for you you can do a review like before.

intrigeri, u: the check for wrap the file to 79 chars is not stable. Different tools have a different idea what it means to wrap to 79 chars ;D IMO remove this check completely.

#60 Updated by enrico 2019-04-22 10:52:04

• Assignee changed from enrico to hefee

Cool! I made a few annotations there, mostly nitpicks.

#61 Updated by hefee 2019-05-10 21:52:30

• Assignee changed from hefee to enrico

Okay I think this is the final round of review.

#62 Updated by CyrilBrulebois 2019-05-23 21:23:20

• Target version changed from Tails_3.14 to Tails_3.15

#63 Updated by intrigeri 2019-06-02 14:42:53

• Status changed from In Progress to Needs Validation

#64 Updated by enrico 2019-06-03 09:40:46

• Assignee changed from enrico to hefee

I’m having some difficulty following the flow of progress between redmine and gitlab. Also, it looks like I made comments on the merge request but forgot to notify them here, sorry about that.

Today I am having problem manipulating notes on salsa: I get 404 on the AJAX posts my browser does when adding or updating a note/discussion. I’ll revert to commenting here.

The discussion that ends with me saying “Ah, shame, fair enoguh” can be resolved.

I’m giving a quick look at all the files involved.

In PoFile.msgcat():
- f.flush() does not seem needed, since the file is opened read-only. Should it be seek(0), to rewind the file before passing it to subprocess?
- instead of passing stdin=f to subprocess, pass input=content, or pass the file name to msgcat’s command line if msgcat would use it to print nicer error messages
- PoFile.msgcat() does not change self.pf: looking at unify_po_headers, it seems that it is called to do one last round of postprocessing of the po file with msgcat, but the msgcat output does not seem to be stored
- if my comments above are valid, I would try to check for msgcat effects in unit tests

The rest looks good to me.

#65 Updated by hefee 2019-06-04 13:51:44

• Assignee changed from hefee to enrico

enrico wrote:
> I’m having some difficulty following the flow of progress between redmine and gitlab. Also, it looks like I made comments on the merge request but forgot to notify them here, sorry about that.

that is totally fine. As we want the review process to be moved to Gitlab. And this should not end up in copying all comments between Redmine and Gitlab.

> Today I am having problem manipulating notes on salsa: I get 404 on the AJAX posts my browser does when adding or updating a note/discussion. I’ll revert to commenting here.
>
> The discussion that ends with me saying “Ah, shame, fair enoguh” can be resolved.

marked as resolved.

> I’m giving a quick look at all the files involved.
>
> In PoFile.msgcat():
> - f.flush() does not seem needed, since the file is opened read-only. Should it be seek(0), to rewind the file before passing it to subprocess?
> - instead of passing stdin=f to subprocess, pass input=content, or pass the file name to msgcat’s command line if msgcat would use it to print nicer error messages

=> Good ideas, but I’ve rewrote the msgcat method to see the new needs_rewrap method to use polib only for the rewrap test.

> - PoFile.msgcat() does not change self.pf: looking at unify_po_headers, it seems that it is called to do one last round of postprocessing of the po file with msgcat, but the msgcat output does not seem to be stored

It is not obvious indeed, but the needs_rewrap method updates the changed flag, if file is not wrapped properly and this triggers PoFile.save(), when existing the contextmanager.

> - if my comments above are valid, I would try to check for msgcat effects in unit tests

I added new tests for the needs_rewrap method directly. But we also had the testfile length in checkPo/unifyPo folder, this file is the testcase, for only rewrap is needed.

> The rest looks good to me.

cool ;D

#66 Updated by intrigeri 2019-06-25 13:07:20

• Status changed from Needs Validation to In Progress

Applied in changeset commit:tails|52afa659e1a7815c2b37e2103bb627f9b9735457.

#67 Updated by intrigeri 2019-06-25 13:14:09

• Status changed from In Progress to Resolved

#68 Updated by intrigeri 2019-06-27 17:16:56

• Assignee deleted (enrico)