422103 - Buildbot should be able to generate update snippets

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[[e-mail] (:bhearsum)]

Basically, Buildbot should replicate the logic from post-mozilla-rel.pl::update_create_stats() (http://lxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#733) and line #692 on in post-mozilla-rel.pl::update_create_package() (http://lxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#692).

So, that's:
* Create partial and complete snippets
* Upload to aus2-staging

(Rob, this is what I wrote down from our conversation on Friday, can you confirm that this is correct?)

Comment 1

[Robert Helmer [:rhelmer]]

(In reply to comment #0)
> Basically, Buildbot should replicate the logic from
> post-mozilla-rel.pl::update_create_stats()
> (http://lxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#733)
> and line #692 on in post-mozilla-rel.pl::update_create_package()
> (http://lxr.mozilla.org/mozilla/source/tools/tinderbox/post-mozilla-rel.pl#692).
> 
> So, that's:
> * Create partial and complete snippets
> * Upload to aus2-staging
> 
> (Rob, this is what I wrote down from our conversation on Friday, can you
> confirm that this is correct?)
> 

Yes, although you only need to create and upload complete.txt IIRC, partials and partial.txt are done on the server side.

Comment 2

[[e-mail] (:bhearsum)]

> Yes, although you only need to create and upload complete.txt IIRC, partials
> and partial.txt are done on the server side.
> 

The code seems to agree with you.

Comment 3

[[e-mail] (:bhearsum)]

While looking at a possible implementation for this I noticed that it requires a reasonable amount of information that we can easily extract from the build system/tree (app version, buildid, size, hash, etc). This is the type of logic that I don't think we should put into every BuildStep that needs that information. I propose that we change the existing GetBuildID() function into SetMozillaBuildProperties() (or something like that -- the name is open to suggestion). It would gather the BuildID, md5 hash, size of the package, package filename, app version (and anything else we need) and set some as build properties. This way we can refer to them using WithProperties in ShellCommand's, File* steps, and have them easily accessible when we _do_ need to write a custom step.

How does that sound?

Comment 4

[[e-mail] (:bhearsum)]

CC'ing others in case they have thoughts on this.

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> While looking at a possible implementation for this I noticed that it requires
> a reasonable amount of information that we can easily extract from the build
> system/tree (app version, buildid, size, hash, etc). This is the type of logic
> that I don't think we should put into every BuildStep that needs that
> information. I propose that we change the existing GetBuildID() function into
> SetMozillaBuildProperties() (or something like that -- the name is open to
> suggestion). It would gather the BuildID, md5 hash, size of the package,
> package filename, app version (and anything else we need) and set some as build
> properties. This way we can refer to them using WithProperties in
> ShellCommand's, File* steps, and have them easily accessible when we _do_ need
> to write a custom step.
> 
> How does that sound?

Sounds good to me. We should be using sha1 not md5 (AUS supports both), I'd say it's not worth supporting MD5 anymore.

It should be really easy to gather this info, and would be really useful in other places e.g. Upload classes I'd imagine.

Comment 6

[[e-mail] (:bhearsum)]

Attachment: a data gathering BuildStep, master-side

Alright, this is part of the build property setting BuildStep talked about in previous comments. This part is pretty simple -- it calls the remote command, and parses the log to set build properties afterwards.

The Moz2, Mobile, and any other Buildbot currently using GetBuildID should be switched over to this once it's in the tree.

Second patch to come.

Comment 7

[[e-mail] (:bhearsum)]

Attachment: slave side command

This is where the work is actually done. Note that this is a patch that will go right into our Buildbot tree (BUILDBOT_0_7_6_BRANCH). Because it's a class required on the slave there's really no good way to do this -- a buildbotcustom strategy will not work there.

I've put it way down at the bottom of commands.py to try and avoid merge conflicts when we pull patches from upstream.

The class itself isn't _too_ complicated. It finds the buildid, app version, package filename/hash/size, installer filename/hash/size (on win32), and complete mar filename/hash/size. It only supports sha1 as a hash function. If any of these things don't exist, or can't be found for some reason it will still set the build property of that name, but it will be set as 'None'. Therefore, anything trying to access these properties should be testing for that.

I feel like this class is a little messy -- maybe I should move some of the data gathering sections into functions to make it a bit clearer, I'm not really sure. Suggestions are welcome, though!

Comment 8

[[e-mail] (:bhearsum)]

I did a Build on my MacBook with this class as the last step. Here's what was dumped in the log (the complete update looks funny but that is the correct size -- i had trouble getting a proper update going on this machine):

buildid: 2008031914
appVersion: 3.0b3pre
packageFilename: firefox-3.0b3pre.en-US.mac.dmg
packageHash: 804c9e422098e623ac51e9477bcc325e7827dfcb
packageSize: 10639883
installerFilename: UNKNOWN
installerHash: UNKNOWN
installerSize: UNKNOWN
completeMarFilename: firefox-3.0b3pre.en-US.mac.complete.mar
completeMarHash: b1bb099613e475c0893304ff4daa8f31f1b794c2
completeMarSize: 2566


I verified the file sizes and hashes for the files -- they're all correct.


After these are checked in the rest of this bug should be a synch.

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 310540 [details] [diff] [review]
slave side command

I like what this gives us, but I wish there was a better way to load slave commands (without having to add it to the buildbot install).

Seems like the master could upload these classes on-demand to slaves.. I guess that's really a buildbot RFE though :)

Comment 10

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 310540 [details] [diff] [review]
slave side command

from my minimal understanding of the original code you're porting from, this looks like it'll do the job. I'll echo rhelmer's sentiments that I wish we could put this in some bolted-on file rather than in the buildbot tree.

Would it be possible to add a module to a local python's site directory that imports buildbot.slave.commands, adds SetMozillaBuildProperties and then adds it through registerSlaveCommand?

That might be a path to "easy" slave-side buildbot extensions without polluting the tree.

anyway, r+ for now, but I think that's worth investigating.

Comment 11

[[e-mail] (:bhearsum)]

(In reply to comment #10)
> (From update of attachment 310540 [details] [diff] [review])
> Would it be possible to add a module to a local python's site directory that
> imports buildbot.slave.commands, adds SetMozillaBuildProperties and then adds
> it through registerSlaveCommand?
> 

I'm not sure how this would work...and to be honest, if we have something that requires slave installation I would prefer it to be in the Buildbot tree, I think.

I'm in the process of coding up the rest of this bug now and I've found that it is trivial to run multiple RemoteCommand's in a single BuildStep. If there was a standard Buildbot RemoteCommand that executed any Python you fed it (on the slave) that would be *perfect* for what we need. This is long-haul type of thinking, but it seems like the right way to do it.

Comment 12

[[e-mail] (:bhearsum)]

Attachment: rename BuildID -> buildid, Sha1Hash -> Hash

I've renamed buildid to be consistent with the existing GetBuildID step and the Sha1Hash properties just for the sake of simplicity.

Comment 13

[[e-mail] (:bhearsum)]

Attachment: fix windows installer finding, look for partial mar too

I've fixed a problem where the windows installer wasn't being looked for in the right place. I also forgot to look for a partial mar, whoops ;-).

Comment 14

[[e-mail] (:bhearsum)]

Attachment: generate update snippets within Buildbot

This step depends on SetMozillaBuildProperties to provide it with most of the information it needs. It's pretty straightforward with one trick: The FileDownload SlaveCommand is used to actually create the files on the slaves. I followed the pattern used in buildbot.steps.transfer.FileDownload -- using _FileReader on the master and passing it to the SlaveCommand. I've tested this pretty well -- all combinations of createPartialSnippet && createCompleteSnippet.

No point in uploading to aus2-staging in this step -- we can do that with a ShellCommand later.

Comment 15

[Robert Helmer [:rhelmer]]

Comment on attachment 311356 [details] [diff] [review]
fix windows installer finding, look for partial mar too

Hmm, I don't think you'll find a partial MAR, the build system only generates a complete MAR. What is this for?

Comment 16

[[e-mail] (:bhearsum)]

Attachment: same as before, forget about the partials

Comment 17

[[e-mail] (:bhearsum)]

Attachment: [checked in] SetMozillaBuildProperties, no partial updates

Comment 18

[[e-mail] (:bhearsum)]

Attachment: [checked in] slave-side command, no partials

Comment 19

[[e-mail] (:bhearsum)]

Comment on attachment 311403 [details] [diff] [review]
[checked in] slave-side command, no partials

Checking in buildbot/slave/commands.py;
/cvsroot/mozilla/tools/buildbot/buildbot/slave/commands.py,v  <--  commands.py
new revision: 1.1.1.2.2.3; previous revision: 1.1.1.2.2.2
done

Comment 20

[[e-mail] (:bhearsum)]

Comment on attachment 311402 [details] [diff] [review]
[checked in] SetMozillaBuildProperties, no partial updates

Checking in steps/misc.py;
/cvsroot/mozilla/tools/buildbotcustom/steps/misc.py,v  <--  misc.py
new revision: 1.3; previous revision: 1.2
done

Comment 21

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

Comment on attachment 311401 [details] [diff] [review]
same as before, forget about the partials

The snippet specific stuff looks OK in here, though obviously this needs finishing:
+        # download URL
+        snippet += "URL GOES HERE\n"

I'll leave it to the Robs to comment on the buildbot stuff.

Comment 22

[[e-mail] (:bhearsum)]

Attachment: [checked in] buildstep to generate complete snippet - take 3

Alright, this is a complete version of this class -- it even spits out real URLs.

It is able to generate the same type of URLs we use for nightly updates. I've made the datedDir optional in case we want to support updates for other types of builds in the future (eg, hourlies). It *should* support bouncer URLs out-of-box through the use of WithProperties. With a bit more work it could support generating them in a more intelligent manner within the class.

I went back and forth on whether or not to make 'milestone' a build property. I decided against it because it's not something we gather from the tree/objdir - so it has to be in the master.cfg *anyways*. I'm open to change there, though.

Comment 23

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

Comment on attachment 311784 [details] [diff] [review]
[checked in] buildstep to generate complete snippet - take 3

Seems fine to me. 

How will uploading the snippet to aus2-staging be done ? (another function here vs directly by a step). Are you aiming at providing updates for more than Moz2 branch ? Don't think the existing tools and AUS support the latter, but just mozilla-central would work OK.

Comment 24

[[e-mail] (:bhearsum)]

(In reply to comment #23)
> (From update of attachment 311784 [details] [diff] [review])
> Seems fine to me. 
> 
> How will uploading the snippet to aus2-staging be done ? (another function here
> vs directly by a step). Are you aiming at providing updates for more than Moz2
> branch ? Don't think the existing tools and AUS support the latter, but just
> mozilla-central would work OK.
> 

(I could've sworn I replied to this already.)

Uploading will be done in a simple ShellCommand. No need for a custom class since the snippet name is static. I think we will want to provide updates for more than just mozilla-central at some point...just that should be fine for now, though. I see what you mean about the tools and AUS, though. I wonder if we should add 'milestone' in there, or something. In any case, that's a different discussion.

Comment 25

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

For background, the issue is that the builders publish to a directory on aus2-staging depending on their branch; AUS maps the app version & name to a dir with the likes of 
  http://mxr.mozilla.org/mozilla/source/webtools/aus/xml/inc/config-dist.php#83
We'd need some way to distinguish between various long-lived Moz2 branches in the update request, in order to know what update to give them. The partial patch generator should scale without problems.

Comment 26

[Benjamin Smedberg]

Because the version numbers will be the same, I don't think we can use branch mappings: we'll need to use different update channels, e.g.:

"nightly" for mozilla-central
"actionmonkey-nightly" for actionmonkey
etc...

But really, we can punt on this for now... mozilla-central updates are quite a bit more important than the others.

Comment 27

[[e-mail] (:bhearsum)]

Comment on attachment 311784 [details] [diff] [review]
[checked in] buildstep to generate complete snippet - take 3

Checking in updates.py;
/cvsroot/mozilla/tools/buildbotcustom/steps/updates.py,v  <--  updates.py
initial revision: 1.1
done

Comment 28

[[e-mail] (:bhearsum)]

Going to keep this open while I get updates going for mozilla-central.

Comment 29

[[e-mail] (:bhearsum)]

Things appear to be OK with this. SetMozillaBuildProperties is going red when it shouldn't be, I'd like to fix that before resolving this bug.

Comment 30

[[e-mail] (:bhearsum)]

Attachment: [checked in] one line fix for SetMozillaBuildProperties

It turns out that a newline at the end causes the master-side step to throw an exception. Whould've thunk it!

Comment 31

[Robert Helmer [:rhelmer]]

Comment on attachment 314070 [details] [diff] [review]
[checked in] one line fix for SetMozillaBuildProperties

Hmm, what exception does it throw? Just wondering why this line and not the lines above it..

Comment 32

[[e-mail] (:bhearsum)]

It throws something like this:
>>> foo = "\na\nb\nc\n"
>>> a,b = foo.split("\n")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: too many values to unpack
>>> 

Inside the loop it attempts to split an empty string (property.split() where property is an empty string).

Comment 33

[[e-mail] (:bhearsum)]

Comment on attachment 314070 [details] [diff] [review]
[checked in] one line fix for SetMozillaBuildProperties

landed on BUILDBOT_0_7_6_BRANCH:
Checking in buildbot/slave/commands.py;
/cvsroot/mozilla/tools/buildbot/buildbot/slave/commands.py,v  <--  commands.py
new revision: 1.1.1.2.2.4; previous revision: 1.1.1.2.2.3
done