454261 - add Mercurial and long filename support to patcher

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Description

[[e-mail] (:bhearsum)]

Rob Helmer has a nice script over in http://hg.mozilla.org/users/robert_roberthelmer.com/update-packaging/file/6d3625c5d700/nightly/nightly_partials.py that wraps a lot of the nightly partial mar generation. This will probably involve tidying it up a bit, putting it in tools/update-packaging, and getting MercurialBuildFactory to call it. Notably, this will specifically *not* involve generating snippets. For nightlies, we will leave that to the cronjob on prometheus-vm, for releases we will rely on patcher.

I think a good plan is to get this going on nightlies first, to help weed out any bugs.

This is similar to bug 404768, but I'm filing a new bug since that seems to be Tinderbox focused...

Comment 1

[[e-mail] (:bhearsum)]

Attachment: [checked in] move nightly-partials.py to update-packaging, make snippets optional

Mostly this patch just moves nightly_partials.py to update-packaging (since it depends on make_incremental_updates.py). It also makes snippets optional. I've got a better diff to post in a second.

Rob, I was thinking of just working out of your update-packaging repo until I get everything working. Is that ok with you?

Comment 2

[[e-mail] (:bhearsum)]

Attachment: diff of my actual changes

Comment 3

[Nick Thomas [:nthomas] (UTC+12)]

Could you elaborate on not generating snippets with Rob's stuff? eg What are the issues there, and what changes do you have in mind for snippet generation on prometheus-vm ? The latter is expecting to generate the partial too, so we'd have to teach it a new trick if it needs to d/l them instead. Might be less work to get the snippets going in buildbot.

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 337661 [details] [diff] [review]
[checked in] move nightly-partials.py to update-packaging, make snippets optional

diff looks fine. Working out of my hg repo is ok, don't need my permission :)

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> Could you elaborate on not generating snippets with Rob's stuff? eg What are
> the issues there, and what changes do you have in mind for snippet generation
> on prometheus-vm ? The latter is expecting to generate the partial too, so we'd
> have to teach it a new trick if it needs to d/l them instead. Might be less
> work to get the snippets going in buildbot.

The word Snippet is super overloaded here.

There are two text config files formats used for nightly updates -
1) tinderbox generated, for uploading to prometheus
2) prometheus generated, for uploading to aus

This script only does #1 so far. Prometheus has the history of prior updates needed to know where updates should go... Perhaps this is a moot point as current aus nightly behavior is to always upgrade to latest if the user is more than one update behind.

#2 style snippets are an older version of what is used for releases.

I think making the nightly partials script generate and upload #2 style snippets directly to aus would be ok for nightlies, would not work for releases since there are explicit update paths. Having a central place to store update path configuration is the point of the aus3 work, see my aus by repo for that.

Thinking about it now, the simplest path is probably to modify the script in this bug to output aus snippets and cut out prometheus. The requirements for releases are not so simple though, do not expect that to be as easy.

Comment 6

[[e-mail] (:bhearsum)]

Okay, I didn't realize that prometheus-vm wouldn't do a snippet for an already-generated partial. I'm going to finish testing the partial mar stuff then I'll tackle AUS snippets.

Comment 7

[[e-mail] (:bhearsum)]

Comment on attachment 337661 [details] [diff] [review]
[checked in] move nightly-partials.py to update-packaging, make snippets optional

changeset:   4:0bc4d327499a

Comment 8

[[e-mail] (:bhearsum)]

Attachment: follow-up patch

Highlights include:
* Makefile target to call nightly_partials.py - I'm open to suggestions about LATEST_MAR_URL - not really sure the best way to do these things in Makefiles.
* Use 'from_decoded' instead of 'partial_decoded' in make_incremental_updates.py (to avoid needing to parse the partial mar filename)
* Make nightly_partials use the "normal" partial mar format (for nightlies). Basically, s/version/buildid/. This kindof sucks because we have to unpack the mar twice. Once to get the buildid in nightly_partials, and make_incremental_updates.py does it again when actually creating the partial. I don't think this is *huge* overhead though.

I've tested this patch on all three platforms (by hand) and am in the process of testing with "real" nightlies on staging-master.

Comment 9

[[e-mail] (:bhearsum)]

(In reply to comment #5)
> There are two text config files formats used for nightly updates -
> 1) tinderbox generated, for uploading to prometheus
> 2) prometheus generated, for uploading to aus
> 
> This script only does #1 so far.

Are you sure it does #1? AFAIK, other than the filename, the snippet is exactly the format AUS expects. (Maybe that's the only difference, I'm not sure, tbh.)

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> (In reply to comment #5)
> > There are two text config files formats used for nightly updates -
> > 1) tinderbox generated, for uploading to prometheus
> > 2) prometheus generated, for uploading to aus
> > 
> > This script only does #1 so far.
> 
> Are you sure it does #1? AFAIK, other than the filename, the snippet is exactly
> the format AUS expects. (Maybe that's the only difference, I'm not sure, tbh.)

Hmm I do not remember actually. Sure would be convenient if it is already aus-style.

I think the differences were pretty minor anyway.

Comment 11

[[e-mail] (:bhearsum)]

So, I've got partial mars and aus snippets being generated with nightly_partials.py now. I'm in the process of writing a Makefile target that contains a bit of logic for uploading AUS snippets to aus2-staging. I'm going to wait until that's done to post final patches. Pretty sure I'm over all the major hurdles here, though. Once we have Ted's upload target I can finish testing and switch over the nightlies.

Because nightly_partials.py is going to be used for release too I'd like to rename it to something more approriate. It feels like what it's *really* for is generating partials at build time. Therefore I'm considering a name like 'inline_partials.py', or something simimar. Suggestions welcome.

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #11)
> Because nightly_partials.py is going to be used for release too I'd like to
> rename it to something more approriate. It feels like what it's *really* for is
> generating partials at build time. Therefore I'm considering a name like
> 'inline_partials.py', or something simimar. Suggestions welcome.

Hmm, didn't we discover that win32 partials should be created after the builds are signed, and therefore can't happen on the tinderbox ?

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to comment #12)
> (In reply to comment #11)
> > Because nightly_partials.py is going to be used for release too I'd like to
> > rename it to something more approriate. It feels like what it's *really* for is
> > generating partials at build time. Therefore I'm considering a name like
> > 'inline_partials.py', or something simimar. Suggestions welcome.
> 
> Hmm, didn't we discover that win32 partials should be created after the builds
> are signed, and therefore can't happen on the tinderbox ?

The signed builds could be transfered back to the tinderbox after signing.

The idea is that each build machine should be responsible for it's release tasks, e.g. no reason to block generating updates for Mac on the fact that Windows isn't signed.

In this scenario, the win32 release process would have a step between repack and updates where it waits for signed builds to appear (right now the whole release process waits on this, and it also takes ~3x the time it would if it just ran per-OS).

Linux and Mac could go ahead and generate/push updates and to final staging, for all it matters. Not sure if all the other pieces are in place for this, but I don't think it'd be hard to do if the desire is there.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to comment #12)
> (In reply to comment #11)
> > Because nightly_partials.py is going to be used for release too I'd like to
> > rename it to something more approriate. It feels like what it's *really* for is
> > generating partials at build time. Therefore I'm considering a name like
> > 'inline_partials.py', or something simimar. Suggestions welcome.
> 
> Hmm, didn't we discover that win32 partials should be created after the builds
> are signed, and therefore can't happen on the tinderbox ?

Hmm, yeah. I remember that. I think what Rob is saying makes sense, though. If we make "Build" look like this:
* en-US build
* sign
* partial mar
* repack
* sign installers

We'd work around this. We'd probably save a bunch of time by not doing repacks until we sign the en-US, too. That theory will require some testing, though.

Comment 15

[Nick Thomas [:nthomas] (UTC+12)]

Hmm, given our current codebase we'll have problems with that sequencing. Each locale has almost the same binaries as en-US, but setup.exe and localized/uninstall/helper.exe have localized strings and must be signed separately for each locale. So (unfortunately) we can't get away with just repacking a signed en-US build. Similarly we can't build partial mars for locales out of the en-US one, so we'd probably need something like
* en-US build
* l10n builds (repack)
* sign internals
* sign installers
* partial mars

Comment 16

[Robert Helmer [:rhelmer]]

(In reply to comment #15)
> Hmm, given our current codebase we'll have problems with that sequencing. Each
> locale has almost the same binaries as en-US, but setup.exe and
> localized/uninstall/helper.exe have localized strings and must be signed
> separately for each locale. So (unfortunately) we can't get away with just
> repacking a signed en-US build. Similarly we can't build partial mars for
> locales out of the en-US one, so we'd probably need something like
> * en-US build
> * l10n builds (repack)
> * sign internals
> * sign installers
> * partial mars

I agree, this makes sense, and that's the current sequence isn't it, except for signing installers which right now comes last?

Signing installers does not really need to happen for this bug does it? I think you might as well do it at the same time as signing internals, since you've got the key warmed up and everything :)

Absolutely need to repack before generating partials, it's not just an issue with signing (although this is an important point for win32) but with the fact that you're generating binary diffs, so you need to have the exact "from" and "to" file trees. The localization repack changes the contents of the "to" for each locale, so you need to have that before you can start in on partials.

make_incremental_updates.py was created to make partials faster in light of this fact; it uses caching to speed up the process considerably when dealing with lots of localized builds (under the assumption that the localization repack process changes very few files, so the expensive binary diff only needs to be done once for most files).

Anyway to cut through the noise of what I suggested before, the minimum change needed here would be to modify the update builder, and if you wanted a minor speedup to assign 3 buildslaves (one per OS) to that builder. I think it would be a better design to do "sign", "update", "stage" processes as buildsteps on each of the platform builders, instead of doing them as separate builders, but maybe that should be separated from this bug.

Comment 17

[[e-mail] (:bhearsum)]

(In reply to comment #16)
> (In reply to comment #15)
> I think it would
> be a better design to do "sign", "update", "stage" processes as buildsteps on
> each of the platform builders, instead of doing them as separate builders, but
> maybe that should be separated from this bug.

This is close to our original plan. We're certainly planning on getting rid of the separate "stage" step. I was going to leave Updates::Execute but have it only do snippet generation -- I'm not sure how we'd merge that part of it into the build process. I guess we could maintain separate patcher configs per platform but that seems like a pita! Besides, I think it's more important that MAR generation happens w/ the build.

Back to the point....where does this bug go from here? Given comments 15 and 16 I don't think adding partial mar support to MercurialBuildFactory is going to be very useful (because we need to do repacks before signing before partials).

I don't think putting build+repack+sign+partials in the same BuildFactory is a lot of help either, tbh. MercurialBuildFactory is already crammed full of things, and I think it's very good that as it stands now, we can use it for both nightlies and release builds.

I don't want to return to using patcher's '--create-patches' - that would force us to --build-tools, as well, which is just silly.

Given all of the previous comments, I'd like to propose the following:
* Build - stays as it is, does not do partials
* Repack - stays as it is
* Sign - does internals + installers
* Updates::Execute - looks like this:
** Download complete mars
** Run nightly_partials.py to generate partials
** Run patcher2.pl --create-patchinfo for snippets

Does this make sense to people? I need to confirm that we can --create-patchinfo w/out first running --build-tools (but I don't see why we can't).

Comment 18

[[e-mail] (:bhearsum)]

(In reply to comment #17)
> ** Run nightly_partials.py to generate partials

Of course, as soon as I started testing I realized that nightly_partials.py assumes mar and mbsdiff are built already - which is fine when we do partials @ build time, but not so great if we do them post-facto.

If we did build+repack+sign+updates in a single Builder we could get around this (because the builddir would exist and we'd be able to build the mar tools), but this seems less than ideal.

We could also enable nightly_partials.py (or just use a new script altogether) to checkout m-c and build the tools.

It's really starting to feel like our shortest path to success here is to just use patcher. We'd have to add a new option, --build-tools-hg (or something), to get the most recent version of the mar tools and such.

Thoughts?

Comment 19

[[e-mail] (:bhearsum)]

Attachment: hg support for patcher

Turns out that it's pretty trivial to support hg with patcher. I've tested --build-tools/--build-tools-hg, --download, and --create-patches locally.

Does it make sense to people to go this route?

Comment 20

[[e-mail] (:bhearsum)]

I'm morphing this bug since so much discussion has happened here. The route we're going now for partials+snippets is to add Mercurial + long file name support to patcher.

Comment 21

[[e-mail] (:bhearsum)]

So, adding Mercurial support is pretty trivial, as evidenced by my latest patch ('hg support for patcher').

Long filenames shouldn't be _too_ much harder since Patcher reads in URL and path formats from the patcher config. Unfortunately, patcher assumes that the format will be the same across all platforms. This assumption doesn't work when we get to filenames since Windows uses, "Firefox 3.1 Setup.exe", Mac is..."Firefox 3.1.dmg", and linux uses the short format (firefox-3.1.tar.bz2).

I've got a couple ideas about how to make this work...

One option is to simply maintain a separate patcher-config per platform for Mercurial based releases. This should/would involve no code changes to patcher, which is always plus. IIRC we don't manually edit patcher configs in the normal case, so it shouldn't be much of a maintenance burden. I think we may have to update the patcher config bumper to code with this, though.

Another option is to add support for per-platform URLs. This would work the same way the channel specific URLs currently do. This would definitely require patcher hacking. I feel like this option will take longer and be riskier, and could break 1.8 and 1.9 updates.

I really favour the first option (assuming it will work with as few changes as I believe it will). I'll do some testing and see if my assumptions are true.

In the meantime, any thoughts about this or other ideas would be much appreciated.

Comment 22

[[e-mail] (:bhearsum)]

A few more thoughts on platform-specific configs:
The other nice thing here is that we can parallelive generation of updates.

Doing this will give us snippet directories on aus2-staging for every platform * channel combination (total of 12) per release. We could probably work around this by having an additional builder that merges them all together.

Comment 23

[[e-mail] (:bhearsum)]

So, as it turns out, we don't pretty-up MAR file names. This means that none of workarounds listed in comment 21 or 22 are necessary. We *should* be able to simply change around the paths in the patcher config file and it will work. I did a short test of this by pulling from releases/...I didn't have any problems with it.

More to come...

Comment 24

[[e-mail] (:bhearsum)]

Attachment: initial moz191 patcher config

Just a basic patcher-config for 3.1 builds. I put in the <rc> section after looking at previous patcher configs but for the life of me I can't remember what it does. (And reading patcher code simply isn't helping today.)

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 339303 [details] [diff] [review]
initial moz191 patcher config

urgh. so many mistakes in this. will re-post

Comment 26

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 338893 [details] [diff] [review]
hg support for patcher

>Index: patcher2.pl
>+    if ($fromHg) {
>+        { # Pull mozilla-central
...
>+    } else {
>+        { # Checkout 'client.mk'.
>+    }

This patch looks fine to me. A suggestion though, the code that follows this block can be confusing, because it writes out a mozconfig and calls 'make -f client.mk'. In CVS that checks out and then builds, while m-c just builds. Please add a comment explaining this for those of used to the old behaviour. ;-)

A bigger question though, where does this land ? In both CVS and Hg ? I wonder if they have diverged at all since m-c was set up. Do you have a preference for using the same cvs/hg tag for all releases ? And managing this stuff in general ?

Comment 27

[[e-mail] (:bhearsum)]

(In reply to comment #26)
> A bigger question though, where does this land ? In both CVS and Hg ? I wonder
> if they have diverged at all since m-c was set up. Do you have a preference for
> using the same cvs/hg tag for all releases ? And managing this stuff in general
> ?

I don't think there's any other place for it land than CVS. Awhile back we removed patcher and a few other things from mozilla-central. To me, all of patcher lives in CVS...I don't think there's any reason to fork it.

Comment 28

[Nick Thomas [:nthomas] (UTC+12)]

slap me silly with the weapon of your choice - dunno why I though tools/update-packaging/ == tools/patcher

Comment 29

[[e-mail] (:bhearsum)]

Attachment: [checked in] support for building tools out of mozilla-central, with better comments

Checking in MozAUSConfig.pm;
/cvsroot/mozilla/tools/patcher/MozAUSConfig.pm,v  <--  MozAUSConfig.pm
new revision: 1.18; previous revision: 1.17
done
Checking in patcher2.pl;
/cvsroot/mozilla/tools/patcher/patcher2.pl,v  <--  patcher2.pl
new revision: 1.35; previous revision: 1.34
done

Comment 30

[[e-mail] (:bhearsum)]

Attachment: test run, 3.1a2 -> 3.1b1

Here's the output of a test-run with the official 3.1a2 builds and some test 3.1b1 builds. Everything looks OK except the 3.1a2 partial snippets. I've never been around for a b1 build so I'm not sure how patcher is supposed to be configured. I tried removing the <partial> block and/or removing the completemarurl from the <3.1a2> section. Both of those caused patcher to bail. I remember working around this for major update by symlinking 2.0.0.16-3.0.1 to 3.0.1 before running --create-patches. Do we normally have to do something like that for our first Beta?

Nick or Rob, any ideas here? The patcher config used is at the top, and two example snippets are at the bottom.


I had to patch make_incremental_updates.py a little bit to make this work. Specifically, I had to make decode_filename support both long filenames and releases/ style file/dir structure.

Comment 31

[[e-mail] (:bhearsum)]

Attachment: [checked in] make_incremental_updates support for long filenames

Here's the make_incremental_updates patch I needed to do a 3.1a2->3.1b1 test run. It's nothing complicated, simply tries both long filenames and "release" style file/dir structures in decode_filename().

I'm not sure whether to land this in both CVS and mozilla-central. It turns out we've already landed patches to this file in only one, so we're already forked...

In addition to the 3.1a2->3.1b1 test I also did a 3.0.1->3.0.2 test (limited to en-US + de for the sake of time). I did a spot check on the mars and snippets and they seemed OK.

Comment 32

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #30)
> Created an attachment (id=339483) [details]
> test run, 3.1a2 -> 3.1b1

Hmm, that's weird. I wonder if the fast-mode patcher generator created the small file referenced by the partial snippet. Not sure it's worth putting much effort into it, as it turns out we first did updates between b1 and b2 for Fx2.0 and 3.0, when you have a genuine partial and a1,a2 are easy complete updates without forcing the patcher config.

Comment 33

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 339495 [details] [diff] [review]
[checked in] make_incremental_updates support for long filenames

Seems fine to me, apart from noticing that (?P<version>(\w+\.)+\w+) is a working regexp for the version. r+ assuming that testing against schrep's test suite passes.

Comment 34

[[e-mail] (:bhearsum)]

Comment on attachment 339495 [details] [diff] [review]
[checked in] make_incremental_updates support for long filenames

So, I downloaded schrep's test suite from here: people.mozilla.org/~schrep/test.zip

It's a bit out of date, needs massaging to work again. (The filenames it uses are incompatible now, other, weirder issues).

I'm going to punt on testing with it for now. It's out of date now, and it's I had some strange issues trying to make it work. It's not the most robust but we should probably massage into working. I'll try to come back to this post-b1, but for now I think I need to move on.

Comment 35

[[e-mail] (:bhearsum)]

Comment on attachment 339495 [details] [diff] [review]
[checked in] make_incremental_updates support for long filenames

Checking in make_incremental_updates.py;
/cvsroot/mozilla/tools/update-packaging/make_incremental_updates.py,v  <--  make_incremental_updates.py
new revision: 1.8; previous revision: 1.7
done

Comment 36

[[e-mail] (:bhearsum)]

Comment on attachment 339495 [details] [diff] [review]
[checked in] make_incremental_updates support for long filenames

Now in mozilla-central:
0bf7accdb2c2

Comment 37

[[e-mail] (:bhearsum)]

Attachment: [checked in] use from_decoded instead of partial_decoded (because partials are named without locale and platform in them)

One last bit here, Nick.

Comment 38

[[e-mail] (:bhearsum)]

Comment on attachment 343419 [details] [diff] [review]
[checked in] use from_decoded instead of partial_decoded (because partials are named without locale and platform in them)

Checking in make_incremental_updates.py;
/cvsroot/mozilla/tools/update-packaging/make_incremental_updates.py,v  <--  make_incremental_updates.py
new revision: 1.9; previous revision: 1.8
done

changeset:   20584:d7d64f68423b

Comment 39

[Chris Cooper [:coop] (he/him)]

Schrep's tests got imported a while ago.

Comment 40

[Chris Cooper [:coop] (he/him)]

Moving closed Future bugs into Release Engineering in preparation for removing the Future component.