add Mercurial and long filename support to patcher

RESOLVED FIXED

Status

defect
P3
normal
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: bhearsum, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [hg-automation][revisit testsuite])

Attachments

(4 attachments, 5 obsolete attachments)

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...
Priority: -- → P2
Whiteboard: [hg-automation]
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?
Attachment #337661 - Flags: review?(robert)
Posted patch diff of my actual changes (obsolete) — Splinter Review
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.
Attachment #337661 - Flags: review?(robert) → review+
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 :)
(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.
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 on attachment 337661 [details] [diff] [review]
[checked in] move nightly-partials.py to update-packaging, make snippets optional

changeset:   4:0bc4d327499a
Attachment #337661 - Attachment description: move nightly-partials.py to update-packaging, make snippets optional → [checked in] move nightly-partials.py to update-packaging, make snippets optional
Posted patch follow-up patch (obsolete) — Splinter Review
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.
Attachment #337879 - Flags: review?(ted.mielczarek)
Attachment #337879 - Flags: review?(robert)
Attachment #337879 - Flags: review?(robert) → review+
(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.)
(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.
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.
Depends on: 454594
Priority: P2 → P3
Whiteboard: [hg-automation] → [hg-automation][waiting on dep bug]
Priority: P3 → P2
(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 ?
(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.
(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.
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
(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.
(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).
(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?
Posted patch hg support for patcher (obsolete) — Splinter Review
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?
Attachment #337879 - Flags: review?(ted.mielczarek)
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.
Summary: add partial mar generation support to MercurialBuildFactory → add Mercurial and long filename support to patcher
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.
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.
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...
Attachment #338893 - Flags: review?(nthomas)
Posted patch initial moz191 patcher config (obsolete) — Splinter Review
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.)
Attachment #339303 - Flags: review?(nthomas)
Attachment #339303 - Attachment is obsolete: true
Attachment #339303 - Flags: review?(nthomas)
Comment on attachment 339303 [details] [diff] [review]
initial moz191 patcher config

urgh. so many mistakes in this. will re-post
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 ?
Attachment #338893 - Flags: review?(nthomas) → review+
(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.
slap me silly with the weapon of your choice - dunno why I though tools/update-packaging/ == tools/patcher
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
Attachment #337661 - Attachment is obsolete: true
Attachment #337662 - Attachment is obsolete: true
Attachment #337879 - Attachment is obsolete: true
Attachment #338893 - Attachment is obsolete: true
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.
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.
Attachment #339495 - Flags: review?(nthomas)
(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 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.
Attachment #339495 - Flags: review?(nthomas) → review+
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 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
Attachment #339495 - Attachment description: make_incremental_updates support for long filenames → [checked in] make_incremental_updates support for long filenames
No longer depends on: 454594
Whiteboard: [hg-automation][waiting on dep bug] → [hg-automation]
Whiteboard: [hg-automation] → [hg-automation][revisit testsuite]
Comment on attachment 339495 [details] [diff] [review]
[checked in] make_incremental_updates support for long filenames

Now in mozilla-central:
0bf7accdb2c2
Assignee: bhearsum → nobody
Status: ASSIGNED → NEW
Priority: P2 → P3
Attachment #343419 - Flags: review?(nthomas) → review+
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
Attachment #343419 - Attachment description: use from_decoded instead of partial_decoded (because partials are named without locale and platform in them) → [checked in] use from_decoded instead of partial_decoded (because partials are named without locale and platform in them)
Component: Release Engineering → Release Engineering: Future
Schrep's tests got imported a while ago.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Moving closed Future bugs into Release Engineering in preparation for removing the Future component.
Component: Release Engineering: Future → Release Engineering
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.