Closed Bug 661679 Opened 9 years ago Closed 9 years ago

release l10n scripts for mobile

Categories

(Release Engineering :: General, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aki, Assigned: aki)

References

Details

(Whiteboard: [l10n][mobile][automation])

Attachments

(2 files, 6 obsolete files)

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.
Assignee: nobody → aki
Attached patch wip py script (obsolete) — Splinter Review
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.
Attached patch wip tools changes (obsolete) — Splinter Review
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.
Attachment #537709 - Attachment is obsolete: true
Attached file test repack bash script (obsolete) —
This is what I'm using to repack from the commandline, to bypass the command line args + json.
Attachment #538200 - Flags: review?(rail) → review+
Attached patch wip: this works on linux [!] (obsolete) — Splinter Review
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.
Attachment #538444 - Flags: feedback?(bhearsum)
Attached file log for test-run of linux repack 1/5 (obsolete) —
Attachment #538199 - Attachment is obsolete: true
Looks like I'm missing xpis and need to change the upload dir to include platform + locale.
Er, no, xpis are in install/ for some reason.
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?
Attachment #538444 - Flags: feedback?(bhearsum) → feedback+
(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.
(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.
(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?
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.
Attachment #538201 - Attachment is obsolete: true
Attachment #538444 - Attachment is obsolete: true
Attachment #538445 - Attachment is obsolete: true
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.
Win32 uploads work when we put staging keys on staging slaves.
Marking for review.
Attachment #539351 - Attachment description: working on linux → tested on win32, darwin10, linux
Attachment #539351 - Flags: review?(bhearsum)
Comment on attachment 539351 [details] [diff] [review]
tested on win32, darwin10, linux

Looks good to me!
Attachment #539351 - Flags: review?(bhearsum) → review+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Attached patch fix lib/python/build/upload.py (obsolete) — Splinter Review
This should fix lib/python/build/upload.py
Attachment #540844 - Flags: review?(lsblakk)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 540844 [details] [diff] [review]
fix lib/python/build/upload.py

Wrong bug...
Attachment #540844 - Attachment is obsolete: true
Attachment #540844 - Flags: review?(lsblakk)
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.