661679 - release l10n scripts for mobile

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[Aki Sasaki (not active)]

We have release l10n scripts for desktop and nightly l10n scripts for mobile.
We need release l10n scripts for mobile.

This will fix bug 535301.

Comment 1

[Aki Sasaki (not active)]

Attachment: wip py script

I pretty much merged create-release-repacks.py and nightly-mobile-repacks.py.

This has a *lot* of duplicate code (the original 2 scripts have a lot of duplicate code; this adds a 3rd).  In the interests of time and reduced risk I don't want to touch the Firefox desktop create-release-repacks.py for now.  Longer term we can probably create a single script that does all of the above, with different configs.

Untested, and may require additional helper function work to read the l10n-changesets.json format.  I'm planning on testing by creating a hardcoded-config release_mobile_repacks.sh and trying against a staging-stage set of candidate builds, before adding the real release_mobile_repacks.sh.

Comment 2

[Aki Sasaki (not active)]

Attachment: wip tools changes

Currently I'm getting this far:

Downloading http://staging-stage.build.mozilla.org/pub/mozilla.org/fennec/candidates/5.0b1-candidates/build1/linux/en-US/fennec-5.0b1.en-US.linux-i686.tar.bz2 to fennec.tar.bz2

This is wrong; it should be http://staging-stage.build.mozilla.org/pub/mozilla.org/mobile/candidates/5.0b1-candidates/build1/linux-i686/fennec-5.0.en-US.linux-i686.tar.bz2

So: I need to shoehorn in appName (for mobile) as well as productName (already there, for fennec-).  I also need to shoehorn in appVersion as well as version (version for 5.0b1-candidates; appVersion for fennec-5.0).

Depending on how we upload our desktop builds, we may be able to keep the en-US/ subdirectory.

Comment 3

[Aki Sasaki (not active)]

Attachment: mobile l10n mozconfigs for mozilla-beta

Comment 4

[Aki Sasaki (not active)]

Attachment: test repack bash script

This is what I'm using to repack from the commandline, to bypass the command line args + json.

Comment 5

[Aki Sasaki (not active)]

Comment on attachment 538200 [details] [diff] [review]
mobile l10n mozconfigs for mozilla-beta

http://hg.mozilla.org/build/buildbot-configs/rev/e644db4854d2

Comment 6

[Aki Sasaki (not active)]

Attachment: wip: this works on linux [!]

http://staging-stage.build.mozilla.org/pub/mozilla.org/mobile/candidates/5.0b4-candidates/build1/  (just did chunk 1/5)

Needs some cleanup, testing on win32/macosx.  Then tie-in to ScriptFactory+scheduler.

Comment 7

[Aki Sasaki (not active)]

Attachment: log for test-run of linux repack 1/5

Comment 8

[Aki Sasaki (not active)]

Looks like I'm missing xpis and need to change the upload dir to include platform + locale.

Comment 9

[Aki Sasaki (not active)]

Er, no, xpis are in install/ for some reason.

Comment 10

[[e-mail] (:bhearsum)]

Comment on attachment 538444 [details] [diff] [review]
wip: this works on linux [!]

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

I don't think anything glaring wrong here. Other than the one about wget-en-US, I'd like to see the specific comments below addressed. Looks like this is making great progress!

::: lib/python/release/download.py
@@ +28,5 @@
>  
>  def downloadReleaseBuilds(stageServer, productName, brandName, version,
> +                          buildNumber, platform, appVersion=None,
> +                          candidatesDir=None):
> +    print "appVersion %s candidatesDir %s" % (appVersion, candidatesDir)

If you're planning to keep this in, please switch it to using Python's logging.

::: lib/python/release/paths.py
@@ +1,4 @@
>  from urlparse import urlunsplit
>  
>  def makeCandidatesDir(product, version, buildNumber, nightlyDir='nightly',
> +                      protocol=None, server=None, appName=None):

Can we replace both product and appName with ftpProductName or something similar? To me, appName is the directory the code for a product lives in, and I'd prefer not to overload it here.

::: scripts/l10n/nightly-mobile-repacks.py
@@ +63,5 @@
>                     l10nRepoDir, makeDirs, localeSrcDir, env)
> +    fullCandidatesDir = makeCandidatesDir(ftpProduct, version, buildNumber,
> +                                      protocol='http', server=stageServer,
> +                                      appName=appName, nightlyDir=nightlyDir)
> +    input_env = retry(downloadReleaseBuilds,

I know it's probably too late to do it now, but there's probably no reason you can't use "make wget-en-US". Downloading by hand is purely at a legacy thing at this point. No need to block on this though, of course.

@@ +186,5 @@
> +    except KeyError:
> +        l10nRepoDir = path.split(releaseConfig["l10nRepoPath"])[-1]
> +
> +    ftpProduct = "mobile"
> +    brand = "fennec"

These should be pulled from the releaseConfig rather than hardcoded.

::: scripts/l10n/release_repacks.sh
@@ +14,5 @@
>  JSONTOOL="$PYTHON $SCRIPTS_DIR/buildfarm/utils/jsontool.py"
>  workdir=`pwd`
>  
>  platform=$1
> +stage_platform=$2

Don't pass stage_platform to this script. You can pull it out of the branchConfig in release-mobile-repacks.py.

@@ +22,2 @@
>  
> +branch=$($basename $($JSONTOOL -k properties.branch $PROPERTIES_FILE))

Typo here, $basename vs basename

@@ +36,5 @@
>  fi
>  
>  cd $SCRIPTS_DIR/../..
>  $PYTHON $SCRIPTS_DIR/clobberer/clobberer.py -s scripts -s buildprops.json \
> +  $CLOBBERER_URL $branch "$builder" $slavebuilddir $slavename $master

Out of curiosity, why do we need quotes around the builder name?

Comment 11

[Aki Sasaki (not active)]

(In reply to comment #10)
> ::: lib/python/release/download.py
> @@ +28,5 @@
> >  
> >  def downloadReleaseBuilds(stageServer, productName, brandName, version,
> > +                          buildNumber, platform, appVersion=None,
> > +                          candidatesDir=None):
> > +    print "appVersion %s candidatesDir %s" % (appVersion, candidatesDir)
> 
> If you're planning to keep this in, please switch it to using Python's
> logging.

This was for debugging, removed.

> ::: lib/python/release/paths.py
> @@ +1,4 @@
> >  from urlparse import urlunsplit
> >  
> >  def makeCandidatesDir(product, version, buildNumber, nightlyDir='nightly',
> > +                      protocol=None, server=None, appName=None):
> 
> Can we replace both product and appName with ftpProductName or something
> similar? To me, appName is the directory the code for a product lives in,
> and I'd prefer not to overload it here.

Yes and no.
No because product is 'fennec', appName is 'mobile', and it's /pub/mozilla.org/mobile/ not /pub/mozilla.org/fennec/.

Yes because we're not actually using 'fennec' in makeCandidatesDir at all, so I replaced ftpProduct with appName when *calling* makeCandidatesDir.

So appName=None is removed from makeCandidatesDir now.

> ::: scripts/l10n/nightly-mobile-repacks.py
> @@ +63,5 @@
> >                     l10nRepoDir, makeDirs, localeSrcDir, env)
> > +    fullCandidatesDir = makeCandidatesDir(ftpProduct, version, buildNumber,
> > +                                      protocol='http', server=stageServer,
> > +                                      appName=appName, nightlyDir=nightlyDir)
> > +    input_env = retry(downloadReleaseBuilds,
> 
> I know it's probably too late to do it now, but there's probably no reason
> you can't use "make wget-en-US". Downloading by hand is purely at a legacy
> thing at this point. No need to block on this though, of course.

For version 2.

> @@ +186,5 @@
> > +    except KeyError:
> > +        l10nRepoDir = path.split(releaseConfig["l10nRepoPath"])[-1]
> > +
> > +    ftpProduct = "mobile"
> > +    brand = "fennec"
> 
> These should be pulled from the releaseConfig rather than hardcoded.

Done.

> ::: scripts/l10n/release_repacks.sh
> @@ +14,5 @@
> >  JSONTOOL="$PYTHON $SCRIPTS_DIR/buildfarm/utils/jsontool.py"
> >  workdir=`pwd`
> >  
> >  platform=$1
> > +stage_platform=$2
> 
> Don't pass stage_platform to this script. You can pull it out of the
> branchConfig in release-mobile-repacks.py.

Sadly, no.
We've got 3 platform variables in mobile land:

complete_platform: linux-mobile, linux-android, linux-maemo5-gtk, macosx-mobile, win32-mobile, etc. # jhford wanted to avoid special casing android and maemo everywhere there was an |if platform.startswith('linux'):|
platform: linux, macosx, win32  # catlee drops everything after the first -
stage_platform: linux, android, maemo5-gtk, macosx, win32 # outwardly facing names

I think this is a problem, but don't have an immediate fix.
We can pass in complete_platform and then go through the same programmatic logic to figure out the other two, or we can take the programmatic logic that has already happened and pass it in.

Passing it in will be less error prone.

Moving to non-programmatic configs for everything, and not overloading variables like 'platform' for other configuration options like 'mobile' or 'debug', is the right solution here, but is massively out of scope for this bug.

> @@ +22,2 @@
> >  
> > +branch=$($basename $($JSONTOOL -k properties.branch $PROPERTIES_FILE))
> 
> Typo here, $basename vs basename

Fixed.
 
> @@ +36,5 @@
> >  fi
> >  
> >  cd $SCRIPTS_DIR/../..
> >  $PYTHON $SCRIPTS_DIR/clobberer/clobberer.py -s scripts -s buildprops.json \
> > +  $CLOBBERER_URL $branch "$builder" $slavebuilddir $slavename $master
> 
> Out of curiosity, why do we need quotes around the builder name?

I think you put that there in the original nightly_mobile_repacks.sh:
http://hg.mozilla.org/build/tools/rev/4903d4415882#l8.42

I'm not entirely sure why you put them there, but it seems to work.

I'm going to test these changes, along with trying to get the upload locations in the right places.

Comment 12

[[e-mail] (:bhearsum)]

(In reply to comment #11)
> > ::: scripts/l10n/release_repacks.sh
> > @@ +14,5 @@
> > >  JSONTOOL="$PYTHON $SCRIPTS_DIR/buildfarm/utils/jsontool.py"
> > >  workdir=`pwd`
> > >  
> > >  platform=$1
> > > +stage_platform=$2
> > 
> > Don't pass stage_platform to this script. You can pull it out of the
> > branchConfig in release-mobile-repacks.py.
> 
> Sadly, no.
> We've got 3 platform variables in mobile land:
> 
> complete_platform: linux-mobile, linux-android, linux-maemo5-gtk,
> macosx-mobile, win32-mobile, etc. # jhford wanted to avoid special casing
> android and maemo everywhere there was an |if platform.startswith('linux'):|
> platform: linux, macosx, win32  # catlee drops everything after the first -
> stage_platform: linux, android, maemo5-gtk, macosx, win32 # outwardly facing
> names

I understand this, but I still don't see an advantage to passing this through the shell scripts. You could drop the stage_platform references here, and add this in release-mobile-repacks.py:
stagePlatform = branchConfig["platforms"][options.platform]["stage_platform"]

Can't you?

Hard for me to be 100% sure without seeing the release.py glue.

Comment 13

[Aki Sasaki (not active)]

(In reply to comment #12)
> I understand this, but I still don't see an advantage to passing this
> through the shell scripts. You could drop the stage_platform references
> here, and add this in release-mobile-repacks.py:
> stagePlatform = branchConfig["platforms"][options.platform]["stage_platform"]
> 
> Can't you?

Maybe?

Yes if options.platform is 'linux-mobile' instead of 'linux'.  'linux' is desktop Firefox linux; 'linux-mobile' is mobile Firefox linux.  However, this duplicates logic to figure out stage_platform.  If that logic changes, we need to make those changes in multiple places if we only pass in one platform.  If we pass in both, that logic change only needs to happen in one place.  I think that's less ugly, until we get rid of programmatic configs.

What's the objection to passing in another variable?

Comment 14

[Aki Sasaki (not active)]

Attachment: tested on win32, darwin10, linux

This addresses all concerns, works on linux, and uploads to linux/LOCALE/ now.

I want to test on osx and win32. If that goes well, I'll ask for review.

Comment 15

[Aki Sasaki (not active)]

The above patch works on osx (used a darwin10 slave).
It breaks on win32 upload -- I'm guessing it has to do with hardcoded post_upload.py behavior for win32 that doesn't apply here, but I'm tracking it down.

Comment 16

[Aki Sasaki (not active)]

Win32 uploads work when we put staging keys on staging slaves.
Marking for review.

Comment 17

[[e-mail] (:bhearsum)]

Comment on attachment 539351 [details] [diff] [review]
tested on win32, darwin10, linux

Looks good to me!

Comment 18

[Aki Sasaki (not active)]

Comment on attachment 539351 [details] [diff] [review]
tested on win32, darwin10, linux

http://hg.mozilla.org/build/tools/rev/58c5b0cffb07

Comment 19

[Rail Aliiev [:rail]]

There is a typo:
s/--release-to-tryserver-builds/--release-to-try-builds/

I found that during comparing the same function from buildbotcustom/process/factory.py

Would be a problem to merge the functions (and import WithProperties here) to avoid such mistakes?

Comment 20

[Aki Sasaki (not active)]

(In reply to comment #19)
> There is a typo:
> s/--release-to-tryserver-builds/--release-to-try-builds/
> 
> I found that during comparing the same function from
> buildbotcustom/process/factory.py
> 
> Would be a problem to merge the functions (and import WithProperties here)
> to avoid such mistakes?

--release-to-try* is nowhere in my patch ?

Not entirely sure what's wrong here.

Comment 21

[Rail Aliiev [:rail]]

(In reply to comment #20)
> --release-to-try* is nowhere in my patch ?
> 
> Not entirely sure what's wrong here.

My bad, thought that you have added that function... I will file another bug to fix it. Sorry for the noise.

Comment 22

[Rail Aliiev [:rail]]

Attachment: fix lib/python/build/upload.py

This should fix lib/python/build/upload.py

Comment 23

[Rail Aliiev [:rail]]

Comment on attachment 540844 [details] [diff] [review]
fix lib/python/build/upload.py

Wrong bug...