693581 - Fix LSV scripts to work with rapid release process

Status: RESOLVED FIXED

(Mozilla Localizations :: Infrastructure, defect)

Description

[Milos Dinic [:Milos]]

Attachment: Fixing LSV helers scripts

Axel,

I didn't want to hg rename 2.0 in references, so as not to make this patch  enormously big. Once I get a r+, I'll do that, and ask you for another review.

Comment 1

[Axel Hecht [:Pike]]

Comment on attachment 566181 [details] [diff] [review]
Fixing LSV helers scripts

The change to compare.py isn't to my liking, as now it doesn't work for 1.9.2 anymore, and still doesn't work for central. Which, fwiw, is why I didn't really like stas' directory structure, and prefer a dir with the full upstream paths instead. That is releases/l10n/mozilla-aurora/* instead of l10n-mozilla-aurora.

update-clones.py doesn't work for 1.9.2 anymore either?

Also, I'd really like to see the reference changes in a patch, too. A bit noisy, but not really big.

Comment 2

[Milos Dinic [:Milos]]

Attachment: Take 2

Comment 3

[Staś Małolepszy :stas]

Comment on attachment 567132 [details] [diff] [review]
Take 2

Review of attachment 567132 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work, Milos.  I'm gonna r- this for now as I'd like to see the fixes to my comments before I try this out.  Also, we should figure out between you, Axel and me what to do with central.

I'd like to see the planned changes to reference/, too.  In fact, the easiest way to test this patch might be for you to clone your own fork and push this patch there.

::: README
@@ +1,4 @@
>  Set up the clones
>  =================
>  
> +mkdir ~/src

Do you need to cd into src after it's created?

@@ +7,2 @@
>  
> +./configure  /* In next step we're running configure script that'll clone all the repositories. We're downloading more than 1GB of data, so it'll take some time

The file is called configure.sh.

::: configure.sh
@@ +1,1 @@
> +#!/bin/bash

The permissions on the file are 644.  I think you should make it executable, i.e. 755.

::: helpers/diff-all.sh
@@ +16,3 @@
>  ./helpers/diff.sh browser 1.9.2 > $OUTPUT/${TIME}_browser-1.9.2.html
>  ./helpers/diff.sh browser central > $OUTPUT/${TIME}_browser-central.html
>  ./helpers/diff.sh mobile central > $OUTPUT/${TIME}_mobile-central.html

Should we drop comparing central all together in this script?  I'd still support it, but turn it off by default.

::: helpers/diff.sh
@@ +8,5 @@
>  fi
>  
> +# Arguments:
> +# -a        app
> +# -b        branch

This suggests that -a and -b are options passed to the diff.sh script, while in fact the script only take positional arguments (like in diff-all.sh).

@@ +13,5 @@
> +
> +if [ "$2" = "1.9.2" ]; then
> +    python compare.py -a ${1} -b 1.9.2 reference/${1}/1.9.2
> +elif [ "$2" = "central" ]; then
> +    python compare.py -a ${1} -b ${2} reference/${1}/aurora

reference/${1}/aurora is probably wrong here?  Should it be central?

::: tools/update-clones.py
@@ +16,5 @@
> +if len(args) != 1:
> +    p.error('L10n base is required (the directory where all locales\' repos reside.)')
> +l10nbase = args[0]
> +
> +if opts.branch == "trunk":

This comes from the old days, I'd call this 'central' now, not 'trunk.'

Comment 4

[Milos Dinic [:Milos]]

(In reply to Staś Małolepszy :stas from comment #3)
> Comment on attachment 567132 [details] [diff] [review] [diff] [details] [review]
> Take 2
> 
> Review of attachment 567132 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Nice work, Milos.  I'm gonna r- this for now as I'd like to see the fixes to
> my comments before I try this out.  Also, we should figure out between you,
> Axel and me what to do with central.
> 
> I'd like to see the planned changes to reference/, too.  In fact, the
> easiest way to test this patch might be for you to clone your own fork and
> push this patch there.
> 
> ::: README
> @@ +1,4 @@
> >  Set up the clones
> >  =================
> >  
> > +mkdir ~/src
> 
> Do you need to cd into src after it's created?

True. Added.

> 
> @@ +7,2 @@
> >  
> > +./configure  /* In next step we're running configure script that'll clone all the repositories. We're downloading more than 1GB of data, so it'll take some time
> 
> The file is called configure.sh.

My bad. Fixed README.

> 
> ::: configure.sh
> @@ +1,1 @@
> > +#!/bin/bash
> 
> The permissions on the file are 644.  I think you should make it executable,
> i.e. 755.
>

Changed permissions to that file to 755
 
> ::: helpers/diff-all.sh
> @@ +16,3 @@
> >  ./helpers/diff.sh browser 1.9.2 > $OUTPUT/${TIME}_browser-1.9.2.html
> >  ./helpers/diff.sh browser central > $OUTPUT/${TIME}_browser-central.html
> >  ./helpers/diff.sh mobile central > $OUTPUT/${TIME}_mobile-central.html
> 
> Should we drop comparing central all together in this script?  I'd still
> support it, but turn it off by default.
> 

It's only us who use this, so I wouldn't hide it. But we can talk about this.

> ::: helpers/diff.sh
> @@ +8,5 @@
> >  fi
> >  
> > +# Arguments:
> > +# -a        app
> > +# -b        branch
> 
> This suggests that -a and -b are options passed to the diff.sh script, while
> in fact the script only take positional arguments (like in diff-all.sh).

Indeed. I fixed that too.

> 
> @@ +13,5 @@
> > +
> > +if [ "$2" = "1.9.2" ]; then
> > +    python compare.py -a ${1} -b 1.9.2 reference/${1}/1.9.2
> > +elif [ "$2" = "central" ]; then
> > +    python compare.py -a ${1} -b ${2} reference/${1}/aurora
> 
> reference/${1}/aurora is probably wrong here?  Should it be central?

As per IRC, we're comparing everything(including central) to aurora, or what used to be 2.0 branch, except for 1.9.2, which we still compare to its own branch.

> 
> ::: tools/update-clones.py
> @@ +16,5 @@
> > +if len(args) != 1:
> > +    p.error('L10n base is required (the directory where all locales\' repos reside.)')
> > +l10nbase = args[0]
> > +
> > +if opts.branch == "trunk":
> 
> This comes from the old days, I'd call this 'central' now, not 'trunk.'

Fixed compare.py.

As per your suggestion, I forked LSV. It's now at 
https://hg.mozilla.org/users/mdinic_mozilla.com/lsv

Changes from your review in: 
https://hg.mozilla.org/users/mdinic_mozilla.com/lsv/rev/3e1cdaed704d

Thanks!

Comment 5

[Axel Hecht [:Pike]]

Comment on attachment 567132 [details] [diff] [review]
Take 2

Canceling the review request, waiting for the next iteration.

Comment 6

[Milos Dinic [:Milos]]

Changes committed in http://hg.mozilla.org/l10n/l10n-src-verification/rev/58efc91560a1 and http://hg.mozilla.org/l10n/l10n-src-verification/rev/4a5d0bb88426