Project

General

Profile

Feature #11967

Feature #5630: Reproducible builds

refresh-translations: don't update PO files unless something other POT-Creation-Date has changed

Added by intrigeri about 3 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Build system
Target version:
-
Start date:
11/19/2016
Due date:
% Done:

100%

Feature Branch:
451f:tails/feature/11967+refresh_translations
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Associated revisions

Revision ad6f1b15 (diff)
Added by intrigeri about 3 years ago

Slightly rewrite intltool_report (refs: #11967).

Revision 3e60303b
Added by intrigeri about 3 years ago

Merge branch 'feature/11967+refresh_translations' (Closes: #11967)

History

#1 Updated by u about 3 years ago

  • Assignee changed from u to intrigeri
  • % Done changed from 0 to 10
  • QA Check set to Ready for QA
  • Feature Branch set to 451f:tails/feature/11967+refresh_translations

Please review for corner cases and escapes that I might have overlooked.

#2 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to u
  • % Done changed from 10 to 60
  • QA Check changed from Ready for QA to Dev Needed

Please review for corner cases and escapes that I might have overlooked.

Great! It looks like it does the job. I mostly have comments about the shell programming.

In:

if [ $(diff "${locale}.po" "${locale}.po.new" | grep -c ^@ | wc -l) -eq "1" ]; then

I think that the output of wc will always be "1". Besides, wc is not needed since you're doing grep -c. And let's avoid quoting "1" while we're doing number comparison (-eq). All in all something like this (untested!) should be enough:

if [ $(diff "${locale}.po" "${locale}.po.new" | grep -c '^@') -eq 1 ]; then

It seems to me that even if ${locale}.po.new does not exist, the code diffs it with {locale}.po.
Using [ -f ${locale}.po.new ] || continue earlier to avoid doing that should save some time.

grep 'POT-Creation-Date' is not enough to ensure "Only header changes in potfile". E.g. we might use the 'POT-Creation-Date' string elsewhere, e.g. in the contribute/ directory. We'll need a stricter regexp that matches all the lines we want, and nothing else.

In fi; you don't need ; since a newline follows.

The last rm -f ${locale}.po.new in compare_po_headers() seems unnecessary.

compare_po_headers could have a name that expresses better what it does: its current name suggests it just "compares" stuff, while it renames files and stuff. But actually, forget it: the added code should simply go into intltool_update_po -- it basically makes PO files update more clever.

#3 Updated by u about 3 years ago

intrigeri wrote:

Great! It looks like it does the job. I mostly have comments about the shell programming.

fair enough.

I think that the output of wc will always be "1". Besides, wc is not needed since you're doing grep -c. And let's avoid quoting "1" while we're doing number comparison (-eq). All in all something like this (untested!) should be enough:

[...]

Comparing two files in which only the POT-Creation-Date has changed:

The diff options are not correct indeed, I will try to find something better because your untested version does not work :)

#4 Updated by u about 3 years ago

  • QA Check changed from Dev Needed to Ready for QA

Updated it again.

#5 Updated by intrigeri about 3 years ago

  • QA Check changed from Ready for QA to Dev Needed

Updated it again.

"New PO file for ${locale} does not exist. Skipping." does not exactly match what the test does.

Let's use grep -c instead of grep | wc -l.

I'm don't think that counting occurrences of ^"> " is enough: what if there are two chunks in the diff, one that is about POT-Creation-Date, and the other that only removes lines and adds none. Then it seems that the current code will believe "Only header changes in potfile", which is untrue. We should make sure that exactly 1 line is removed, exactly 1 line is added, and both those lines match the regexp ^'(?:>|<) "POT-Creation-Date:' (that might need grep -E).

s/Delete/delete/

-f is not needed in rm -f ${locale}.po.new since we already checked that the file exists.

All right!

#6 Updated by u about 3 years ago

  • QA Check changed from Dev Needed to Ready for QA

(Sidenote: tails.pot is changed during the operation.)

#7 Updated by intrigeri about 3 years ago

  • QA Check changed from Ready for QA to Dev Needed

Almost there!

On top of what its message says, a41f865fcaad90a658c72df6c91af038f64e1231 seems to add useless parenthesis to a couple regexps. Maybe remove them?

The nested if logic is wrong: the "else" clause won't be hit in some situations where it should. I suggest using "&&" to express the condition, instead of nesting if's.

We're now checking that "exactly 1 line is removed, exactly 1 line is added, and at least one line matches the regexp ^'(?:>|<) "POT-Creation-Date:'", while we should check that "exactly 1 line is removed, exactly 1 line is added, and both those lines match the regexp ^'(?:>|<) "POT-Creation-Date:'".

Also, let's add the PO files report to language_statistics.sh before we merge this branch.

#8 Updated by u about 3 years ago

  • Assignee changed from u to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

#9 Updated by intrigeri about 3 years ago

  • Assignee changed from intrigeri to u
  • QA Check changed from Ready for QA to Dev Needed

In if [ ! -f ${locale}.po.new -a ! -f ${locale}.po ]; then, I think that we want -o instead of -a: if any of these two files does not exist, then there's not much useful work the rest of this function can do.

"New PO file for ${locale} does not exist" matches neither what the code currently does, nor what it should IMO do (see above).

How about:

[ -f ${locale}.po ]     || continue
[ -f ${locale}.po.new ] || continue

?

I don't understand why the question mark is there in ^'?>', ^'?<'. But perhaps it comes from when you had parenthesis in there, and "?" was meant to be "?:", which would have been a good idea there.

Similarly, instead of ^'(?>|<), I guess you meant ^'(?:>|<). Which would be simpler like this, BTW: '^[<>].

To end with, I prefer the "^" to be inside the single quotes: in some shells, it is a special char that's interpreted, so it makes it easier to test the code if it's protected inside single quotes

#10 Updated by u about 3 years ago

  • Assignee changed from u to intrigeri
  • QA Check changed from Dev Needed to Ready for QA

Similarly, instead of ^'(?>|<), I guess you meant ^'(?:>|<). Which would be simpler like this, BTW: '^[<>].

Just as a side note: this does not give the same result:


$ diff de.po de.po.new | grep -Ec '^(?:>|<) "POT-Creation-Date:'
1
$ diff de.po de.po.new | grep -Ec '^[<>] "POT-Creation-Date:'
2

Using the second version now.

#11 Updated by u about 3 years ago

intrigeri wrote:

In if [ ! -f ${locale}.po.new -a ! -f ${locale}.po ]; then, I think that we want -o instead of -a: if any of these two files does not exist, then there's not much useful work the rest of this function can do.

right, that's not correct.

I don't understand why the question mark is there in ^'?>', ^'?<'. But perhaps it comes from when you had parenthesis in there, and "?" was meant to be "?:", which would have been a good idea there.

There was a misunderstanding of what "?" does indeed.

But I don't understand what "?:" should do. When I try this, it does not do what i expect, instead I get 0 as a result. So I'm skipping this and use ^> resp. ^>
Feel free to adapt this to whatever you think is correct.

To end with, I prefer the "^" to be inside the single quotes: in some shells, it is a special char that's interpreted, so it makes it easier to test the code if it's protected inside single quotes

ack.

#12 Updated by intrigeri about 3 years ago

Just as a side note: this does not give the same result:


> $ diff de.po de.po.new | grep -Ec '^(?:>|<) "POT-Creation-Date:'
> 1
> $ diff de.po de.po.new | grep -Ec '^[<>] "POT-Creation-Date:'
> 2

> 

OK, I guess that ERE don't work like PCRE.

Using the second version now.

Good.

#13 Updated by intrigeri about 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 60 to 100

#14 Updated by intrigeri over 2 years ago

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

Also available in: Atom PDF