387970 - Release automation regressions/issues in _M5

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P2)

Description

[J. Paul Reed [:preed]]

We can split these out into separate bugs to track them, but I wanted to get these issues down while they were fresh in my head:

-- Bootstrap::Step::Shell() needs to use sysvar when Config::Get()ing logDir; if you don't, various steps refuse to run on Win32, because in bootstrap.cfg, we only have one logDir specified, and on Win32, the logDir path is different, due to cygwin. I tried to fix this by adding a win32_logDir, but that doesn't work because Shell() doesn't use sysvar.

-- The _M5 automation, out of the box, won't run on Win32, Mac, or most Linux installs; the PatcherConfig step now use()s Config::General which... hehe... installed on most machines, and surely isn't on Mac and Win32.

-- The TinderConfig step needs to be run for each platform; this isn't a huge deal, but it would be nice if we could run it once on the tagging machine (maybe optionally?) instead of doing it on each platform. That's a feature enhancement. The bug is...

-- The TinderConfig step doesn't tag (with -F, if rc > 1) the configs it bumps with the _RELEASE tag.

I may add more as I find them during the 2.0.0.5 release. We can track these whichever way is convenient to the team/whomever fixes these bugs.

Comment 1

[J. Paul Reed [:preed]]

Attachment: Fix the Config::General issue, take 1

Comment 2

[J. Paul Reed [:preed]]

Attachment: Fix the TinderConfig not-tagging issue, take 1

This is completely off the cuff... the only "testing" I did was "perl -c."

It's just a... suggestion, ya know ;-)

Comment 3

[J. Paul Reed [:preed]]

-- The Push step seemingly worked beautifully... but the directories got created with wonky permissions, so they weren't visible to users. Was this on purpose? (it's not a bad idea, actually...)

Either way, should have a step to clean up the perms of the files and the directories.

Comment 4

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

(In reply to comment #3)

That's a testing omission rather than on purpose. Not sure why all that sticky bit stuff on the staging server didn't take care of this, using "mkdir -p" perhaps.

Comment 5

[J. Paul Reed [:preed]]

-- You have to run TinderConfig again if you bump the RC. In retrospect, this makes perfect sense (the _RELEASE configs don't need any changes, but l10n does, so it can find the new builds), but I don't know/think it's documented anywhere. It would be great if the TinderConfig stuff was more transparent in terms of not having to figure out when to run it (i.e. it just automatically runs when it needs to, and tags as necessary).

Comment 6

[J. Paul Reed [:preed]]

-- The PatcherConfig step doesn't:

a. Update the from field in the <current-update> stanza, so you don't get current updates generated (you get them updated from R-2 -> R, instead of R-1 -> R); that's a bug.

b. Tag the patcher config; that's a nice feature.

Comment 7

[J. Paul Reed [:preed]]

Regarding comment #6, it looks like the "to" line in the same stanza doesn't get updated either.

Comment 8

[J. Paul Reed [:preed]]

-- If you have to run the TinderConfig step again (for another RC, or because you're building another product on the same machine), the checkouts will bump into each other, because they're all checked out in /builds/configs/tinderbox-configs.

They should be checked out into something like /builds/configs/$app-$version-$rc-tinderbox-configs, or some such.

Comment 9

[Robert Helmer [:rhelmer]]

The verification for the l10n Repack and Update steps is annoying:

* l10n verification must be run on a mac e.g. bm-xserve01, in "verify-only" mode
* update verification must be invoked on one of each: mac/linux/windows in "verify-only" mode
* update verification still requires it's own configuration in mozilla/testing/release/updates; this should be auto-generated (or just set the correct settings in the environment)

Update verification can be run in a quick "HTTP HEAD check-only" mode, doing "verify.sh -t". There's also a mar-check mode, "-m", that makes sure that the MARs match the size/checksum advertised in the update.xml

I tend to run these in my checkout when I generate patches to the config files (to make sure I got the configuration right), but they'd be useful to have in the automation proper, especially as a post-stage verification step.

Rewriting the update verification (bug 373995) needs to happen, but we should consider any smaller changes we can do to make this more automatic in the meantime.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> Update verification can be run in a quick "HTTP HEAD check-only" mode, doing
> "verify.sh -t". There's also a mar-check mode, "-m", that makes sure that the
> MARs match the size/checksum advertised in the update.xml
> 
> I tend to run these in my checkout when I generate patches to the config files
> (to make sure I got the configuration right), but they'd be useful to have in
> the automation proper, especially as a post-stage verification step.

Specifically, I look at the output of "-t" for any "404" HTTP result codes, and also for any "Content-Type" headers that are not "application/octet-stream". This test does NOT print "FAIL", it's purely informative.

The output of "-m" will contain FAIL if any MAR files do not match the advertised size/checksum in update.xml 

Note that these tests are a subset of the complete "-c" test that Bootstrap runs; you aren't missing anything by not running them. However, they tend to find simple AUS misconfiguration and other problems much earlier, because they are so much faster to run.

Comment 11

[Robert Helmer [:rhelmer]]

The PatcherConfig step did not generate a "past-release" line for 2.0.0.3, which means that users still on 2.0.0.3 would not have seen an offer directly to 2.0.0.5 (they would've had to jump 2.0.0.3->2.0.0.4->2.0.0.5).

Update verification currently would not have caught this (QA did this time), but we can catch this class of problems during the verification rewrite.  

I've put my thoughts on this in bug 373995 comment 7.

Comment 12

[J. Paul Reed [:preed]]

Oh snap!


log: Ending time is 15:10:22 07/17/07
log: Running shell command in /data/cltbld/firefox-2.0.0.5/batch1/stage-signed:
log:   arg0: /data/cltbld/bin/verify-locales.pl
log:   arg1: -m
log:   arg2: /data/cltbld/firefox-2.0.0.5/batch1/config/shipped-locales
log: Starting time is 15:10:22 07/17/07
log: Logging output to /data/cltbld/builds/release/logs/stage_verify_l10n.log
log: Timeout: 3600
Can't find locale manifest /data/cltbld/firefox-2.0.0.5/batch1/config/shipped-locales; bailing...
Failed to load locale manifest at /data/cltbld/bin/verify-locales.pl line 277.
log: output: Can't find locale manifest /data/cltbld/firefox-2.0.0.5/batch1/config/shipped-locales; bailing...
Failed to load locale manifest at /data/cltbld/bin/verify-locales.pl line 277.

Step Stage died: shell call returned bad exit code: 2 at Bootstrap/Step.pm line 99.


While running:

./release -o Stage 2>&1 | tee logs/release-Stage.log

Comment 13

[J. Paul Reed [:preed]]

The problem in comment 12 was the shipped-locales file gets used, but never gets checked out. "oops"

Also, I had to remove (empty) linux, windows, and mac -xpi directories in the mar directory after I was done.

Comment 14

[J. Paul Reed [:preed]]

For Thunderbird, at least, the automation *still* doesn't get directory and file permissions correct; I had to run the following (for ffx and tbird) in stage-merged:

chgrp -R thunderbird .
find -type d -not -regex '.*contrib.*' -exec chmod 0755 {} \;
find -type f -exec chmod 0644 {} \;

I thought I fixed this, but it may be getting mangled by all the rsyncs we do for signing purposes...

Comment 15

[J. Paul Reed [:preed]]

Attachment: Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util

This is admittedly an odd place for this patch, but it's really to address the not-tagging of the cvs configs in the TinderConfig step. We want to be able to tag certain things (in lots of places that aren't in Bootstrap::Step::Tag), and it was inconvenient to have CvsTag() in that class, so getting ahold of a handle to it was... a weird way to do it.

So this moves CvsTag() from Bootstrap::Step::Tag to the Bootstrap::Util class; I'll do some testing on this (perl -c was happy with it, but that doesn't really tell us much), but wanted to get it out for review.

I also moved GetDiffFileList(), which is a utility function that isn't really related to tagging either. ;-)

Comment 16

[J. Paul Reed [:preed]]

Attachment: Fix the candidates directory permission problem

This is kinda kludgy (mostly the chmodParentArg stuff), but it should fix the problem we're having with the modes on the directory creation.

Comment 17

[J. Paul Reed [:preed]]

Attachment: Bump patcher2.cfg to/from/past-update stanzas even if rc > 1

Address the PatcherConfig issues in comment 6 and 7; the problem was that we have code to do this, but it's triggered on rc = 1; problem is when we do a respin, we may hit updates at rc 2, 3, 4, etc.

This can be wrong if we didn't get to the updates step on rc1 (because we respun before that); so, we need to switch this onetime bumping code on/off based on whether or not it's already been run, which we can figure out by looking at the current config and comparing it to the version we're building in bootstrap.cfg.

Comment 18

[J. Paul Reed [:preed]]

Comment on attachment 276851 [details] [diff] [review]
Bump patcher2.cfg to/from/past-update stanzas even if rc > 1


> use Bootstrap::Step;
>-use Bootstrap::Config;

Ignore this diff chunk; that was a merge error on my part.

Comment 19

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

Comment on attachment 276838 [details] [diff] [review]
Fix the candidates directory permission problem

>+    my $chmodParentArg = '..';
>+    $chmodParentArg .= ' ../..' if ($candidateDir =~ /unsigned/);

You said you need to fix the candidates dir in 
  .../firefox/nightly/2.0.0.6-candidates/rc1/unsigned/
so that's ../.. on the GetFTPCandidateDir output in the 2.0.0.x case, otherwise it's just .. right ?

>+    $this->Shell(
>+      cmd => 'ssh',
>+      cmdArgs => ['-2', '-l', $sshUser, $sshServer,
>+                  'chmod 0755 ' . $candidateDir . ' ' . $chmodParentArg],

Use CvsCatfile here ?

Alternatively you can go (something like)
 my $chmodDir = CvsCatfile($config->GetFtpCandidateDir(bitsUnsigned => 0),'..');
since that never adds the unsigned/ suffix.

You'll also need to s/sshUser/stagingServer/g and s/sshUser/stagingUser/g since the changes in bug 391968 attachment 276817 [details] [diff] [review]

Comment 20

[J. Paul Reed [:preed]]

(In reply to comment #19)
> (From update of attachment 276838 [details] [diff] [review])
> >+    my $chmodParentArg = '..';
> >+    $chmodParentArg .= ' ../..' if ($candidateDir =~ /unsigned/);
> 
> You said you need to fix the candidates dir in 
>   .../firefox/nightly/2.0.0.6-candidates/rc1/unsigned/
> so that's ../.. on the GetFTPCandidateDir output in the 2.0.0.x case, otherwise
> it's just .. right ?

On stage, the 2.0.0.6-candidates directory has the wrong perms; the rc1 and the unsigned directory have correct permissions (due to the rsync). So, the logic is if the path contains an unsigned directory, then the full path is:

.../firefox/nightly/2.0.0.6-candidates/rc1/unsigned; since we really want the candidates directory, what we need is .../firefox/nightly/2.0.0.6-candidates/rc1/unsigned/../...

If we're not in the unsigned case, then we want 

firefox/nightly/2.0.0.6-candidates/rc1/..

But in the unsigned case, I just reset the permissions on all the directories, since I'm there; that's why it's .=, not =.

So, that code is wrong, but for different reasons; it should be:

my $chmodParentArg = "$candidateDir/..";
$chmodParentArg .= " $candidateDir/../.." if ($candidateDir =~ /unsigned/);

> >+    $this->Shell(
> >+      cmd => 'ssh',
> >+      cmdArgs => ['-2', '-l', $sshUser, $sshServer,
> >+                  'chmod 0755 ' . $candidateDir . ' ' . $chmodParentArg],
> 
> Use CvsCatfile here ?

I could. I didn't because really, we want a catfile() that gives Unix-style separators (which is what CvsCatfile() does, but only because we were "lucky"); I didn't use it because this isn't related to CVS really at all. Hrm...

> Alternatively you can go (something like)
>  my $chmodDir = CvsCatfile($config->GetFtpCandidateDir(bitsUnsigned =>
> 0),'..');
> since that never adds the unsigned/ suffix.

That's a good call; and now that I think about it, the mkdir and the chmod get run three times, for every platform's Push() call. Oh well.

> You'll also need to s/sshUser/stagingServer/g and s/sshUser/stagingUser/g since
> the changes in bug 391968 attachment 276817 [details] [diff] [review]

Thanks for the heads up; I'll fix that problem if CVS complains about up-to-date checks.

I'll actually post a new patch for these changes.

Comment 21

[J. Paul Reed [:preed]]

Attachment: Fix the candidates directory permission problem, Take II

A few changes that make this clearer; also, I removed the version of this code in the Repack step, because the directory only needs to be fixed once (that I know of), when it gets created. Technically, it really only needs to run for RC 1, but the way the automation creates these directories, it would be annoying to run this for only RC 1 (and it should be harmless to re-run it).

I also slipped in a Bootstrap::Config fix (when I was looking at refactoring GetFtpCandidateDir()) to make that function use the interface and not rely on the underlying implementation/data structure.

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 276714 [details] [diff] [review]
Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util

Looks good, please don't check mozilla/tools/release/t/tinder-config.pl in :) (i'll fix the test so that stops getting modified in-place)

Comment 23

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

Comment on attachment 276860 [details] [diff] [review]
Fix the candidates directory permission problem, Take II

r+, please take care with the sshUser & sshServer changes I mentioned last time.

Comment 24

[J. Paul Reed [:preed]]

(In reply to comment #23)
> (From update of attachment 276860 [details] [diff] [review])
> r+, please take care with the sshUser & sshServer changes I mentioned last
> time.

So I went ahead and checked in, and didn't get a warning on checkin of an up-to-date check, so I may have bit-rotted the patch in bug 391968, sorry about that rhelmer. (I couldn't have changed it, though, since those variables don't  exist in that function yet.)

Comment 25

[J. Paul Reed [:preed]]

Alright, just landed a bunch of stuff:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=&branchtype=match&dir=&file=&filetype=match&who=preed%mozilla.com&whotype=match&sortby=Date&hours=6&date=explicit&mindate=2007-08-15+16%3A45&maxdate=2007-08-15+17%3A30&cvsroot=%2Fcvsroot

Comment 26

[Robert Helmer [:rhelmer]]

Comment on attachment 276714 [details] [diff] [review]
Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util

>Index: Bootstrap/Step/Tag.pm
>===================================================================
>RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag.pm,v
>retrieving revision 1.11
>diff -u -8 -r1.11 Tag.pm
>--- Bootstrap/Step/Tag.pm	10 Aug 2007 23:30:01 -0000	1.11
>+++ Bootstrap/Step/Tag.pm	14 Aug 2007 23:43:54 -0000
>+    # We need to provide the fullpath to Bootstrap::Util::CvsTag() now.
>     my $config = new Bootstrap::Config();
>-    my $logDir = $config->Get(sysvar => 'logDir');
>+    $args{'logFile'} = catfile($config->Get(sysvar => 'logDir'), $logFile);


Sorry, not sure how I missed this before; but all of the current callers of CvsTag specify the absolute path, so you'll get e.g. "/builds/logs//builds/logs/${logfile}" in some cases now, which will fail.

Comment 27

[Robert Helmer [:rhelmer]]

Comment on attachment 276714 [details] [diff] [review]
Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util

>Index: Bootstrap/Step/Tag.pm
>===================================================================
>RCS file: /cvsroot/mozilla/tools/release/Bootstrap/Step/Tag.pm,v
>retrieving revision 1.11
>+    my $dumpedCore = $rv->{'dumpedCore'};
>+    if ($timedOut || ($exitValue != 0)) {
>+        $this->Log(msg => "Bootstrap::Step::Tag::CvsTag failed; rv was $exitValue, output: $rv->{'output'}");
>+        die("Bootstrap::Step::Tag::CvsTag: returned bad exit code: $exitValue");
>     }


BTW, this makes it hard to see timeouts :) ^

Comment 28

[Robert Helmer [:rhelmer]]

Comment on attachment 276860 [details] [diff] [review]
Fix the candidates directory permission problem, Take II

drive-by review - this patch doesn't work, in Bootstrap/Config.pm the arguments have to be a hash e.g.:

Get(var => 'product')

not:

Get('product')

Comment 29

[J. Paul Reed [:preed]]

(In reply to comment #28)
> (From update of attachment 276860 [details] [diff] [review])
> drive-by review - this patch doesn't work, in Bootstrap/Config.pm the arguments
> have to be a hash e.g.:
> 
> Get(var => 'product')
> 
> not:
> 
> Get('product')

Fixed.

Comment 30

[Robert Helmer [:rhelmer]]

Comment on attachment 276714 [details] [diff] [review]
Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util

I'm going to back this patch out; had a problem after I fixed the logging problem. Let's retest and try again.

Comment 31

[Robert Helmer [:rhelmer]]

Attachment: use CvsCatfile

Attachment 276860 [details] [diff] introduce CvsCatfile; we also need to "use" it. Going to go ahead and land this one.

Comment 32

[J. Paul Reed [:preed]]

So I went through this bug last night, and believe these are the remaining things left:

-- Re-land the move of CvsTag() from Bootstrap::Step::Tag to Bootstrap::Util

-- Tag the TinderConfig-bumped files, as necessary (which is dependent upon the above).

-- Make TinderConfig run when it should (which may mean refactoring out when/how TinderConfig is actually called); this is so that if an RC is bumped, then the TinderConfig steps get re-run, as necessary, without a human having to figure out where to restart the process. I've got a couple of ideas, but for _M6, maybe it'll be better to leave things as they are.

-- I took a stab at fixing the Config::General requirement here; I could either try again (I know it's possible), or punt on it altogether, since the wiki instructions for the build automation require Config::General to be installed on all machines (which I don't think is necessary/desired).

-- Investigate the permissions problem outlined in comment 14; I believe cf already fixed this (I seem to remember some patches running by that addressed permissions), but I'll make sure.

I think everything else I raised in comments 0, 3, 5, 6, 7, and 12 has been addressed. If not, please let me know.
 
Taking this bug, assigning it P2, since I'm actively working on it now, and I'll take a stab at this next week.

Comment 33

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

(In reply to comment #32)
> -- Investigate the permissions problem outlined in comment 14; I believe cf
> already fixed this (I seem to remember some patches running by that addressed
> permissions), but I'll make sure.

See bug 394500 where I did a bit of debuggging.
 

Comment 34

[J. Paul Reed [:preed]]

Attachment: no flags Details | Diff Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util, take II

You'll want to use interdiff for reviewing this patch.

This fixes the following issues with the first version of the patch:

-- Move the default timeout definition from Bootstrap::Step to Bootstrap::Util. The original version of this patch ended up leaving the timeout value undefined, so MozBuild::Util::RunShellCommand()'s default of 10 minutes took over; this isn't long enough for many of the tagging operations we perform, so it would actually time out.

-- The timeout was hard to spot because, as rhelmer pointed out, both the timeout and return value cases were handling in the same error branch, but timeout instances weren't specifically called out, making timeouts--which this was, fundamentally--hard to spot. Changed the error message to include the value of the timeout variable.

-- There were some CVS tagging steps that didn't define a logFile, so they got the default ("default.log"); this isn't horrible, but those commands should have their own logfiles (and there were a few instances of this, so default.log got appended to each time).

-- In the same vein, I found out that the "output" variable was defaulting to 0 in the Bootstrap::Util version of CvsTag. Bootstrap::Step::Shell queries the Bootstrap::Config object to see if it should dump the output of the tag in realtime, so we do that here, which required two changes: one to query the config object and set the proper value in the Bootstrap::Step::Tag::CvsTag() shim, and a change in Bootstrap::Util::CvsTag() to pick up that value, and use it.

-- Make Bootstrap::Step::Shell() use the exec timeout defined in Util.

I ran this through a testing run on my desktop, which just did the tagging for an RC1.

Comment 35

[Robert Helmer [:rhelmer]]

Comment on attachment 280676 [details] [diff] [review]
   	 no flags   	 Details  | Diff
Move CvsTag from Bootstrap::Step::Tag to Bootstrap::Util, take II

Looks ok to me. 

Why not switch all callers over to Bootstrap::Util::CvsTag() and remove Bootstrap::Step::Tag?

Let's go with this patch for now regardless.

Comment 36

[J. Paul Reed [:preed]]

(In reply to comment #35)
> (From update of attachment 280676 [details] [diff] [review])
> Looks ok to me. 
> 
> Why not switch all callers over to Bootstrap::Util::CvsTag() and remove
> Bootstrap::Step::Tag?

Just trying to simplify the change; it's not a bad idea to do that, though.

> Let's go with this patch for now regardless.

Landed:

Checking in Bootstrap/Step.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step.pm,v  <--  Step.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bootstrap/Util.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Util.pm,v  <--  Util.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bootstrap/Step/Tag.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Tag.pm,v  <--  Tag.pm
new revision: 1.14; previous revision: 1.13
done
Checking in Bootstrap/Step/Tag/l10n.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Tag/l10n.pm,v  <--  l10n.pm
new revision: 1.7; previous revision: 1.6
done

Comment 37

[J. Paul Reed [:preed]]

Attachment: Tag files that are modified by the TinderConfig step

This tags configs that are modified by the TinderConfig Bootstrap step, after they've been modified.

Comment 38

[J. Paul Reed [:preed]]

Comment on attachment 280687 [details] [diff] [review]
Tag files that are modified by the TinderConfig step

Oh, also: I tested this in the rc1 and rc2 cases, but running it basically twice, back to back (once with rc = 1, once with rc = 2).

Comment 39

[J. Paul Reed [:preed]]

(In reply to comment #35)
> (From update of attachment 280676 [details] [diff] [review])
> Looks ok to me. 
> 
> Why not switch all callers over to Bootstrap::Util::CvsTag() and remove
> Bootstrap::Step::Tag?

I finally remembered why I did this (specifically, why I didn't make all of the Tag substeps use Bootstrap::Util::CvsTag()): it's because if I did that, I would have to have added all of the error checking and die() logic to every call site; rather than do that, I centralized the argument handling, call itself, and then the error handling detection and die() logic. Also, keeping it in a subclass of step should ensure all the Right Things (tm) happen when die() is called.

I'm gonna check in a comment explaining this, but am probably going to keep Bootstrap::Step::Tag::CvsTag() around as a shim to Bootstrap::Util::CvsTag(), with the associated logic to do the right thing in the context of Bootstrap itself.

Comment 40

[J. Paul Reed [:preed]]

(In reply to comment #32)
> So I went through this bug last night, and believe these are the remaining
> things left:
> 
> -- Re-land the move of CvsTag() from Bootstrap::Step::Tag to Bootstrap::Util

Done (comment 36).

> -- Tag the TinderConfig-bumped files, as necessary (which is dependent upon the
> above).

Outstanding patch on this waiting for review.

> -- Make TinderConfig run when it should (which may mean refactoring out
> when/how TinderConfig is actually called); this is so that if an RC is bumped,
> then the TinderConfig steps get re-run, as necessary, without a human having to
> figure out where to restart the process. I've got a couple of ideas, but for
> _M6, maybe it'll be better to leave things as they are.

So I talked a bit with rhelmer about this, and I believe it's now unnecessary. The automation automatically runs the TinderConfig step after the tagging step, so it should just always do the correct thing; a human won't have to remember to re-run it.

> -- I took a stab at fixing the Config::General requirement here; I could either
> try again (I know it's possible), or punt on it altogether, since the wiki
> instructions for the build automation require Config::General to be installed
> on all machines (which I don't think is necessary/desired).

Also talked to rhelmer, and he said punting on this was ok, since the build instructions for the new buildbot infrastructure () include setting up 

> -- Investigate the permissions problem outlined in comment 14; I believe cf
> already fixed this (I seem to remember some patches running by that addressed
> permissions), but I'll make sure.

I started looking through this, but didn't have a chance to with the Netapp meetings and such. I'll take a stab next week.

Comment 41

[Robert Helmer [:rhelmer]]

Comment on attachment 280687 [details] [diff] [review]
Tag files that are modified by the TinderConfig step

Looks good but I don't really understand below:

>+        foreach my $configTagName (@tagNames) {
>+            # XXX - Don't like doing this this way (specifically, the logic 
>+            # change depending on the name of the branch in this loop...) 
>+            #
>+            # Also, the force argument to CvsTag() is interesting; we only
>+            # want to cvs tag -F a whatever_RELEASE tag if we're not tagging
>+            # the first RC; so, the logic is (rc > 1 && we're doing a _RELEASE


Why do we only want to "cvs tag -F" a _RELEASE tag if it's not the first rc? It's not necessary to use -F if the tag is not already applied but it should work the same as a normal tag regardless.

I agree it's annoying having to do the "_l10n" branch stuff, but I think it's just a consequence of the way we do tinder-config branches.

Anyway, looks fine as is, I wouldn't worry about only using -F if you're really forcing, but I'm not opposed to leaving it if you'd rather it be explicit like this.

Comment 42

[J. Paul Reed [:preed]]

(In reply to comment #41)
> (From update of attachment 280687 [details] [diff] [review])
> Looks good but I don't really understand below:
> 
> >+        foreach my $configTagName (@tagNames) {
> >+            # XXX - Don't like doing this this way (specifically, the logic 
> >+            # change depending on the name of the branch in this loop...) 
> >+            #
> >+            # Also, the force argument to CvsTag() is interesting; we only
> >+            # want to cvs tag -F a whatever_RELEASE tag if we're not tagging
> >+            # the first RC; so, the logic is (rc > 1 && we're doing a _RELEASE
> 
> 
> Why do we only want to "cvs tag -F" a _RELEASE tag if it's not the first rc?
> It's not necessary to use -F if the tag is not already applied but it should
> work the same as a normal tag regardless.

The design assumption is that a "cvs tag foo_RELEASE" operation will succeed if it's RC 1; if it fails in that case (presumably because the tag exists), I want to be able to know that, because it violates the design assumption. So that's why the logic enforces the design assumption (which is a design "requirement," I guess).

> I agree it's annoying having to do the "_l10n" branch stuff, but I think it's
> just a consequence of the way we do tinder-config branches.

Yes, and unfortunately, we can't tag tinder-configs any other way (since the regular _RELEASE tags are already used on those files, on different branches). My particular distaste here was the construct:

foreach [some list of things] {
   if (something) {
      some behavior for some/most items in the list
   } else {
      completely different behavior
   }
}

I don't think that's a particularly robust construct.

> Anyway, looks fine as is, I wouldn't worry about only using -F if you're really
> forcing, but I'm not opposed to leaving it if you'd rather it be explicit like
> this.

Different engineering aesthetic, I guess. :-)

Landed:

Checking in Bootstrap/Step/TinderConfig.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/TinderConfig.pm,v  <--  TinderConfig.pm
new revision: 1.4; previous revision: 1.3
done

Comment 43

[J. Paul Reed [:preed]]

Attachment: Fix perms of deliverables created by the Stage()ing step + some cleanup

This patch does a number of things; in the Bootstrap::Step::Stage()ing step:

-- A lot of paths were being created multiple times by calling catfile() or rebuilding paths that had already been created. I cleaned these up so we only created (sometimes constituent) paths once, so they would only have to be changed in one place in the future.

-- Bootstrap::Step::Stage::GroomFiles() was not chmod()/chown()ing files or directories that it created through either mkdir or moves, so deliverables in the tree would have the wrong perms. This fixes that.

-- Found a relatively serious bug in MozBuild::Util::MkdirWithPaths: it always returns success even if the directory creation wasn't successful. (It turns out $@ is always defined; the proper test is whether it's the empty string).

Comment 44

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

Comment on attachment 281703 [details] [diff] [review]
Fix perms of deliverables created by the Stage()ing step + some cleanup

>Index: Bootstrap/Step/Stage.pm
>===================================================================
...
>     # fix xpi dir names - This is a hash of directory names in the pre-stage
>     # dir -> directories under which those directories should be moved to;
>     # the name will be "xpi", so windows-xpi becomes win32/xpi, etc.
>     my %xpiDirs = ('windows-xpi' => 'win32',
>                    'linux-xpi' => 'linux-i686',
>                    'mac-xpi' => 'mac');
(which continues ...)
    foreach my $xpiDir (keys(%xpiDirs)) {

        my $fromDir = catfile($stageDir, 'batch1', 'stage-unsigned', $xpiDir);
        my $toDir = catfile($stageDir, 'batch1', 'stage-unsigned',
         $xpiDirs{$xpiDir}, 'xpi');

        if (-e $fromDir) {
           move($fromDir, $toDir)
            or die(msg => "Cannot rename $fromDir $toDir: $!");
	
You could use $batch1Dir here too. It looks like TrimCallback() is setting the perms correctly on (and in) prestage-trimmed/*-xpi/ (a bit earlier). Presumably the move preserves that ok ? Might be worth a comment that GroomFiles() does perms for the rest of the bits.

For kicks you could extend GroomFiles to handle the xpi dirs too. :-)

...	
> sub GroomFiles {
...
>     my $config = new Bootstrap::Config();
>+    my (undef, undef, $gid) = getgrnam($config->Get(var => 'product'));

We've used
       or die "Could not getgrname for $product: $!";
for the other getgrnam calls. Worth matching them ?
 
>-                    MkdirWithPath(dir => $pretty_dirname) 
>-                        or die "Cannot create $pretty_dirname: $!";
>+                    my @dirsCreated = ();
>+
>+                    eval { @dirsCreated = mkpath($pretty_dirname, 1, 0777) };
>+           
>+                    if ($@ ne '') {
>+                        die("Cannot create $pretty_dirname: $@");
>+                    }
>+
>+                    foreach my $dir (@dirsCreated) {
>+                        chmod(0755, $dir) or die("Could not chmod $dir to 755");
>+                        chown(-1, $gid, $dir) or die("Could not chown $dir " .
>+                         "to group $gid");
>+                    }
>+

Not sure why you specify perms in the mkpath call. It seems to be an optional argument, so it's a little confusing when chmod is setting the perms you care about.

>Index: MozBuild/Util.pm
>===================================================================
...
>     eval { mkpath($dirToCreate, $printProgress, $dirMask) };
>-    return defined($@);
>+    return ($@ eq '');
> }

Nice catch!

Comment 45

[Robert Helmer [:rhelmer]]

Attachment: create symbols dir, and remove _RELEASE_l10n tag from clean mirror

This makes creating a staging environment for the latest bootstrap code easier.

Comment 46

[Robert Helmer [:rhelmer]]

Attachment: create symbols dir, and remove _RELEASE_l10n tag from clean mirror (2)

missed the RCs.

this is reminding me that we should make the release-specific parts of this makefile more generic, maybe parameters passed to make or at least variables (e.g. RELEASE=FIREFOX_2_0_0_4).

Comment 47

[J. Paul Reed [:preed]]

(In reply to comment #44)

> You could use $batch1Dir here too. It looks like TrimCallback() is setting the
> perms correctly on (and in) prestage-trimmed/*-xpi/ (a bit earlier). Presumably
> the move preserves that ok ? Might be worth a comment that GroomFiles() does
> perms for the rest of the bits.

Fixed; the move shouldn't affect the perms, as long as they're created correctly. 

> For kicks you could extend GroomFiles to handle the xpi dirs too. :-)

I... *could*... but that's another bug, I'm sure. I actually think a lot of this step should be refactored; GroomFiles() was a port of the code in groom-files, and that code was weird in... weird ways. It'd be good to reexamine it at some point (and in my comment, I pointed out how I'd like to refactor how we set permissions and ownership, since we do so in a mishmash of places. :-(

> We've used
>        or die "Could not getgrname for $product: $!";
> for the other getgrnam calls. Worth matching them ?

Fixed.

> Not sure why you specify perms in the mkpath call. It seems to be an optional
> argument, so it's a little confusing when chmod is setting the perms you care
> about.

Couple of things: I ended up taking out the mask, but kept the argument to print out which directories get created, for logging purposes.

The File::Path documentation is... not great here... in reading the source to File::Path, that last argument is not a mode; it's a mask, and it gets modified by the current umask in the environment.

So, the default is 777, but directories that get created aren't created as rwxrwxrwx; that's why the next chmod call is necessary.

I blame the authors; they gloss over this bigtime... which brings me to:

> >Index: MozBuild/Util.pm
> >===================================================================
> ...
> >     eval { mkpath($dirToCreate, $printProgress, $dirMask) };
> >-    return defined($@);
> >+    return ($@ eq '');
> > }
> 
> Nice catch!

Well... it would've been a better catch if I'd have caught it when I wrote it; but the sample code in the documentation gives the example:

        eval { mkpath($dir) };
         if ($@) {
           print "Couldn't create $dir: $@";
         }

which is kinda ambiguous with Perl's sloshy type conversion about what this really means. $@ is probably *never* undef... and I *probably* should've known that. ;-)

C'est la Vie.

Thanks for the r=; landed:

Checking in MozBuild/Util.pm;
/cvsroot/mozilla/tools/release/MozBuild/Util.pm,v  <--  Util.pm
new revision: 1.20; previous revision: 1.19
done
Checking in Bootstrap/Step/Stage.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Stage.pm,v  <--  Stage.pm
new revision: 1.20; previous revision: 1.19
done

I think this is the last of the original list of _M5 problems, but I'll leave this bug open to review rhelmer's patch and get that in.

Comment 48

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

Attachment: Fix perms on stage-merged/KEY

(In reply to comment #47)
I can verify that this patch does great things on the staging setup.

find stage-merged batch1/{stage-signed,mar}  -type d \( ! -user cltbld -o ! -group firefox -o ! -perm 755 \)  -ls
3015103    4 drwx------   4 cltbld   cltbld       4096 Sep 23 21:47 stage-merged
3015104    4 drwxrwsr-x   2 cltbld   firefox      4096 Sep 23 21:47 stage-merged/contrib
3015106    4 drwxrwsr-x   2 cltbld   firefox      4096 Sep 23 21:47 stage-merged/contrib-localized

find stage-merged batch1/{stage-signed,mar}  -type f \( ! -user cltbld -o ! -group firefox -o ! -perm 644 \)  -ls
3015107    4 -rw-------   1 cltbld   cltbld       3804 Sep 23 21:47 stage-merged/KEY

The dirs are as I expected (or don't matter), and there is only the KEY file with wrong perms and group membership. This patch should fix that.

Comment 49

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

(In reply to comment #48)
> Created an attachment (id=282091) [details]
> Fix perms on stage-merged/KEY

Checking in Stage.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Stage.pm,v  <--  Stage.pm
new revision: 1.21; previous revision: 1.20
done

Comment 50

[Robert Helmer [:rhelmer]]

Checking in Makefile;
/cvsroot/mozilla/tools/release/Makefile,v  <--  Makefile
new revision: 1.8; previous revision: 1.7
done

Comment 51

[J. Paul Reed [:preed]]

Whee!