Closed Bug 397554 Opened 12 years ago Closed 10 years ago

bootstrap should checkout/update tinderbox

Categories

(Release Engineering :: General, defect, P3, major)

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: rhelmer, Assigned: nthomas)

References

Details

Attachments

(2 files, 11 obsolete files)

17.15 KB, patch
Details | Diff | Splinter Review
17.35 KB, patch
rhelmer
: review+
Details | Diff | Splinter Review
The bootstrap Build and Repack steps depend on Tinderbox. They should make sure that these installs are up to date.

Additionally, I think it'd be very useful if these steps could do the basic Tinderbox install, if it does not exist:

* create /builds/tinderbox
* check out mozilla/tools/tinderbox
* create builddir e.g. Fx-Mozilla1.8-Release/ directory
* run install-links on directory
* check out and symlink from tinderbox-configs/ directory
Blocks: end2end-bld
Group: mozillaorgconfidential
Group: mozillaorgconfidential
Attached patch rough first shot (obsolete) — Splinter Review
This is kind of a brute force way to do this :)

I'm thinking about landing it this way, and then trying to refactor Build.pm and Repack.pm into some common methods (also make buildRoot and buildDir more palatable). 

Or I can do that first and land it as one giant patch, what do you think Nick?
Assignee: build → rhelmer
Status: NEW → ASSIGNED
Attachment #285183 - Flags: review?(nrthomas)
Severity: normal → enhancement
Comment on attachment 285183 [details] [diff] [review]
rough first shot

This looks pretty good, although there are enough comments to put r- and ask to see it again. I think we want some parts asap (eg updating tinderbox code), while other bits are the enhancement severity you've set (tbox setup, forcing configs to the release tag). Pretty happy to land now and do the refactoring when we pick up support for Free Software (and maybe Debug) builds - it's not like there's no duplication between Build & Repack at the moment ;-).

>Index: Bootstrap/Step/Build.pm
>===================================================================

>         $this->Shell(
>           cmd => 'mount',
>           cmdArgs => ['-b', '-sc', '/cygdrive'],
>-          dir => $buildDir,
>+          dir => $logDir,

What does this change do ? Were we getting default.log's in the $buildDir ?

>+    # run tinderbox
>+    $this->Shell(
>       cmd => './build-seamonkey.pl',
>       cmdArgs => ['--once', '--mozconfig', 'mozconfig', '--depend', 
>                   '--config-cvsup-dir', 
>                   catfile($buildDir, 'tinderbox-configs')],

Could trim the cvsup-dir & argument from here, no need to update twice. Also in Repack.

>Index: Bootstrap/Step/Repack.pm
>===================================================================
...
>+    my $releaseTag = $productTag . '_RELEASE';

Need to append "_l10n" to this tag.

>+    $this->Shell(
>+      cmd => 'cvs',
>+      cmdArgs => ['-d', $mozillaCvsroot,
>+                  'co', 'mozilla/tools/tinderbox'],
>+      dir => $l10n_buildRoot,
>+      logFile => catfile($logDir, 'build_tinderbox-checkout.log'),
>+      timeout => 360
>+    );

Do you also worry that we could end up using different tinderbox code for Build and Repack ? Perhaps we can use pullDate here, although this may overload that pref in restrictive ways.

Also need to use unique logfile names from the ones in Build, here and below.

>+    # create tinderbox buildDir if it does not exist, and install tinderbox
>+    if (! -d $l10n_buildDir) {
>+        MkdirWithPath(dir => $l10n_buildDir) 
>+            or die("Cannot mkdir $l10n_buildDir: $!");
>+        $this->Shell(
>+          cmd => catfile($l10n_buildRoot,
>+                         'mozilla', 'tools', 'tinderbox', 'install-links'),
>+          cmdArgs => ['.'],
>+          dir => $l10n_buildDir,
>+          logFile => catfile($logDir, 'build_tinderbox-create.log'),
>+          timeout => 360
>         );

Nit: the . argument isn't used, install-links assumes the dir structure we always use.

>+        # XXX l10n only hack - symlink build-firefox.pl to build-seamonkey.pl
>+        unlink(catfile($l10n_buildDir,'build-seamonkey.pl'))
>+            or die("Could not unlink build-seamonkey.pl: $!");
>+        symlink(catfile($l10n_buildRoot,'mozilla','tools','tinderbox',
>+                        'build-firefox.pl'), 
>+                catfile($l10n_buildDir, 'build-seamonkey.pl'))
>+            or die("Could not symlink build-firefox.pl: $!");
>+    }

Interesting, I hadn't noticed this before. Is it related to the tag problem above ? Any idea why it's l10n only ?
Attachment #285183 - Flags: review?(nrthomas) → review-
QA Contact: mozpreed → build
Great catches thanks! Answered your questions below:

(In reply to comment #2)
> (From update of attachment 285183 [details] [diff] [review])
> >Index: Bootstrap/Step/Build.pm
> >===================================================================
> 
> >         $this->Shell(
> >           cmd => 'mount',
> >           cmdArgs => ['-b', '-sc', '/cygdrive'],
> >-          dir => $buildDir,
> >+          dir => $logDir,
> 
> What does this change do ? Were we getting default.log's in the $buildDir ?

We need to specify a dir to run in that already exists; maybe we should make Shell default to ~/ or /tmp or something?

> >Index: Bootstrap/Step/Repack.pm
> >===================================================================
> ...
> >+    my $releaseTag = $productTag . '_RELEASE';
> 
> Need to append "_l10n" to this tag.
> 
> >+    $this->Shell(
> >+      cmd => 'cvs',
> >+      cmdArgs => ['-d', $mozillaCvsroot,
> >+                  'co', 'mozilla/tools/tinderbox'],
> >+      dir => $l10n_buildRoot,
> >+      logFile => catfile($logDir, 'build_tinderbox-checkout.log'),
> >+      timeout => 360
> >+    );
> 
> Do you also worry that we could end up using different tinderbox code for Build
> and Repack ? Perhaps we can use pullDate here, although this may overload that
> pref in restrictive ways.


If we're going to do anything I'd say pull by tag. I don't think we need to worry about separate l10n/en-US tinderbox versions, I'd like to avoid that. 

 
> >+        # XXX l10n only hack - symlink build-firefox.pl to build-seamonkey.pl
> >+        unlink(catfile($l10n_buildDir,'build-seamonkey.pl'))
> >+            or die("Could not unlink build-seamonkey.pl: $!");
> >+        symlink(catfile($l10n_buildRoot,'mozilla','tools','tinderbox',
> >+                        'build-firefox.pl'), 
> >+                catfile($l10n_buildDir, 'build-seamonkey.pl'))
> >+            or die("Could not symlink build-firefox.pl: $!");
> >+    }
> 
> Interesting, I hadn't noticed this before. Is it related to the tag problem
> above ? Any idea why it's l10n only ?


This is how the 1.8 branch tinderboxes are configured; it doesn't work with build-seamonkey.pl only build-firefox.pl (I forget the exact error but I am sure I can reproduce it).
Blocks: 401936
No longer blocks: end2end-bld
This is a bigger deal now that we're working on moving nightlies over to release automation, bumping severity.
Severity: enhancement → major
(In reply to comment #2)
> (From update of attachment 285183 [details] [diff] [review])
> >+    # run tinderbox
> >+    $this->Shell(
> >       cmd => './build-seamonkey.pl',
> >       cmdArgs => ['--once', '--mozconfig', 'mozconfig', '--depend', 
> >                   '--config-cvsup-dir', 
> >                   catfile($buildDir, 'tinderbox-configs')],

Doesn't CLOBBER-file support depend on this? We could make the checkout only happen if the dir or files don't exist.. I could go either way here.
Attached patch address nick's review comments (obsolete) — Splinter Review
Untested, just brought this patch up to date. Note that I added nightly support to the Build.pm patch, but Repack.pm doesn't have nightly support so skipped it there (will add nightly Repack support in a separate bug).

Fixed:

* do not use $logDir as temporary cwd; just omit "dir" arg 
* append _l10n to $releaseTag
* use unique logfile for l10n
* install-links nit

Outstanding:

* should we pull tinderbox from tag? what tag should that be for nightlies and for releases?
* does CLOBBER file support require config-cvsup-dir? if so, should checkout of tinderbox-configs only happen once per install, and let config-cvsup-dir handle updates from there?
* why do the 1.8 l10n tinderboxes symlink build-firefox.pl to build-seamonkey.pl? is this required, does it work for other branches, etc?

Nick, does this look like a reasonable summary? Do you think we should block initial landing of this patch on some or all of the outstanding issues? I'm thinking yes on first, and we need to understand the third..
Attachment #285183 - Attachment is obsolete: true
Attachment #297318 - Flags: review?(nrthomas)
Whiteboard: waiting for review
Comment on attachment 297318 [details] [diff] [review]
address nick's review comments

(In reply to comment #6)
> Outstanding:
> 
> * should we pull tinderbox from tag? what tag should that be for nightlies and
> for releases?

I'm kinda torn about the Tinderbox tag issue. In principle, it's a good idea to be working from a tag for the releases, and probably also use that for nightly builds too, so that commits aren't able to break the whole farm. On the other hand, we want to make Tinderbox go away, and there are sufficiently few changes to its code to make the setup time and additional overhead questionable.

How about this - we're probably stuck with Tinderbox for the support period of Fx2, but what about Fx3 ? ie how soon could we be calling make targets directly from Bootstrap/Buildbot ?

> * does CLOBBER file support require config-cvsup-dir? if so, should checkout of
> tinderbox-configs only happen once per install, and let config-cvsup-dir handle
> updates from there?

Yes, it does look that way. The "# update tinderbox config files" section should probably become "# checkout tinderbox config files" and move inside the setup-from-scratch if.

> * why do the 1.8 l10n tinderboxes symlink build-firefox.pl to
> build-seamonkey.pl? is this required, does it work for other branches, etc?

I didn't do this for the trunk boxes I set up, as an experiment, and found it wasn't required. Doing a diff, it looks like build-firefox.pl doesn't support --interval, and sets 
  $TreeSpecific::extrafiles = 'mozilla/browser/config';
Both the trunk and 1.8 seem to have BOOTSTRAP_* variables in client.mk, so I strongly suspect that build-firefox.pl is no longer required. Also, it hasn't been changed since the initial commit in 2004 ;-)

> Nick, does this look like a reasonable summary? Do you think we should block
> initial landing of this patch on some or all of the outstanding issues? I'm
> thinking yes on first, and we need to understand the third..

Seems like a good summary to me, but I context switched into this from cold :-) The patch looks fine from a quick look, but I didn't go nuts given the changes you probably need to make for the 2nd and 3rd issues. I did notice that $l10n_buildDir isn't defined in Repack. If we can come to a consensus on the tag issue then lets get this in.
Attachment #297318 - Flags: review?(nrthomas)
Whiteboard: waiting for review
(In reply to comment #7)
> (From update of attachment 297318 [details] [diff] [review])
> (In reply to comment #6)
> > Outstanding:
> > 
> > * should we pull tinderbox from tag? what tag should that be for nightlies and
> > for releases?
> 
> I'm kinda torn about the Tinderbox tag issue. In principle, it's a good idea to
> be working from a tag for the releases, and probably also use that for nightly
> builds too, so that commits aren't able to break the whole farm. On the other
> hand, we want to make Tinderbox go away, and there are sufficiently few changes
> to its code to make the setup time and additional overhead questionable.
> 
> How about this - we're probably stuck with Tinderbox for the support period of
> Fx2, but what about Fx3 ? ie how soon could we be calling make targets directly
> from Bootstrap/Buildbot ?


Hm, we're pretty close to releasing Fx3, at this point I'm hoping to do better on Moz2 and if possible backport, but we need to keep Fx3 and Fx2 shippable so I'm assuming we're stuck with Tinderbox for a while yet.


> > * does CLOBBER file support require config-cvsup-dir? if so, should checkout of
> > tinderbox-configs only happen once per install, and let config-cvsup-dir handle
> > updates from there?
> 
> Yes, it does look that way. The "# update tinderbox config files" section
> should probably become "# checkout tinderbox config files" and move inside the
> setup-from-scratch if.


Ok will do.


> > * why do the 1.8 l10n tinderboxes symlink build-firefox.pl to
> > build-seamonkey.pl? is this required, does it work for other branches, etc?
> 
> I didn't do this for the trunk boxes I set up, as an experiment, and found it
> wasn't required. Doing a diff, it looks like build-firefox.pl doesn't support
> --interval, and sets 
>   $TreeSpecific::extrafiles = 'mozilla/browser/config';
> Both the trunk and 1.8 seem to have BOOTSTRAP_* variables in client.mk, so I
> strongly suspect that build-firefox.pl is no longer required. Also, it hasn't
> been changed since the initial commit in 2004 ;-)


Oh, you know what, I think that extrafiles is the problem - we can fix this as a mozconfig setting, that's probably the whole problem here. Would be great to get rid of this weirdness, thanks for investigating that!


> > Nick, does this look like a reasonable summary? Do you think we should block
> > initial landing of this patch on some or all of the outstanding issues? I'm
> > thinking yes on first, and we need to understand the third..
> 
> Seems like a good summary to me, but I context switched into this from cold :-)
> The patch looks fine from a quick look, but I didn't go nuts given the changes
> you probably need to make for the 2nd and 3rd issues. I did notice that
> $l10n_buildDir isn't defined in Repack. If we can come to a consensus on the
> tag issue then lets get this in.
 

Thanks for the responses, this is a lot clearer to me now. I'm also torn on the tagging, because it's not really a regression from our current state, but it's also arguably just a bad thing to do not to track our tools.

The real fix is to just start tagging Tinderbox to go along with specific releases. I don't think we can pin it to the product release tag (e.g. FIREFOX_2_0_0_x_RELEASE) unfortunately, because we only maintain it on trunk and not branches.

We could start tagging it along with the release automation tag that's applied to mozilla/tools/release, what do you think?

I think for nightly mode we should just pull trunk, ideally whatever timestamp is used for the build (that's not really doable just yet unfortunately because that's determined inside tinderbox). For now I'd be ok with nightlies just pulling the tip of the trunk, as that's what we do for nightly builders right now.
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 297318 [details] [diff] [review] [details])
> > (In reply to comment #6)
> > > Outstanding:
[snip]
> > How about this - we're probably stuck with Tinderbox for the support period of
> > Fx2, but what about Fx3 ? ie how soon could we be calling make targets directly
> > from Bootstrap/Buildbot ?
> 
> Hm, we're pretty close to releasing Fx3, at this point I'm hoping to do better
> on Moz2 and if possible backport, but we need to keep Fx3 and Fx2 shippable so
> I'm assuming we're stuck with Tinderbox for a while yet.

Rob & I talked about this over lunch. Calling Make targets directly from Bootstrap, or even better directly from Buildbot, is something I'd still like to do before the FF3 release, if at all possible. Rob's point about destabilizing risk to FF3 is valid, and we do have to be careful with that, but we have to be even *more* careful after we ship. Also, this would simplify our Moz2 rollout. 

A big part of this is getting nightlies moved over first, imho. The staging framework gives us a sanity check that we havent broken anything, but nightly & depend builds gets many many more eyeballs looking any problems, and would help me sleep better at night.

(Didnt want to derail this bug about checking out tagged version of tinderbox, but this specific point felt important. How about we continue this discussion in person or in bug#299909?)
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (From update of attachment 297318 [details] [diff] [review] [details] [details])
> > > (In reply to comment #6)
> > > > Outstanding:
> [snip]
> > > How about this - we're probably stuck with Tinderbox for the support period of
> > > Fx2, but what about Fx3 ? ie how soon could we be calling make targets directly
> > > from Bootstrap/Buildbot ?
> > 
> > Hm, we're pretty close to releasing Fx3, at this point I'm hoping to do better
> > on Moz2 and if possible backport, but we need to keep Fx3 and Fx2 shippable so
> > I'm assuming we're stuck with Tinderbox for a while yet.
> 
> Rob & I talked about this over lunch. Calling Make targets directly from
> Bootstrap, or even better directly from Buildbot, is something I'd still like
> to do before the FF3 release, if at all possible. Rob's point about
> destabilizing risk to FF3 is valid, and we do have to be careful with that, but
> we have to be even *more* careful after we ship. Also, this would simplify our
> Moz2 rollout. 
> 
> A big part of this is getting nightlies moved over first, imho. The staging
> framework gives us a sanity check that we havent broken anything, but nightly &
> depend builds gets many many more eyeballs looking any problems, and would help
> me sleep better at night.
> 
> (Didnt want to derail this bug about checking out tagged version of tinderbox,
> but this specific point felt important. How about we continue this discussion
> in person or in bug#299909?)


Let's discuss this elsewhere, and do a schedule for moving away from Tinderbox based on the information we have. I'm willing to bet that we're not going to get nightlies and releases off of Tinderbox before Fx3 with all of the other things on our plate, but that's just a SWAG.

I think we should land this bug regardless, because it definitely makes what we have now better.
Blocks: 409880
(In reply to comment #8)
> The real fix is to just start tagging Tinderbox to go along with specific
> releases. I don't think we can pin it to the product release tag (e.g.
> FIREFOX_2_0_0_x_RELEASE) unfortunately, because we only maintain it on trunk
> and not branches.
> 
> We could start tagging it along with the release automation tag that's applied
> to mozilla/tools/release, what do you think?

Tagging with the automation tag would be a good improvement on what we do now, and Tinderbox logically goes with Bootstrap. Looks like we started doing this already - RELEASE_AUTOMATION_M5_3, ...M5_4, and ...M6_1 tags exist - but we'd need to pull from that too. To cope with nightly and release builds on the same box, a couple of options come to mind:

 - keep switching the tag on the existing checkout (need to add automation tag to config, use -A for version == nightly, otherwise -r <thetag>)

 - have separate checkouts for nightly and release builds, teach the setup code to deal with that, and manually apply these changes to existing boxes (or blow are existing build dirs)

The first one makes local patches difficult, but we don't like them anyways. The second one feels better to me, but is a bit more work. What say you ?

> I think for nightly mode we should just pull trunk, ideally whatever timestamp
> is used for the build (that's not really doable just yet unfortunately because
> that's determined inside tinderbox). For now I'd be ok with nightlies just
> pulling the tip of the trunk, as that's what we do for nightly builders right
> now.

We'd be testing the same code used for the release, so this sounds fine to me.
(In reply to comment #11)
> (In reply to comment #8)
> > The real fix is to just start tagging Tinderbox to go along with specific
> > releases. I don't think we can pin it to the product release tag (e.g.
> > FIREFOX_2_0_0_x_RELEASE) unfortunately, because we only maintain it on trunk
> > and not branches.
> > 
> > We could start tagging it along with the release automation tag that's applied
> > to mozilla/tools/release, what do you think?
> 
> Tagging with the automation tag would be a good improvement on what we do now,
> and Tinderbox logically goes with Bootstrap. Looks like we started doing this
> already - RELEASE_AUTOMATION_M5_3, ...M5_4, and ...M6_1 tags exist - but we'd
> need to pull from that too. To cope with nightly and release builds on the same
> box, a couple of options come to mind:
> 
>  - keep switching the tag on the existing checkout (need to add automation tag
> to config, use -A for version == nightly, otherwise -r <thetag>)
> 
>  - have separate checkouts for nightly and release builds, teach the setup code
> to deal with that, and manually apply these changes to existing boxes (or blow
> are existing build dirs)
> 
> The first one makes local patches difficult, but we don't like them anyways.
> The second one feels better to me, but is a bit more work. What say you ?


Hmm yeah I see your point. I don't think it would actually be any more work though; wouldn't we just change the builddir in the nightly bootstrap.cfg, and it would take care of the rest?

We do have the various incarnations of "/builds/tinderbox/" in the nightly l10n configs, but we can ensure these are correct by using the TinderConfig bumper.

I'll work on a patch that just does "-A" for nightly mode and "-r RELEASE_AUTOMATION_WHATEVER" otherwise, and the decision between sharing the /builds/tinderbox/mozilla checkout or not becomes a config issue.

Also, only checkout tinder-configs area if it does not exist; otherwise let build-seamonkey do it. I'll fix the l10n mozconfigs so the build-firefox.pl hack is no longer needed.

Should that cover everything?
Man the duplication in here was driving me batty, so I made a Bootstrap::Tinderbox class to centralize a lot of this.

I think this covers everything we discussed, and should hopefully be easier to read and understand. I just made Install/Update/Clobber/Run methods, but I'll bet we could have most if not all of Push, Announce and StoreBuildID in there too.

I disabled the "build-firefox.pl" l10n hack, so we'll have to land the mozconfig change at the same time, but I think it'll be an improvement just to remove the duplication in here if nothing else.
Attachment #297318 - Attachment is obsolete: true
Attachment #298594 - Flags: review?(nrthomas)
Whiteboard: waiting for review
Priority: P3 → P2
Comment on attachment 298594 [details] [diff] [review]
create separate Tinderbox class for install/update/build

>Index: Bootstrap/Tinderbox.pm
>===================================================================
...
>+sub new {
...
>+    my $this->{'step'} = args{'step'};

Install() wants a bootstrapVersion, but none is set here, or passed in by the instantiation calls.

>+sub Install {
...
>+        $this->{'step'}->CvsCo(cvsroot => $this->{'mozillaCvsroot'},
>+            modules => [CvsCatfile('mozilla', 'tools', 'tinderbox-configs',
>+                                   $this->{'product'}, $this->{'osname'})],
>+            tag => $tinderConfigVersion,
>+            workDir => $this->{'buildDir'},

Think you need a
   checkoutDir => 'tinderbox-configs',
here.

>+sub Update {
>+    my $this = shift;
>+
>+    my $tinderboxVersion = $this->{'bootstrapVersion'};
>+    if ($this->{'version'} eq 'nightly') {
>+        $tinderboxVersion = 'HEAD';
>+    }
>+
>+    $this->Shell(
>+      cmd => 'cvs',
>+      cmdArgs => ['-d', $this->{'mozillaCvsroot'},
>+                  'up', $tinderboxVersion],

Missing '-r' before $tinderboxVersion. Consider -Pd arguments too.

>+sub Clobber {
>+    my $this = shift;
>+
>+    # remove last-built file, which forces a clobber
>+    my $lastBuilt = catfile($this->{'buildDir'}, $this->{'buildPlatform'}, 
>+                            'last-built');
>+    if (! unlink($lastBuilt)) {
>+        $this->{'step'}->Log(msg => "Cannot unlink last-built file $lastBuilt: $!");
>+    } else {
>+        $this->{'step'}>Log(msg => "Unlinked $lastBuilt");
>+    }

Urgh, I know this is old code, but its kinda sucky. Wouldn't it be better to test for the existence of the $lastBuilt, then try to remove it, and die on an error ? As it is, the "Cannot ..." message will be given after a fresh setup of tinderbox.

>+sub Run {

You've got calls to Build() in the Build.pm & Repack.pm.

>+    my $this = shift;
>+
>+    my $buildLog = $args{'buildLog'};

Need to test that this mandatory argument is passed in ?

>Index: Bootstrap/Step/Build.pm
>===================================================================
...
>+    my $tbox = new Bootstrap::Tinderbox(
>+      version => $version,
>+      buildDir => $buildDir,
>+      buildRoot => $buildRoot,
>+      buildPlatform => $buildPlatform,
>+      productTag => $productTag,
>+      branchTag => $branchTag,
>+      osname => $osname,

osname is undefined.

>Index: Bootstrap/Step/Repack.pm
>===================================================================
>+    my $tbox = new Bootstrap::Tinderbox(
>+      version => $version,
>+      buildDir => $buildDir,
>+      buildRoot => $buildRoot,
>+      buildPlatform => $buildPlatform,

Need a l10n_ prefix on the right hand side for these last three vars.

>+      productTag => $productTag,
>+      branchTag => $branchTag,
>+      osname => $osname,

$osname is undefined.
Attachment #298594 - Flags: review?(nrthomas) → review-
(In reply to comment #12)
> Hmm yeah I see your point. I don't think it would actually be any more work
> though; wouldn't we just change the builddir in the nightly bootstrap.cfg, and
> it would take care of the rest?

Do you mean buildRoot ? Or perhaps you could expand on what sort of change buildDir would have.
 
Whiteboard: waiting for review
Attached patch take 4 (obsolete) — Splinter Review
* get bootstrapVersion from config
* assert that we have all required args, in new() and Build()
* checkoutDir for tinderbox-configs in Install()
* check out tinderbox-configs before symlinking from it
* make Update() do "-r TAG -dP" in release mode, "-A -dP" in nightly mode
* make Clobber() check for existence of last-built file 
* rename Run() to Build()
* define osname 
* use l10n_ prefixed args
Attachment #298594 - Attachment is obsolete: true
Attachment #298748 - Flags: review?(nrthomas)
(In reply to comment #15)
> (In reply to comment #12)
> > Hmm yeah I see your point. I don't think it would actually be any more work
> > though; wouldn't we just change the builddir in the nightly bootstrap.cfg, and
> > it would take care of the rest?
> 
> Do you mean buildRoot ? Or perhaps you could expand on what sort of change
> buildDir would have.

I did mean buildRoot.

Comment on attachment 298748 [details] [diff] [review]
take 4

>Index: Bootstrap/Tinderbox.pm
>===================================================================
...
>+sub new {
...
>+    my $this->{'bootstrapVersion'} = $args{'bootstrapVersion'};

Since this is really a tag, bootstrapTag would be better. Or even automationTag, since this is now Bootstrap and Tinderbox code. Open to suggestions.

...
>+    } elsif (not defined($this->{'buildDir'})) {
>+        die("ASSERT: Bootstrap::Tinderbox(): buildRoot is required");

need 'buildRoot' in the test.

>+    } elsif (not defined($this->{'buildRoot'})) {
>+        die("ASSERT: Bootstrap::Tinderbox(): buildPlatform is required");

need 'buildPlatform' in the test.

>+    } elsif (not defined($this->{'buildPlatform'})) {
>+        die("ASSERT: Bootstrap::Tinderbox(): productTag is required");

These two lines can go away.

>+sub Install {
...
>+        $this->CvsCo(cvsroot => $this->{'mozillaCvsroot'},
>+            modules => [CvsCatfile('mozilla', 'tools', 'tinderbox')],
>+            tag => $tinderboxVersion,
>+            workDir => $this->{'buildRoot'},
>+        );

I meant to ask last time - do we need to check anything else out for tests ? Mostly for nightlies I guess.

>+    # create tinderbox buildDir if it does not exist, and install tinderbox
...
>+        # check out tinderbox-configs dir if it does not exist
>+        # build-seamonkey.pl will keep it up to date
>+        if (! -d catfile($this->{'buildDir'}, 'tinderbox-configs')) {
>+            my $tinderConfigVersion = $this->{'releaseTag'};
>+            if ($this->{'version'} eq 'nightly') {
>+                $tinderConfigVersion = $this->{'branchTag'};
>+            }

Houston, we have problems:
* releaseTag isn't defined here
* we normally checkout the configs on their branch and leave Tinderbox to pull the tip of that

For the latter, I guess the newly minted TinderConfig::DetermineBranches can help. I wondered about the release builds pulling from the release tag rather than a branch (ie productTag . 'RELEASE' or 'RELEASE_l10n'), but I don't think that actually protects against accidental/rogue changes.

>+sub Clobber {
>+    my $this = shift;
>+
>+    # remove last-built file, which forces a clobber
>+    my $lastBuilt = catfile($this->{'buildDir'}, $this->{'buildPlatform'}, 
>+                            'last-built');
>+    if (-e $lastBuild) {
>+        if (! unlink($lastBuilt)) {
>+            die("Cannot unlink last-build file $lastBuild: $!");

s/Build/Built/g in the -e test and die comment.

>Index: Bootstrap/Step/Repack.pm
>===================================================================
...
>+    my $releaseTag = $productTag . '_RELEASE_l10n';

Not sure what this one is for, related to tinder-config checkout stuff I guess.
 
Not certain I got all 4 permutations straight here (release vs nightly, en-US vs l10n), but looks like we're converging on a solution. This comment's shorter than the previous one ;-)
Attachment #298748 - Flags: review?(nrthomas) → review-
(In reply to comment #18)

Thanks, I wonder if the variable test should just be a loop :P Fixed it as-is, anyway.


> >+sub Install {
> ...
> >+        $this->CvsCo(cvsroot => $this->{'mozillaCvsroot'},
> >+            modules => [CvsCatfile('mozilla', 'tools', 'tinderbox')],
> >+            tag => $tinderboxVersion,
> >+            workDir => $this->{'buildRoot'},
> >+        );
> 
> I meant to ask last time - do we need to check anything else out for tests ?
> Mostly for nightlies I guess.


Yeah this is a good point, let's tackle it as a followup patch (as it's an enhancement, I want to make sure we get the core stuff working right first..).


> >+    # create tinderbox buildDir if it does not exist, and install tinderbox
> ...
> >+        # check out tinderbox-configs dir if it does not exist
> >+        # build-seamonkey.pl will keep it up to date
> >+        if (! -d catfile($this->{'buildDir'}, 'tinderbox-configs')) {
> >+            my $tinderConfigVersion = $this->{'releaseTag'};
> >+            if ($this->{'version'} eq 'nightly') {
> >+                $tinderConfigVersion = $this->{'branchTag'};
> >+            }
> 
> Houston, we have problems:
> * releaseTag isn't defined here
> * we normally checkout the configs on their branch and leave Tinderbox to pull
> the tip of that
> 
> For the latter, I guess the newly minted TinderConfig::DetermineBranches can
> help. I wondered about the release builds pulling from the release tag rather
> than a branch (ie productTag . 'RELEASE' or 'RELEASE_l10n'), but I don't think
> that actually protects against accidental/rogue changes.


Hmm I don't think DetermineBranches will be very easy to work with for this.. my intention here was to pass releaseTag in and use it if not in nightly mode, otherwise use branch (and just remove 'HEAD_'). This requires that the l10n Build pass in $branchTag . '_l10n'.
Attached patch take 5 (obsolete) — Splinter Review
Attachment #298748 - Attachment is obsolete: true
Attachment #299244 - Flags: review?(nrthomas)
Whiteboard: waiting for review
Attached patch take 6 (obsolete) — Splinter Review
* can't use _RELEASE tags here, because we're depending on tinderbox to update this :/ just stick with branch for now, we now that the tip of the branch is tagged
* remove unused productTag/releaseTag from Bootstrap::Tinderbox
* standardize on bootstrapTag not bootstrapVersion
* make die() say "last-built" not "last-build"
Attachment #299244 - Attachment is obsolete: true
Attachment #299272 - Flags: review?(nrthomas)
Attachment #299244 - Flags: review?(nrthomas)
Comment on attachment 299272 [details] [diff] [review]
take 6

I started towards testing this, and found a few glitches:

* in bootstrap configs
** add "bootstrapTag    = RELEASE_AUTOMATION_M6_1" (the latest tag on tinderbox)
** add "buildRoot       = /builds/tinderbox"

* in Build.pm
** need "use Bootstrap::Tinderbox;" in the header
** "my $bootstrapTag = $config->Get(" instead of "my $bootstrapTag = $config->GetVersion("

* in TinderConfig.pm
** need "use MozBuild::Util qw(MkdirWithPath);"
** all the "my $this->{'version'} = $args{'version'};" lines need to lose the "my"
** need $this->{'buildDir'} instead of $buildDir in the MkDirWithPath call & die
** need to be able to call CvsCo, but it's not exported by Bootstrap::Step, and TinderConfig isn't a Step.
Attachment #299272 - Flags: review?(nrthomas)
Attached patch tested both Build and Repack (obsolete) — Splinter Review
Thanks for testing that Nick. I just went through a run of both Build and Repack, I think everything is working ok now.
Attachment #299272 - Attachment is obsolete: true
Attachment #299353 - Flags: review?(nrthomas)
Nick points out that the branches used for tinderbox-config checkouts are a bit more complicated than this; think I covered all the cases here, and changed the variable names to be a little clearer:

* en-US nightly - BRANCH
* en-US release - BRANCH_release
* l10n nightly - BRANCH_l10n
* l10n release - BRANCH_l10n_release

We set the branch to HEAD for trunk, but in reality the branches are "release", "l10n", and "l10n_release", so "HEAD_" is still chopped off before use in the tinder-config section of Install(). 

Tested on my local box, seems like it's doing the right thing on each step.
Attachment #299353 - Attachment is obsolete: true
Attachment #299514 - Flags: review?(nrthomas)
Attachment #299353 - Flags: review?(nrthomas)
Comment on attachment 299514 [details] [diff] [review]
use correct tinderconfig branches in nightly and release case

r+, with questions/suggestions/discussion inline.

>Index: Bootstrap/Tinderbox.pm
>===================================================================
...
>+sub Install {
>+    my $this = shift;
>+
>+    if (! -d $this->{'buildRoot'}) {

Perhaps this test should really be on buildRoot/mozilla/tools/tinderbox or buildRoot/mozilla. I hit this doing some testing, where I'd left an empty /builds/tinderbox/ and the tbox code wasn't checked out.

>+        $this->{'step'}->Shell(
>+          cmd => catfile($this->{'buildRoot'},
>+                         'mozilla', 'tools', 'tinderbox', 'install-links'),
>+          cmdArgs => [],
>+          dir => $this->{'buildDir'},
>+          timeout => 360
>+        );

FYI, install-links assumes that the buildDir/ and mozilla/ share the same parent directory, so if we have two tinderbox checkouts then the structure needs to be something like:
   /builds/tinderbox/nightly/mozilla/tools/tinderbox
                            /Fx-Mozilla1.8-Nightly
                            /Fx-Mozilla1.8-l10n-Nightly
   /builds/tinderbox/release/mozilla/tools/tinderbox
                            /Fx-Mozilla1.8-Release
                            /Fx-Mozilla1.8-l10n-Release

buildRoot, l10n_buildRoot, and all the buildDir definitions would need changes. Or maybe just the nightlies go elsewhere, as you alluded to in an earlier comment.

Actually, buildDir only seems to be used in Build & Repack (who'd have thunk it), so we could take this opportunity to avoid duplication between buildRoot and buildDir. Something like
   buildRoot       = /builds/tinderbox/release
   buildDir        = Fx-Mozilla1.8-Release
   <all the buildPlatform's>

and the full path is catfile($buildRoot,$buildDir,$buildPlatform), all looked up with Get(sysvar => ...). Hopefully the windows special cases won't be a pain.

>+        # check out tinderbox-configs dir if it does not exist
>+        # build-seamonkey.pl will keep it up to date
>+        if (! -d catfile($this->{'buildDir'}, 'tinderbox-configs')) {

Seems that this test is guaranteed to be true if buildDir didn't exist earlier. Perhaps it should be removed, or promoted to the level as the other dir tests to cope with errors on earlier runs, eg:

    if (! -d $this->{'buildRoot'}) {
       ...
    if (! -d $this->{'buildDir'}) {
       ...
    if (! -d catfile($this->{'buildDir'}, 'tinderbox-configs')) {
       ...

>+        symlink(catfile($this->{'buildDir'}, 'tinderbox-configs', 'mozconfig'), 
>+                catfile($this->{'buildDir'}, 'mozconfig'))
>+            or die("Could not symlink mozconfig: $!");
>+        symlink(catfile($this->{'buildDir'}, 'tinderbox-configs', 
>+                        'tinder-config.pl'), 
>+                catfile($this->{'buildDir'}, 'tinder-config.pl'))
>+            or die("Could not symlink tinder-config.pl: $!");

I double checked that perl won't barf on /cygdrive/ paths (win32/moz1.8). 

These symlinks are absolute, while we normally set them up as relative, so the (really really uncommon) operation of moving the build dir will break them. Just a note in passing.

>Index: Bootstrap/Step/Repack.pm
>===================================================================
...
>+    my $l10n_buildRoot = $config->Get(sysvar => 'l10n_buildRoot');

What's the story with this one ? Seems unlikely we'd want to put the l10n builds in a different place - is it for completeness or consistency with what we've done before ?
Attachment #299514 - Flags: review?(nrthomas) → review+
(In reply to comment #25)
> Actually, buildDir only seems to be used in Build & Repack (who'd have thunk
> it), so we could take this opportunity to avoid duplication between buildRoot
> and buildDir. Something like
>    buildRoot       = /builds/tinderbox/release
>    buildDir        = Fx-Mozilla1.8-Release
>    <all the buildPlatform's>

This would break TinderConfig, so perhaps it's not such a great idea.
Whiteboard: waiting for review → testing
(In reply to comment #25)
> (From update of attachment 299514 [details] [diff] [review])
> r+, with questions/suggestions/discussion inline.
> 
> >Index: Bootstrap/Tinderbox.pm
> >===================================================================
> ...
> >+sub Install {
> >+    my $this = shift;
> >+
> >+    if (! -d $this->{'buildRoot'}) {
> 
> Perhaps this test should really be on buildRoot/mozilla/tools/tinderbox or
> buildRoot/mozilla. I hit this doing some testing, where I'd left an empty
> /builds/tinderbox/ and the tbox code wasn't checked out.

Yeah I did too actually, I'll fix this before checkin to do:

     if (! -d catfile($this->{'buildRoot'}, 'mozilla', 'tools', 'tinderbox') {


> 
> >+        $this->{'step'}->Shell(
> >+          cmd => catfile($this->{'buildRoot'},
> >+                         'mozilla', 'tools', 'tinderbox', 'install-links'),
> >+          cmdArgs => [],
> >+          dir => $this->{'buildDir'},
> >+          timeout => 360
> >+        );
> 
> FYI, install-links assumes that the buildDir/ and mozilla/ share the same
> parent directory, so if we have two tinderbox checkouts then the structure
> needs to be something like:
>    /builds/tinderbox/nightly/mozilla/tools/tinderbox
>                             /Fx-Mozilla1.8-Nightly
>                             /Fx-Mozilla1.8-l10n-Nightly
>    /builds/tinderbox/release/mozilla/tools/tinderbox
>                             /Fx-Mozilla1.8-Release
>                             /Fx-Mozilla1.8-l10n-Release
> 
> buildRoot, l10n_buildRoot, and all the buildDir definitions would need changes.
> Or maybe just the nightlies go elsewhere, as you alluded to in an earlier
> comment.


I'll patch the configs and post them here for review before landing, so we can get it right and land everything at the same time. I'll do some local testing first.


> >Index: Bootstrap/Step/Repack.pm
> >===================================================================
> ...
> >+    my $l10n_buildRoot = $config->Get(sysvar => 'l10n_buildRoot');
> 
> What's the story with this one ? Seems unlikely we'd want to put the l10n
> builds in a different place - is it for completeness or consistency with what
> we've done before ?


Probably stems from Repack.pm just being a copy of Build.pm, I'll go ahead and make it buildRoot. We could do this for buildPlatform too if we're pretty sure we never want to have it on a machine with a different kernel than the main build, what do you think?
(In reply to comment #27)

Sounds good.

> Probably stems from Repack.pm just being a copy of Build.pm, I'll go ahead and
> make it buildRoot. We could do this for buildPlatform too if we're pretty sure
> we never want to have it on a machine with a different kernel than the main
> build, what do you think?

Ok. Lets hang on to buildPlatform at the moment, at least until we sort out automation for Tb2 (since that uses different machines for l10n and en-US, urgh).

Blocks: 417147
For the initial 1.8 branch rollout, maybe it'd just be simpler to leave things as-is and piggyback on the release automation tag?

I'm thinking staging should always use HEAD, and do the Tinderbox tag thing after all.. it would enable us to make way more radical changes without having to worry about breaking production (but staging would test HEAD so we'd need to keep it stable).

Sorry for the flip-flopping, what do you think about checking in the patch as-is? I'm testing it on all platforms now, it seems to do the right thing so far, and it's definitely better than the nothing we have now.
Whiteboard: testing → need msys fix/workaround
Hmm, we need to figure out the MSYS situation first :/ Apparently it's an nsinstall bug, I don't mind if we workaround or if we fix the root cause, but we can't land this without MSYS support.
(In reply to comment #30)
> Hmm, we need to figure out the MSYS situation first :/ Apparently it's an
> nsinstall bug, I don't mind if we workaround or if we fix the root cause, but
> we can't land this without MSYS support.

I commented in bug 396187 comment 18 about this. Since MSYS also doesn't support symlinks anyway, we need to special-case it, I guess.
Attached patch support MSYS (obsolete) — Splinter Review
Is this enough to support MSYS, if we set e.g.:

buildRoot = /e/builds/tinderbox/
buildDir  = /e/fx19rel # or whatever

I'll start testing, but am I missing anything fundamental here?
Attachment #299514 - Attachment is obsolete: true
Attachment #310289 - Flags: review?(nrthomas)
Attachment #310289 - Attachment is obsolete: true
Attachment #310291 - Flags: review?(nrthomas)
Attachment #310289 - Flags: review?(nrthomas)
Whiteboard: need msys fix/workaround → waiting for review
Comment on attachment 310291 [details] [diff] [review]
support MSYS (include tinderbox class)

>Index: Bootstrap/Tinderbox.pm
>+sub Install {
...
>+        $this->{'step'}->Shell(
>+          cmd => catfile($this->{'buildRoot'},
>+                         'mozilla', 'tools', 'tinderbox', 'install-links'),
>+          cmdArgs => [],
>+          dir => $this->{'buildDir'},
>+          timeout => 360
>+        );

install-links won't cope with the example buildRoot and buildDir you gave.

>+sub Update {
...
>+    # MSYS does not support symlinks, re-install after updating Tinderbox.
>+    if ($sysname =~ /mingw32/i) {
>+        $this->Install();
>+    }

This isn't going to do anything when the dirs already exist. All the checkout stuff in Install() shouldn't be inside the |if ! -d| blocks if you want to call it this way.

Why not get this tested in your own environment or staging ?
Attachment #310291 - Flags: review?(nrthomas) → review-
(In reply to comment #34)
> (From update of attachment 310291 [details] [diff] [review])
> Why not get this tested in your own environment or staging ?

Right, not as simple as I thought it was :) I'll get this tested then re-r?. Thanks! 

Whiteboard: waiting for review → testing in dev
(In reply to comment #34)
> (From update of attachment 310291 [details] [diff] [review])
> >Index: Bootstrap/Tinderbox.pm
> >+sub Install {
> ...
> >+        $this->{'step'}->Shell(
> >+          cmd => catfile($this->{'buildRoot'},
> >+                         'mozilla', 'tools', 'tinderbox', 'install-links'),
> >+          cmdArgs => [],
> >+          dir => $this->{'buildDir'},
> >+          timeout => 360
> >+        );
> 
> install-links won't cope with the example buildRoot and buildDir you gave.


Yeah, forgot about this... guess we could install-links to /e/builds/tinderbox/fx19rel and have a special MSYS-only copy to /e/fx19rel/ :/

> >+sub Update {
> ...
> >+    # MSYS does not support symlinks, re-install after updating Tinderbox.
> >+    if ($sysname =~ /mingw32/i) {
> >+        $this->Install();
> >+    }
> 
> This isn't going to do anything when the dirs already exist. All the checkout
> stuff in Install() shouldn't be inside the |if ! -d| blocks if you want to call
> it this way.


Yeah, I think we just need to special-case MSYS here instead.
I was hoping not to have to do this level of workaround, but this seems like the most direct way to do it. Can't generalize this, because it's a win32+msys specific problem.
Attachment #310291 - Attachment is obsolete: true
Attachment #311034 - Flags: review?(nrthomas)
Whiteboard: testing in dev → waiting for review
Here's another cut at supporting MSYS, based on attachment 311034 [details] [diff] [review]. Its claims to fame are:
* doesn't require another var, just stops using /e/builds/tinderbox/Fx-...
* when installing Tinderbox, do the copy when you are on MSYS, otherwise use install-links (would be fairly easy to fix install-links to not need this)
* in Tinderbox::Update for the MSYS case, copy the tinderbox code you just updated rather than a stale copy
* add buildRoot vars to the 1.8 staging config, revert msys vars in 1.9 config

Untested except perl -c. Any good ?
There's also the post-mozilla.pl issue on MSYS, since install-links symlinks from post-mozilla-rel.pl to post-mozilla.pl. 

I had a look on fx-{mac,linux}-1.9-slave2 and we have stale versions of post-mozilla-rel.pl _copied_ to post-mozilla.pl for the release builds. :-S
Support post-mozilla-rel by copying in the MSYS case.
Attachment #311800 - Attachment is obsolete: true
Attachment #311034 - Flags: review?(nrthomas)
Whiteboard: waiting for review → testing on staging
Blocks: 415180
Whiteboard: testing on staging → needs checkin
Comment on attachment 311803 [details] [diff] [review]
support MSYS - alternative edition with post-mozilla-rel

this patch looks good to me. Up to you if you want to land this or do more testing, etc. Nick.
Attachment #311803 - Flags: review+
Assignee: robert → nobody
Status: ASSIGNED → NEW
QA Contact: build → release
No longer blocks: 415180
Assignee: nobody → nrthomas
Priority: P2 → P3
We no longer need this as patches to tinderbox are pretty non-existent these days. My apologies for letting it sit for so long.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Whiteboard: needs checkin
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.