352230 - Framework for automated build/release harness

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Robert Helmer [:rhelmer]]

We need to automate our release procedure. The first iteration should be a scripted version of our current human-driven process.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: basic description of release process

These are the high-level steps I am targetting for automation.

Comment 2

[Robert Helmer [:rhelmer]]

Named the app-specific parts "Bootstrap".

Landed in mozilla/tools/release/

Comment 3

[Robert Helmer [:rhelmer]]

moving component back

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: example implementation for "Tag" step

Before I go too far down this road, please take a look specifically at the "Tag.pm" changes. Do you have any comments before I implement the rest of the steps?

Comment 5

[J. Paul Reed [:preed]]

Comment on attachment 241000 [details] [diff] [review]
example implementation for "Tag" step

I would translate all possible shell functions to their perl equivalents as part of this automation build out.

i.e.: mkdir -p cvsroot/ becomes mkdir("cvsroot");

ln -fs could be just a symlink().

The |grep -v| calls can be turned into open()/while(<>)/close() calls.

Then, error codes can be checked, including $!, so we get more information than just a "it failed" from RunShellCommand().

Also, I would use chdir() in perl., as opposed to calling cd $dir && $cmd. You may want to just add this functionality to RunShellCommand() (optionally, of course).

I don't really understand the config stuff. I guess that's just for testing?

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: refactored example

Here is an example which uses Perl rather than Shell where appropriate and attempts to catch and throw errors using "die" (as all Step instances are run inside an eval).

Also, tagging is refactored out to a private method with built-in logging and error checking.

The Config implementation is just for testing (will switch it over to Config::General later); I am most interested in feedback on the Tag step for now.

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: refactored example

Same as last patch, just missed a few spots.

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: refactored example

One more time - removed some typos.

Comment 9

[Dave Liebreich [:davel]]

Comment on attachment 241019 [details] [diff] [review]
refactored example

Where does _MINIBRANCH come from here:

+    # Update client.mk to the minibranch you just created.
+    $this->Shell(
+      'cmd' => 'cvs up -r '.$releaseTag.'_MINIBRANCH client.mk',
+      'dir' => "$tagDir/cvsroot/mozilla",
+    );

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> (From update of attachment 241019 [details] [diff] [review] [edit])
> Where does _MINIBRANCH come from here:
> 
> +    # Update client.mk to the minibranch you just created.
> +    $this->Shell(
> +      'cmd' => 'cvs up -r '.$releaseTag.'_MINIBRANCH client.mk',
> +      'dir' => "$tagDir/cvsroot/mozilla",
> +    );
> 

It's hardcoded; the name of the minibranch should be e.g.
FIREFOX_1_5_0_7_MINIBRANCH

Since it's used more than once, a $minibranchTag might make it more readable.

Comment 11

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> (From update of attachment 241019 [details] [diff] [review] [edit])
> Where does _MINIBRANCH come from here:
> 
> +    # Update client.mk to the minibranch you just created.
> +    $this->Shell(
> +      'cmd' => 'cvs up -r '.$releaseTag.'_MINIBRANCH client.mk',
> +      'dir' => "$tagDir/cvsroot/mozilla",
> +    );
> 

Oh, nevermind, I see what you are saying :) There is a $releaseTag typo above it. 

I have a patch now in my local tree to fix this, I'm going to wait for more comments and roll the feedback up into a new patch tomorrow.

Comment 12

[Reed Loden [:reed]]

Comment on attachment 241019 [details] [diff] [review]
refactored example

>+    my $cvsroot        = ':ext:[e-mail]:';
>+    my $mozillaCvsroot = $cvsroot.'/mozilla';
>+    my $mofoCvsroot    = $cvsroot.':/mofo';
>+    my $l10nCvsroot    = $cvsroot.'/l10n';

Notice that $mofoCvsroot has an extra ':' in it. That's a problem. :)

Comment 13

[Robert Helmer [:rhelmer]]

Attachment: corrections with test, and Build example

Assuming the previous patch is the right general idea, here are:

* corrections to the Tag class for issues mentioned
* example Build class
* refactor out CheckLog to Step class
* basic unit test infrastructure and Dummy test (Step) class

Comment 14

[J. Paul Reed [:preed]]

Comment on attachment 241019 [details] [diff] [review]
refactored example

Again: could we get the shell out of the perl?

Is there a reason to output to tee instead of using RunShellCommand and grabbing the output?

Is there a reason CvsTag is "private"? If it's a member of the class, shouldn't the calls be $this->_CvsTag()? (then they could probably be $this->CvsTag().

Also, could we get some horizontal spacing, i.e.:
For '.$productName.' '.$version.', redirect client.mk onto the '.$releaseTag.' tag." client.mk',

becomes

For ' . $productName . ' ' . $version . ', redirect client.mk onto the ' . $releaseTag . ' tag." client.mk',

(Note in the above example: nice use of ' vs. "; that's really helpful and improves readability.)

Comment 15

[Robert Helmer [:rhelmer]]

(In reply to comment #14)
> (From update of attachment 241019 [details] [diff] [review] [edit])
>
> Is there a reason to output to tee instead of using RunShellCommand and
> grabbing the output?


Could be done. Are there other examples or is it just the CVS commands?


> Is there a reason CvsTag is "private"? If it's a member of the class, shouldn't
> the calls be $this->_CvsTag()? (then they could probably be $this->CvsTag().


It's private because I don't plan on using it outside of this class (it's an implementation detail) and it's being called as a static method. If that changes it can be moved (not that _CheckCvsLog was moved to Step::CheckCvsLog).

It's probably not worth bothering about the static vs. instance method for now, if we had two instances of Tag I think this would be fine as a static method though.


> Also, could we get some horizontal spacing, i.e.:
> For '.$productName.' '.$version.', redirect client.mk onto the '.$releaseTag.'
> tag." client.mk',
> 
> becomes
> 
> For ' . $productName . ' ' . $version . ', redirect client.mk onto the ' .
> $releaseTag . ' tag." client.mk',
> 
> (Note in the above example: nice use of ' vs. "; that's really helpful and
> improves readability.) 


Sure. I'll put together a more complete patch and request review on that.

Comment 16

[J. Paul Reed [:preed]]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 241019 [details] [diff] [review] [edit] [edit])
> >
> > Is there a reason to output to tee instead of using RunShellCommand and
> > grabbing the output?
> 
> Could be done. Are there other examples or is it just the CVS commands?

Every shell command that has a system call equivalent in Perl should be translated.

Similarly, with things that can be done in perl directly (gathering output, the sed commands for the client.mk changes, etc.) should be done in perl where possible.

The reasoning here is that calling a system call or doing it in perl gives you more information in failure conditions and control than calling out to shell to do the same.

Comment 17

[Dave Liebreich [:davel]]

I'm not sure I like the side effect of dir - it means the build steps have to keep track of the directory changes, or specify the dir for all commands, or both.

I'm not sure what to do about it, other than save and restore the current working directory.

I'll have more comments later.

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to comment #17)
> I'm not sure I like the side effect of dir - it means the build steps have to
> keep track of the directory changes, or specify the dir for all commands, or
> both.
> 
> I'm not sure what to do about it, other than save and restore the current
> working directory.
 

The assumption I've been going with is that the absolute path will be specified for all commands, to eliminate ambiguity.

Comment 19

[Dave Liebreich [:davel]]

in tools/release/Bootstrap/Step.pm:

Move the call to MozBuild::Util::RunShellCommand after the log message stating that you are running a command.  or s/Running/Ran/

line 36: s/rc/rv/

still reviewing.

Comment 20

[Robert Helmer [:rhelmer]]

I think it probably makes sense to morph this into a "framework" bug, and for me to create a separate tracking bug (and have this be a dependency) so the doesn't get too huge.

I have a feeling that we're going to go through a lot of patches :)

Comment 21

[Robert Helmer [:rhelmer]]

Attachment: just platform/framework improvements

These are just platform/framework improvements I'd like to land ASAP.

* fix typos etc. pointed out
* more complete shell error handling
* fix off-by-one error ("<" vs "<=") in release
* add substeps e.g. Build::Push
* add basic tests
* use Config::General
* make Step::Shell timeout overridable
* refactor log checking functions

Let me know if you see anything wrong with this patch; this is the minimum I want to get landed before trying to get the Steps landed (will do that in separate bug(s)).

Comment 22

[J. Paul Reed [:preed]]

Comment on attachment 241228 [details] [diff] [review]
just platform/framework improvements


>-    while ($currentStep < $#allSteps) {
>+    while ($currentStep <= $#allSteps) {

For things like this, I'm a big fan of scalar(@array); it reduces one-off errors like this; so:

while ($currentStep < scalar(@allSteps)) {
   ...
}

>==================================================================
>--- bootstrap.cfg	(/mirror/release/trunk)	(revision 1580)
>+++ bootstrap.cfg	(/local/release/trunk)	(revision 1580)
>@@ -0,0 +1,9 @@
>+productTag = FIREFOX_1_5_0_7
>+branchTag  = MOZILLA_1_8_0_7
>+pullDate   = 2006-09-06 18:00 PDT
>+rc         = 1
>+version    = 1.5.0.7
>+appName    = browser
>+product    = firefox
>+buildDir   = /builds/tinderbox/
>+pushDate   = 2006090601

Does it make sense to wrap this in a <release> stanza, much like patcher does?


>+# shared static config
>+my $conf = new Config::General("bootstrap.cfg");

You might want to do some more error checking here. Patcher2 does this, and if there's a typo in the config file, it silently dies and and leaves you with a null $conf, which causes problems later.

>+    if ($dir) {
>+        chdir($dir) || die "Cannot chdir to $dir: $!";
>+    }

You probably want this function to be idempotent, so do you want to save/restore the dir?

Looks good otherwise.

Comment 23

[Robert Helmer [:rhelmer]]

Landed on trunk with suggestions from comment #22. Thanks!

Comment 24

[Robert Helmer [:rhelmer]]

Attachment: framework improvements

* support logFile option in RunShellCommand
* return PID from RunShellCommand
** attempt to kill -9 PID if timeout is reached
* centralized logDir
* catch timeouts vs. bad shell exits properly in Step::Shell()

Comment 25

[T.R. Fullhart]

Comment on attachment 241839 [details] [diff] [review]
framework improvements

BTW, CurrentTime() is equivalent to strftime("%T %D", localtime()), if you didn't already know. I like the idea of having a function that knows what our preferred timestamp format is, though.

Comment 26

[Robert Helmer [:rhelmer]]

Attachment: framework improvements

Same improvements as last patch, plus:

release:
 * ability to run just execute or just verify in each step

MozBuild/Util.pm:
 * do not buffer log output

Bootstrap/Step.pm:
 * raise default timeout to 1h
 * use strftime() to make CurrentTime clearer (thanks trf)

Makefile:
 * clean_stage target

README:
 * specify "Pre-flight Checklist", rather than non-existent "Tools run first" :)

I'd like to land this before making more fundamental improvements, let me know what you think.

Comment 27

[Robert Helmer [:rhelmer]]

Attachment: framework improvements

Basically same patch as before, I simplified the config file since I think it needs some rethought, I think the right way to go will probably be something more like patcher uses, so we maintain a history of previous releases. Here is a breakdown of this patch vs. CVS:

* release - more options (just verify or just execute steps)

* t/Bootstrap/Step/Dummy.pm - logFile is required for Shell now

* MozBuild/Util.pm - Shell() autoflush and return PID, use logfile; add MkdirWithPath

* Bootstrap/Step.pm - track time, proper use of Shell error handling, use logFile, die if no config file

* README - more in touch with reality, explain the steps that are still manual

Comment 28

[J. Paul Reed [:preed]]

Comment on attachment 244366 [details] [diff] [review]
framework improvements


>+        if (defined($logfile)) {
>+            open(LOGFILE, ">> $logfile") or die "Could not open logfile $logfile: $!";
>+            LOGFILE->autoflush(1);

If you're doing this for LOGFILE (which I think is a great idea), you may want to do it for STDOUT if $printOutputImmediately is also set.

>+             pid => $pid,
>+    };

Is there a reason for doing this? If that pid is no longer living, then you're handing back invalid data.

>+sub MkdirWithPath
>+{
>+    my $dirToCreate = shift;
>+    my $printProgress = shift;
>+    my $dirMask = shift;

You're gonna hate me, but do you want to %args-ify this? I'd make the args names shorter, too; maybe:

MkdirWithPath(dir => "new_directory", print => 0, mask => 0777) ??

>+    die "ASSERT: MkdirWithPath() needs an arg\n" if not defined($dirToCreate);

I was lazy when I wrote this; if we want to be anal, we might look at what mkpath does if you just call it without any arguments; I'll bet it tries to create $_!! ;-)

>==================================================================
>--- bootstrap.cfg	(/mirror/release/trunk)	(revision 1730)
>+++ bootstrap.cfg	(/local/release/trunk)	(revision 1730)
>@@ -1,13 +1,17 @@
>-<app firefox>
>-  <release 1.5.0.7>
>-    productTag = FIREFOX_1_5_0_7
>-    branchTag  = MOZILLA_1_8_0_7
>-    pullDate   = 2006-09-06 18:00 PDT
>-    rc         = 1
>-    version    = 1.5.0.7
>-    appName    = browser
>-    product    = firefox
>-    buildDir   = /builds/tinderbox/
>-    pushDate   = 2006090601
>-  </release>
>+<app>
>+  productTag      = FIREFOX_1_6_0_7
>+  branchTag       = MOZILLA_1_8_0_BRANCH
>+  pullDate        = 2006-10-30 17:45 PDT
>+  rc              = 1
>+  version         = 1.6.0.7
>+  appName         = browser
>+  product         = firefox
>+  buildDir        = /builds/tinderbox/Fx-Mozilla1.8.0-Release
>+  l10n-buildDir   = /builds/tinderbox/Fx-Mozilla1.8.0-l10n-Release
>+  pushDate        = 2006-10-30-22
>+  logDir          = /builds/release/logs
>+  mozillaCvsroot  = /builds/cvsmirror/cvsroot
>+  l10nCvsroot     = /builds/cvsmirror/l10n
>+  mofoCvsroot     = /builds/cvsmirror/mofo
>+  stageHome       = /data/cltbld
> </app>

Hrm... I liked the old way better. Why'd it change?

>+    my $pid = $rv->{'pid'};
>+    print "Pid: $pid\n";
>     if ($timedOut) {
>-        $this->Log('msg' => "output: $rv->{'output'}");
>+        $this->Log('msg' => "output: $rv->{'output'}") if $rv->{'output'};
>+        system('kill -9 '.$pid);

Use perl's kill().

Also, if the timeout is hit, why isn't RunShellCommand() doing this?

All the caller's shouldn't be responsible for cleaning up after RunShellCommand(). I seem to remember discussing this, though, and there might have been a reason?

>+sub CurrentTime() {
>+    my $this = shift;
>+    my $args = @_;
>+
>+    return strftime("%T %D", localtime());

Nit: I wouldn't bother creating a utility function for this if you're just calling it once or twice, and the only purpose there is pretty printing it for the user. Does scalar(localtime()) not give you something workable?

>+sub MkdirWithPath() {
>+    my $this = shift;
>+    my $args = $_[0];
>+    MozBuild::Util::MkdirWithPath($args);

Why is there a wrapper for this?

Comment 29

[Robert Helmer [:rhelmer]]

(In reply to comment #28)
> (From update of attachment 244366 [details] [diff] [review] [edit])
> 
> >+        if (defined($logfile)) {
> >+            open(LOGFILE, ">> $logfile") or die "Could not open logfile $logfile: $!";
> >+            LOGFILE->autoflush(1);
> 
> If you're doing this for LOGFILE (which I think is a great idea), you may want
> to do it for STDOUT if $printOutputImmediately is also set.


Yeah good call.


> >+             pid => $pid,
> >+    };
> 
> Is there a reason for doing this? If that pid is no longer living, then you're
> handing back invalid data.


This is related to a comment below, I'll address there.


> >+sub MkdirWithPath
> >+{
> >+    my $dirToCreate = shift;
> >+    my $printProgress = shift;
> >+    my $dirMask = shift;
> 
> You're gonna hate me, but do you want to %args-ify this? I'd make the args
> names shorter, too; maybe:


Yes, I will fix up *your* code :P


> MkdirWithPath(dir => "new_directory", print => 0, mask => 0777) ??
> 
> >+    die "ASSERT: MkdirWithPath() needs an arg\n" if not defined($dirToCreate);
> 
> I was lazy when I wrote this; if we want to be anal, we might look at what
> mkpath does if you just call it without any arguments; I'll bet it tries to
> create $_!! ;-)


Sure.


> >==================================================================
> >--- bootstrap.cfg	(/mirror/release/trunk)	(revision 1730)
> >+++ bootstrap.cfg	(/local/release/trunk)	(revision 1730)
> Hrm... I liked the old way better. Why'd it change?


I need to rethink the config file.. it will come back, but "release" needs a way to specify app and version, I realized.


> >+    my $pid = $rv->{'pid'};
> >+    print "Pid: $pid\n";
> >     if ($timedOut) {
> >-        $this->Log('msg' => "output: $rv->{'output'}");
> >+        $this->Log('msg' => "output: $rv->{'output'}") if $rv->{'output'};
> >+        system('kill -9 '.$pid);
> 
> Use perl's kill().
> 
> Also, if the timeout is hit, why isn't RunShellCommand() doing this?
> 
> All the caller's shouldn't be responsible for cleaning up after
> RunShellCommand(). I seem to remember discussing this, though, and there might
> have been a reason?


Yeah I think that's a good call.. then there isn't much reason to return the PID above either.


> >+sub CurrentTime() {
> >+    my $this = shift;
> >+    my $args = @_;
> >+
> >+    return strftime("%T %D", localtime());
> 
> Nit: I wouldn't bother creating a utility function for this if you're just
> calling it once or twice, and the only purpose there is pretty printing it for
> the user. Does scalar(localtime()) not give you something workable?


What I really want to do is make a timer method, just haven't gotten around to it yet. Also, before tfullhart pointed out that I may as well just use strftime(), it was a non-trivial chunk of code, so it made more sense at the time.

 
> >+sub MkdirWithPath() {
> >+    my $this = shift;
> >+    my $args = $_[0];
> >+    MozBuild::Util::MkdirWithPath($args);
> 
> Why is there a wrapper for this?


Given the above there isn't really much point.. I have been trying to hide implementation details from the individual Step classes, in this case I may as well pull in the MozBuild/Util.pm dependency and call it directly.

I noticed you +'d the review - is it ok if I land this stuff and then fix these in (one or more) separate patches? The current patch is getting kind of large, and as I've tested I'd like to land it soon :)

If so, I am going to wait until the latest patch in bug 356185 is reviewed to land - the Step changes should land at the same time.

Comment 30

[J. Paul Reed [:preed]]

(In reply to comment #29)

> I noticed you +'d the review - is it ok if I land this stuff and then fix these
> in (one or more) separate patches? The current patch is getting kind of large,
> and as I've tested I'd like to land it soon :)

I should've clarified: r+, with the above issues addressed.

I'm fine with however you land it, although I don't know which workflow you're using. It would be nice to be able to diff the changes, in a coherent way, and see the patch, with review comments addressed, in CVS. But I know you're using svn/svk for certain things, so

I'll review the Step changes shortly.

Comment 31

[Robert Helmer [:rhelmer]]

(In reply to comment #30)
> (In reply to comment #29)
> 
> > I noticed you +'d the review - is it ok if I land this stuff and then fix these
> > in (one or more) separate patches? The current patch is getting kind of large,
> > and as I've tested I'd like to land it soon :)
> 
> I should've clarified: r+, with the above issues addressed.


Ok, makes sense.

 
> I'm fine with however you land it, although I don't know which workflow you're
> using. It would be nice to be able to diff the changes, in a coherent way, and
> see the patch, with review comments addressed, in CVS. But I know you're using
> svn/svk for certain things, so


Well of course I can do really fine-grained diffs using my SVK repo :) The thing I am concerned about is being out from CVS so long, so for you as reviewer it's a gigantic patch over and over.

What I'd like to do is land what I have (since I've gone through a complete test cycle with it now), and then fix these issue and ask you for a review on just that patch.


> I'll review the Step changes shortly.


Great, thanks!

Comment 32

[Robert Helmer [:rhelmer]]

Attachment: address concerns from previous patch

I am going to go ahead and land this based on the previous r=preed

Comment 33

[Robert Helmer [:rhelmer]]

Attachment: drop config::general dependency

Since we're not really using the expressive power of Config::General, I've dropped the dependency in favor of a perl-based config file, to make this easier to deploy. 

It uses no other deps that are not part of perl 5.8 AFAICT. We've been talking about pulling this info from a centralized store anyway, so this is probably enough until we get that sorted.

Comment 34

[Robert Helmer [:rhelmer]]

Comment on attachment 246230 [details] [diff] [review]
drop config::general dependency

sorry wrong config, one sec

Comment 35

[Robert Helmer [:rhelmer]]

Attachment: implement a simple parser instead of just a "do"

had to "use IO::Handle" to get "autoflush" since apparently Config::General was using that for us before :)

Comment 36

[J. Paul Reed [:preed]]

Comment on attachment 246232 [details] [diff] [review]
implement a simple parser instead of just a "do"

As I understood our conversation on IRC, we're forgoing the use of Config::General for M1, but will (are likely?) to use it for M2, or possibly use a database.

As such, I thought the config stuff would just be commented out, not removed.

Given that, it may make sense to come up with a standard representation of the config that the system expects, and encapsulate all of the config logic into one place that either uses Config::General, does parsing, or talks to a DB, and then returns that representation.

I suppose Config sort of does this, but the parsing could be factored out into a cleaner method.

Also, a Singleton may make sense for the "static config object."

Finally, if I'm reading this correctly, for every call to Config() for a single variable, you open the config file, parse it, throw all those results away, and return the single variable you parsed.

There are (as of now) 62 calls to Config() in the system, so you might want to cache those results of the parsing (or, at *very* least, stop parsing and return when you find the variable you want.)

Comment 37

[Robert Helmer [:rhelmer]]

(In reply to comment #36)
> (From update of attachment 246232 [details] [diff] [review] [edit])
> As I understood our conversation on IRC, we're forgoing the use of
> Config::General for M1, but will (are likely?) to use it for M2, or possibly
> use a database.
> 
> As such, I thought the config stuff would just be commented out, not removed.
> 
> Given that, it may make sense to come up with a standard representation of the
> config that the system expects, and encapsulate all of the config logic into
> one place that either uses Config::General, does parsing, or talks to a DB, and
> then returns that representation.
> 
> I suppose Config sort of does this, but the parsing could be factored out into
> a cleaner method.
> 
> Also, a Singleton may make sense for the "static config object."
> 
> Finally, if I'm reading this correctly, for every call to Config() for a single
> variable, you open the config file, parse it, throw all those results away, and
> return the single variable you parsed.
> 
> There are (as of now) 62 calls to Config() in the system, so you might want to
> cache those results of the parsing (or, at *very* least, stop parsing and
> return when you find the variable you want.)


Good point... it's probably time for a Config class which represents a singleton.

Comment 38

[Robert Helmer [:rhelmer]]

Attachment: config class

Here is a Config singleton class. There are (of course!) a billion ways to do this with Perl, this seems like the most readable to me.

The parser implementation is the same, let me know if this looks ok to go in. I am thinking of removing the Config method from Bootstrap/Step.pm and having each Step  class instantiate and use Config directly in a separate patch, let me know if you have thoughts on that.

Comment 39

[J. Paul Reed [:preed]]

Comment on attachment 246815 [details] [diff] [review]
config class

I would make bootstrap.cfg a constant, and then variablize its use in the function, so you can easily point it at other .cfg files in other ways later if you want.

But that's pretty much a nit; r=preed.