356185 - Implement release automation steps

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Robert Helmer [:rhelmer]]

The release automation system is broken down into a series of steps, which (generally) call out to the shell. Error handling, logging and basic logic should be implemented by the framework where appropriate.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: basic implementation for tag and build steps

This patch depends on attachment 241839 [details] [diff] [review] in bug 352230.

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: more steps implemented

Implement the following steps:

* Tag
* Build
* Source
* Repack
* Stage
* Updates

The only ones I've been able to do a complete test with are Tag, Build, Repack and Updates. Stage is pretty demanding on disk space so I haven't run it all the way through yet.

Note that these are still works in progress; I think it's going to be desirable to break up the big Steps (like Tag and Stage) into small substeps, and have there be a Push and Announce substep where applicable.

It'll also be a *lot* more testable if the steps are broken down into little pieces.

However, I don't want to stay out of the tree too long or the diffs will be incomprehensible :) So, I'd like to land this before I start breaking the steps down. Let me know if you see anything wrong in here or have suggestions for improvement.

Comment 3

[J. Paul Reed [:preed]]

Comment on attachment 242130 [details] [diff] [review]
more steps implemented

>=== trunk/Bootstrap/Step/Repack.pm
>==================================================================
>--- trunk/Bootstrap/Step/Repack.pm	(/mirror/release)	(revision 1615)
>+++ trunk/Bootstrap/Step/Repack.pm	(/local/release)	(revision 1615)
>@@ -4,12 +4,40 @@
> 
> sub Execute {

>+      #'cmd' => './build-seamonkey.pl --once --mozconfig mozconfig --depend --config-cvsup-dir `pwd`/tinderbox-configs',
>+      'cmd' => './build-seamonkey.pl --once --mozconfig mozconfig --clobber',

Can we setup a run with this; I'm wondering if `pwd` would work; might have tot hcnage that to $buildDir/tinderbox-configs.

>=== trunk/Bootstrap/Step/Tag.pm
>==================================================================
>--- trunk/Bootstrap/Step/Tag.pm	(/mirror/release)	(revision 1615)
>+++ trunk/Bootstrap/Step/Tag.pm	(/local/release)	(revision 1615)
>@@ -4,12 +4,235 @@
> 
> sub Execute {
>     my $this = shift;
>-    $this->Shell('cmd' => 'echo tag');
>+
>+    my $product    = $this->Config('var' => 'product');
>+    my $productTag = $this->Config('var' => 'productTag');
>+    my $branchTag  = $this->Config('var' => 'branchTag');
>+    my $pullDate   = $this->Config('var' => 'pullDate');
>+    my $rc         = $this->Config('var' => 'rc');
>+    my $version    = $this->Config('var' => 'version');
>+    my $appName    = $this->Config('var' => 'appName');
>+    my $logDir     = $this->Config('var' => 'logDir');
>+
>+    my $releaseTag    = $productTag.'_RELEASE';
>+    my $rcTag         = $productTag.'_RC'.$rc;
>+    my $minibranchTag = $productTag.'_MINIBRANCH';

Nit: spacing.

>+    my $tagDir = '/builds/tags/'.$releaseTag;
>+
>+    my $mozillaCvsroot = '/cvsroot';
>+    my $mofoCvsroot    = '/mofo';
>+    my $l10nCvsroot    = '/l10n';
>+
>+    # create the main tag directory
>+    if (not -d "$tagDir") {
>+        mkdir("$tagDir") || die "Cannot mkdir $tagDir: $!";
>+    }

Have you considering using MkdirWithPath(), instead of mkdir()? It's basically an impl of mkdir -p. Check MozAUSLib.pm in patcher for the impl.

>+    # Tagging area for Mozilla
>+    if (not -d $tagDir . '/cvsroot') {
>+        mkdir($tagDir . '/cvsroot') 

$tagDir . '$mozillaCvsroot?

>+          || die "Cannot mkdir $tagDir/cvsroot: $!";
>+    }
>+
>+    # Check out Mozilla from the branch you want to tag.
>+    $this->Shell(
>+      'cmd' => 'cvs -d ' . $mozillaCvsroot . ' co -r ' . $branchTag . ' -D "' . $pullDate . '" mozilla/client.mk ',

What if there is no branch tag? Or pull date, for that matter (although, no pull date would probably be pretty rare).

>+      'cmd' => 'gmake -f client.mk checkout MOZ_CO_PROJECT=all MOZ_CO_DATE="' . $pullDate . '"',
>+      'dir' => $tagDir . '/cvsroot/mozilla',
>+      'timeout' => '3600',

It seems the specification of this is inconsistent. Above you calculate it (60 * 60 * 10); here, it's an hour, specified straight out. Maybe before was the default?

>+    $this->Shell(
>+      'cmd' => 'cvs commit -m "For ' . $product . ' ' . $version . ', redirect client.mk onto the ' . $releaseTag . ' tag." client.mk',
>+      'dir' => $tagDir . '/cvsroot/mozilla',

For all of these, should you use the variables you defined above?

>+        # only keep first column
>+        $locale =~ s/\s+.*//;

I would write this as:

s/^(\S+).*//;

>+    my $tagName = $args{'tagName'};
>+    my $coDir   = $args{'coDir'};
>+    my $branch  = $args{'branch'};
>+    my $files   = $args{'files'};
>+    my $force   = $args{'force'};

Nit: spacing.

>+   
>+    my $logDir     = $this->Config('var' => 'logDir');
>+
>+    my $logFile = $logDir . '/' . $tagName . '.log';
>+
>+    my $cmd = 'cvs tag ' . $tagName;
>+    my $checkForOnly = '^T ';
>+
>+    if ($branch and $files) {
>+        $cmd = 'cvs tag -b ' . $tagName . ' ' . $files;
>+        $checkForOnly = '^B ';
>+    }
>+
>+    if ($force and $files) {
>+        $cmd = 'cvs tag -F ' . $tagName . ' ' . $files;
>+    }

I would specify this as: $cvsCommand = 'cvs tag' and then

if ($branch and $files) {
  $cmd = $cvsCommand . '-b ' . $tagName;
} elsif ($force and $files) {
  $cmd = $cvsCommand . '-F' . $tagName;
} else {
  $cmd = $cvsCommand . $tagName
}

>+    my $buildDir   = $this->Config('var' => 'buildDir');
>+    my $productTag = $this->Config('var' => 'productTag');
>+    my $rc         = $this->Config('var' => 'rc');
>+    my $logDir     = $this->Config('var' => 'logDir');
>+    my $rcTag      = $productTag . '_RC' . $rc;

Nit: spacing.


>+# XXX temp disabled
>+#    $this->CheckLog(
>+#        'log' => $buildLog,
>+#        'notAllowed' => 'failed',

Since this will fail on every build (due to the checkout), you might use the tinderbox status at the top of the log, i.e. tinderbox: status: success

It's not perfect, because it will often read fine when things aren't (l10n comes to mind), but...

>+    # create staging area
>+    my $stageDir = 
>+      '/data/cltbld/' . $product . '-' . $version . '/batch-source/rc' . $rc;

/data/cltbld should be a constant.

>+    if (not -d $stageDir) {
>+        File::Path::mkpath($stageDir) || die "Cannot create $stageDir: $!";
>+    }

MkdirWithPath()?

>+    move("$stageDir/../*.bz2", $stageDir);
>+    chmod(0644, "$stageDir/*.bz2");

You might chgrp, too.

>+
>+#    $this->Shell(
>+#      'cmd' => 'rsync -av *.bz2 /home/ftp/pub/' . $product . '/nightly/' . $version . '-candidates/rc' . $rc,

Just a thought: I'd like to get away from using rsync programatically like this, *if possible*. It may not be, especially for M1.

>+
>+    my $product    = $this->Config('var' => 'product');
>+    my $version    = $this->Config('var' => 'version');
>+    my $rc         = $this->Config('var' => 'rc');
>+    my $logDir     = $this->Config('var' => 'logDir');

Nit: spacing.

>+    ## Prepare the staging directory for the release.
>+    # Create the staging directory.
>+
>+    my $stageDir = '/data/cltbld/' . $product . '-' . $version;

Constant.

>+    if (not -d $stageDir) {
>+        mkdir($stageDir) || die "Could not mkdir $stageDir: $!";

MkdirWithPath().

>+ 
>+    # Create skeleton batch directory.
>+    my $skelDir = "$stageDir/batch-skel/stage";
>+    if (not -d "$skelDir") {
>+        File::Path::mkpath($skelDir) || die "Cannot create $skelDir: $!";

MkdirWithPath().

>+    }
>+
>+    # Create the contrib and contrib-localized directories with expected 
>+    # access rights.
>+    if (not -d "$skelDir/contrib") {
>+        mkdir("$skelDir/contrib") || die "Could not mkdir $skelDir/contrib: $!";
>+    }
>+
>+    my ($pwname, $pass, $uid) = getpwnam('cltbld');
>+    my ($grname, $passwd, $gid) = getgrnam($product);
>+
>+    my @dir = ("$skelDir/contrib");
>+#    chown($uid, $gid, @dir) 
>+#      || die "Cannot change ownership on $skelDir/contrib: $!";
>+#    chmod('2775', @dir)
>+#      || die "Cannot change mode on $skelDir/contrib to 2775: $!";
>+ 
>+    # NOTE - should have a standard "master" copy somewhere else
>+    # Copy the KEY file from the previous release directory.
>+    $this->Shell(
>+      'cmd' => 'rsync -a /home/ftp/pub/' . $product . '/releases/1.5/KEY ' . $skelDir . '/',

Just copy the key. Use File::Copy.

>+      'logFile' => $logDir . '/stage-copy_key.log',
>+      'dir' => $skelDir,
>+    );
>+
>+    ## Prepare the merging directory.
>+    $this->Shell(
>+      'cmd' => 'rsync -av batch-skel/stage/ stage-merged/',
>+      'logFile' => $logDir . '/stage-merge_skel.log',
>+      'dir' => $stageDir,
>+    );
>+    
>+    # Collect the release files onto stage.mozilla.org.
>+
>+    if (not -d "$stageDir/batch1/prestage") {
>+      File::Path::mkpath("$stageDir/batch1/prestage") 

MkdirWithPath().


>+    $this->Shell(
>+      'cmd' => 'find . -type f -exec chmod -v 644 {} \;',

Use File::Find.

>+    if (-f "$stageDir/batch1/prestage-trimmed/*.zip") {
>+      unlink("$stageDir/batch1/prestage-trimmed/*.zip")

I don't think this is going to work. You might get what you want by using a glob in the right directory.

>+        || die "Cannot unlink $stageDir/batch1/prestage-trimmed/*.zip: $!";
>+    }
>+
>+    #find . -name xforms.xpi -exec rm -fv {} \;
>+    #find . -type d -name "*-xpi" -empty -exec rmdir {} \;

Should still do these.

>+    ## ja-JP-mac is the JA locale for mac, do not ship ja
>+    unlink("$stageDir/batch1/prestage-trimmed/*.ja.mac*");
>+    unlink("$stageDir/batch1/prestage-trimmed/mac-xpi/ja.xpi");

Same thing; glob?

>+    ## ja is the JA locale for win32/linux, do not ship ja-JP-mac
>+    #find . -name "*ja-JP-mac.win32*" -exec rm -fv {} \;
>+    #find . -name "*ja-JP-mac.linux*" -exec rm -fv {} \;
>+    #find . -name "en-US.xpi" -exec rm -fv {} \;
>+    unlink("$stageDir/batch1/prestage-trimmed/linux-xpi/ja-JP-mac.xpi");
>+    unlink("$stageDir/batch1/prestage-trimmed/windows-xpi/ja-JP-mac.xpi");

We should omit the whole batch1 thing if we're only doing one batch, and it's hardcoded anyway.

Which is to say we should probably call it something else entirely.

>+    $this->Shell(
>+      'cmd' => '/data/cltbld/bin/groom-files --short=' . $version . ' .',
>+      'logFile' => $logDir . '/stage-collect.log',
>+      'dir' => $stageDir . '/batch1/stage',
>+    );

Should support long names.

>+    # fix xpi dir names
>+    rename("$stageDir/batch1/stage/linux-xpi", 
>+           "$stageDir/batch1/stage/linux-i686/xpi")
>+      || die "Cannot rename $stageDir/batch1/stage/linux-xpi $stageDir/batch1/stage/linux-i686/xpi: $!";
>+    rename("$stageDir/batch1/stage/windows-xpi", 
>+           "$stageDir/batch1/stage/win32/xpi")
>+      || die "Cannot rename $stageDir/batch1/stage/windows-xpi $stageDir/batch1/stage/win32/xpi: $!";
>+    rename("$stageDir/batch1/stage/mac-xpi", 
>+           "$stageDir/batch1/stage/mac/xpi")
>+      || die "Cannot rename $stageDir/batch1/stage/mac-xpi $stageDir/batch1/stage/mac/xpi: $!";

You may want to investigate File::Copy::move for these types of ops; it seems to be a bit more robust.

>+      'cmd' => '/data/cltbld/bin/verify-locales.pl -m ' . $stageDir . 'batch-source/mozilla/' . $appName . '/locales/shipped-locales',

Should make this a callable part of the library. But that's not necessarily an m1 thing.

Comment 4

[Robert Helmer [:rhelmer]]

Thanks a lot for the review! I have a patch to address *most* of these issues, can you take a quick look at my responses (below) and let me know if you have any further feedback before I submit another patch?

(In reply to comment #3)
> (From update of attachment 242130 [details] [diff] [review] [edit])
> >=== trunk/Bootstrap/Step/Repack.pm
> >==================================================================
> >--- trunk/Bootstrap/Step/Repack.pm	(/mirror/release)	(revision 1615)
> >+++ trunk/Bootstrap/Step/Repack.pm	(/local/release)	(revision 1615)
> >@@ -4,12 +4,40 @@
> > 
> > sub Execute {
> 
> >+      #'cmd' => './build-seamonkey.pl --once --mozconfig mozconfig --depend --config-cvsup-dir `pwd`/tinderbox-configs',
> >+      'cmd' => './build-seamonkey.pl --once --mozconfig mozconfig --clobber',
> 
> Can we setup a run with this; I'm wondering if `pwd` would work; might have tot
> hcnage that to $buildDir/tinderbox-configs.


Yes, I know this works :) However there is no reason for this kind of shortcut here, I'll change it to $buildDir/tinderbox-configs for clarity's sake.


> >=== trunk/Bootstrap/Step/Tag.pm
> >==================================================================
> >--- trunk/Bootstrap/Step/Tag.pm	(/mirror/release)	(revision 1615)
> >+++ trunk/Bootstrap/Step/Tag.pm	(/local/release)	(revision 1615)

> >+    # create the main tag directory
> >+    if (not -d "$tagDir") {
> >+        mkdir("$tagDir") || die "Cannot mkdir $tagDir: $!";
> >+    }
> 
> Have you considering using MkdirWithPath(), instead of mkdir()? It's basically
> an impl of mkdir -p. Check MozAUSLib.pm in patcher for the impl.


Ok pulling this into MozBuild/Util.pm


> >+    # Check out Mozilla from the branch you want to tag.
> >+    $this->Shell(
> >+      'cmd' => 'cvs -d ' . $mozillaCvsroot . ' co -r ' . $branchTag . ' -D "' . $pullDate . '" mozilla/client.mk ',
> 
> What if there is no branch tag? Or pull date, for that matter (although, no
> pull date would probably be pretty rare).


This is a really good point, but for M1 I am going to punt, and make this work in the way we've done it in the past. I'll comment it, however.

 
> >+      'cmd' => 'gmake -f client.mk checkout MOZ_CO_PROJECT=all MOZ_CO_DATE="' . $pullDate . '"',
> >+      'dir' => $tagDir . '/cvsroot/mozilla',
> >+      'timeout' => '3600',
> 
> It seems the specification of this is inconsistent. Above you calculate it (60
> * 60 * 10); here, it's an hour, specified straight out. Maybe before was the
> default?


Good point; I'll make this more consistent.. any objection to specifying it in seconds and not calculated? 


> >+    $this->Shell(
> >+      'cmd' => 'cvs commit -m "For ' . $product . ' ' . $version . ', redirect client.mk onto the ' . $releaseTag . ' tag." client.mk',
> >+      'dir' => $tagDir . '/cvsroot/mozilla',
> 
> For all of these, should you use the variables you defined above?


Do you mean the "cvsroot/mozilla" part? The $mozillaCvsroot tag is actually misleading; on the staging server it's /cvsroot but in production it's [e-mail]:/cvsroot, so it's not actually used for the same thing.

Please correct if I am misunderstanding.
 

> >+        # only keep first column
> >+        $locale =~ s/\s+.*//;
> 
> I would write this as:
> 
> s/^(\S+).*//;

Ok good call.


> >+    my $tagName = $args{'tagName'};
> >+    my $coDir   = $args{'coDir'};
> >+    my $branch  = $args{'branch'};
> >+    my $files   = $args{'files'};
> >+    my $force   = $args{'force'};
> 
> Nit: spacing.


Just doing it to annoy you :) Seriously though, I don't have a strong opinion, I will take them out so we don't have to maintain the formatting.

I'll go ahead and do this globally.

 
> >+    my $logDir     = $this->Config('var' => 'logDir');
> >+
> >+    my $logFile = $logDir . '/' . $tagName . '.log';
> >+
> >+    my $cmd = 'cvs tag ' . $tagName;
> >+    my $checkForOnly = '^T ';
> >+
> >+    if ($branch and $files) {
> >+        $cmd = 'cvs tag -b ' . $tagName . ' ' . $files;
> >+        $checkForOnly = '^B ';
> >+    }
> >+
> >+    if ($force and $files) {
> >+        $cmd = 'cvs tag -F ' . $tagName . ' ' . $files;
> >+    }
> 
> I would specify this as: $cvsCommand = 'cvs tag' and then
> 
> if ($branch and $files) {
>   $cmd = $cvsCommand . '-b ' . $tagName;
> } elsif ($force and $files) {
>   $cmd = $cvsCommand . '-F' . $tagName;
> } else {
>   $cmd = $cvsCommand . $tagName
> }


Yeah more readable, thanks.

> >+# XXX temp disabled
> >+#    $this->CheckLog(
> >+#        'log' => $buildLog,
> >+#        'notAllowed' => 'failed',
> 
> Since this will fail on every build (due to the checkout), you might use the
> tinderbox status at the top of the log, i.e. tinderbox: status: success
> 
> It's not perfect, because it will often read fine when things aren't (l10n
> comes to mind), but...


I agree.. this is good enough for now (better than looking for "failed" anyway!)


> >+    # create staging area
> >+    my $stageDir = 
> >+      '/data/cltbld/' . $product . '-' . $version . '/batch-source/rc' . $rc;
> 
> /data/cltbld should be a constant.

Made it configurable as stageHome.


> >+    move("$stageDir/../*.bz2", $stageDir);
> >+    chmod(0644, "$stageDir/*.bz2");
> 
> You might chgrp, too.


What group is appropriate?


> >+
> >+#    $this->Shell(
> >+#      'cmd' => 'rsync -av *.bz2 /home/ftp/pub/' . $product . '/nightly/' . $version . '-candidates/rc' . $rc,
> 
> Just a thought: I'd like to get away from using rsync programatically like
> this, *if possible*. It may not be, especially for M1.


I agree, but I am going to defer this to after M1.

> >+    # NOTE - should have a standard "master" copy somewhere else
> >+    # Copy the KEY file from the previous release directory.
> >+    $this->Shell(
> >+      'cmd' => 'rsync -a /home/ftp/pub/' . $product . '/releases/1.5/KEY ' . $skelDir . '/',
> 
> Just copy the key. Use File::Copy.


Yeah rsync for one file is silly.


> >+    $this->Shell(
> >+      'cmd' => 'find . -type f -exec chmod -v 644 {} \;',
> 
> Use File::Find.


Hmm.. seems like what we really want is to run chmod recursively here. We could use File::Find to build that list, but there is probably a simpler way; I'll research it a little.


> >+    if (-f "$stageDir/batch1/prestage-trimmed/*.zip") {
> >+      unlink("$stageDir/batch1/prestage-trimmed/*.zip")
> 
> I don't think this is going to work. You might get what you want by using a
> glob in the right directory.


You are correct; I am going to opt for:
foreach my $file (glob("*blah*")) {
  unlink($file) || die("Cannot unlink $file: $!");
}


> >+    #find . -name xforms.xpi -exec rm -fv {} \;
> >+    #find . -type d -name "*-xpi" -empty -exec rmdir {} \;
> 
> Should still do these.




> We should omit the whole batch1 thing if we're only doing one batch, and it's
> hardcoded anyway.


Hmm good point.. I am going to leave it just for now, since I want to stay close to the release doc, for M1. I do agree though, it should be changed.

  
> >+    $this->Shell(
> >+      'cmd' => '/data/cltbld/bin/groom-files --short=' . $version . ' .',
> >+      'logFile' => $logDir . '/stage-collect.log',
> >+      'dir' => $stageDir . '/batch1/stage',
> >+    );
> 
> Should support long names.


Going to say post-M1 .. M1 is all about being able to reproduce the 1.5.0.7 release, we will most assuredly need changes to support things other than the 1.5 branch. I'll comment it.


> >+    # fix xpi dir names
> >+    rename("$stageDir/batch1/stage/linux-xpi", 
> >+           "$stageDir/batch1/stage/linux-i686/xpi")
> >+      || die "Cannot rename $stageDir/batch1/stage/linux-xpi $stageDir/batch1/stage/linux-i686/xpi: $!";
> >+    rename("$stageDir/batch1/stage/windows-xpi", 
> >+           "$stageDir/batch1/stage/win32/xpi")
> >+      || die "Cannot rename $stageDir/batch1/stage/windows-xpi $stageDir/batch1/stage/win32/xpi: $!";
> >+    rename("$stageDir/batch1/stage/mac-xpi", 
> >+           "$stageDir/batch1/stage/mac/xpi")
> >+      || die "Cannot rename $stageDir/batch1/stage/mac-xpi $stageDir/batch1/stage/mac/xpi: $!";
> 
> You may want to investigate File::Copy::move for these types of ops; it seems
> to be a bit more robust.


How so? I am not against it, just curious about what problems it solves that you know about. I'll do a little research on it as well.


> >+      'cmd' => '/data/cltbld/bin/verify-locales.pl -m ' . $stageDir . 'batch-source/mozilla/' . $appName . '/locales/shipped-locales',
> Should make this a callable part of the library. But that's not necessarily an
> m1 thing.


Yes and yes :)

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to comment #3)
> (From update of attachment 242130 [details] [diff] [review] [edit])
> >+        # only keep first column
> >+        $locale =~ s/\s+.*//;
> 
> I would write this as:
> 
> s/^(\S+).*//;


Came up in testing.. it's actually a little different, since we want to take lines like this:

gu-IN win32 linux

And end up with:

gu-IN

That is, remove everything at and after the first occurrence of whitespace.
I set it to: s/(\s+).*//;
which has the desired behavior.

Does that make sense, and do you have a suggestion for a clearer way to do this?

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: core steps implemented

Basically same as before, takes into account feedback from previous patch. Sorry if I missed anything, let me know! I am trying not to make any huge changes until this all gets through review and landed, since it's already quite large.

Makefile reliably builds a cvsmirror now.

I've run this through a full cycle on the staging box, and made a couple minor tweaks, mostly paths that were slightly off.

Comment 7

[J. Paul Reed [:preed]]

Comment on attachment 244367 [details] [diff] [review]
core steps implemented

>+    # Create updates area.
>+    my $updateDir = '/builds/updates/' . $version;

I'd make things like this configurable in the config file (with a reasonable default).

>+      'cmd' => 'cvs -d ' . $mofoCvsroot . ' co -d config release/patcher/moz180-branch-patcher2.cfg',

This limits its use to the 1.8.0 branch; config file?


> sub Verify {
>     my $this = shift;
>-    $this->Shell('cmd' => 'echo Verify updates');
>+
>+    my $logDir = $this->Config('var' => 'logDir');
>+    my $version = $this->Config('var' => 'version');
>+    my $mozillaCvsroot = $this->Config('var' => 'mozillaCvsroot');
>+
>+    ### quick verification
>+    # ensure that there are only test channels
>+    # ssh aus2-staging.mozilla.org
>+    # cd /opt/aus2/incoming/3-staging/$tmpdir
>+    # find . -name "*.txt" | grep -v test

I'd *really* like to see this check in the initial rev. Basically check that aus2.test ONLY has test channels, and aus2 has both test and release channels.

>+    # convert version number
>+    # SHORT_VERSION=`echo ${VERSION} | sed 's/\.//'`
>+    # SHORT_PREVIOUS_VERSION=`echo ${PREVIOUS_VERSION} | sed 's/\.//'`
>+    
>+    # Create verification area.
>+    my $verifyDir = '/builds/verify/' . $version;
>+    $this->MkdirWithPath($verifyDir);

Same deal here. Also, $this->MkdirWithPath() or die()?

>+    # Symlink to to RC dir
>+    my $fromLink = '/builds/tags/' . $releaseTag;
>+    my $toLink   = '/builds/tags/' . $rcTag;

Same deal here.

>+    if (not -e $toLink) {
>+        symlink("$fromLink", "$toLink")

Minor nit: I try not to quote variables when they're "just variables."

>+      'files'   => 'client.mk',

does _CvsTag take an array if files is specified? It probably should...

>+    $this->Shell(
>+      'cmd' => 'cvs commit -m "For ' . $product . ' ' . $version . ', redirect client.mk onto the ' . $releaseTag . ' tag." client.mk',
>+      'dir' => $tagDir . '/cvsroot/mozilla',
>+      'logFile' => $logDir . '/client_mk-release_tag.log',
>+    );

Should check this didn't fail.

>+    # Create the RC tag.
>+    $this->_CvsTag(
>+      'tagName' => $rcTag,
>+      'coDir'   => $tagDir . '/mofo/talkback/fullsoft',
>+      'logFile' => $logDir . '/mofo_tag-' . $rcTag. '.log',
>+    );

We have traditionally NOT created _RC tags for the mofo repo. I just checked, and it's inconsistent.

The reasoning is that this directory seldom changes, and it's never changed between an RC.

> sub Verify {
>     my $this = shift;
>-    $this->Shell('cmd' => 'echo Verify tag');
>+    # XXX temp disable 
>+    #$this->Shell('cmd' => 'echo Verify tag');
> }
> 
>+sub _CvsTag {
>+    my $this = shift;
>+    my %args = @_;
>+
>+    my $tagName = $args{'tagName'};
>+    my $coDir = $args{'coDir'};
>+    my $branch = $args{'branch'};
>+    my $files = $args{'files'};
>+    my $force = $args{'force'};
>+    my $logFile = $args{'logFile'};
>+   
>+    my $logDir     = $this->Config('var' => 'logDir');
>+
>+    my $cmd;
>+    my $cvsCommand = 'cvs tag';
>+    my $checkForOnly = '^T ';
>+
>+    if ($branch and $files) {
>+        $cmd = $cvsCommand . ' -b ' . $tagName . ' ' . $files;
>+        $checkForOnly = '^B ';
>+    } elsif ($force and $files) {
>+        $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
>+    } else {
>+        $cmd = $cvsCommand . ' ' . $tagName;
>+    }

You miss the case that I have a set of files I want to tag (but no force or branch tag)


>+    $this->Shell(
>+      'cmd' => './build-seamonkey.pl --once --mozconfig mozconfig --depend --config-cvsup-dir ' . $buildDir . '/tinderbox-configs',
>+      'dir' => $buildDir,
>+      'logFile' => $buildLog,
>+      'timeout' => 36000
>+    );

unlink("last-built") (in the right directory) before this?

>+    rename("$stageDir/../*.bz2", $stageDir);

I think you want File::Copy::move().

>+    chmod(0644, "$stageDir/*.bz2");

Does this work? (the *?)

>+    my ($pwname, $pass, $uid) = getpwnam('cltbld');
>+    my ($grname, $passwd, $gid) = getgrnam($product);

Check for failure here.

>+    $this->Shell(
>+      'cmd' => 'find . -type f -exec chmod -v 644 {} \;',
>+      'logFile' => $logDir . '/stage-collect_change-perms.log',
>+      'dir' => $stageDir . '/stage-merged',
>+    );

Can you use File::Find here?

For all these unlinks(), I'd probably log somewhere that I'm deleting them.

>+    foreach my $file (glob("$stageDir/batch1/prestage-trimmed/*.zip")) {
>+      unlink($file) || $this->Log('msg' => "Cannot unlink $file: $!");
>+    }

Comment 8

[Robert Helmer [:rhelmer]]

Attachment: core steps implemented

Addresses comments from previous review.

Comment 9

[J. Paul Reed [:preed]]

Comment on attachment 244968 [details] [diff] [review]
core steps implemented

>=== Bootstrap/Step/Repack.pm

>+    my $buildLog = $logDir . '/' . $rcTag . '-build-l10n.log';
>+    my $lastBuilt = $buildDir . '/' . $buildPlatform . '/last-built';

For constructs like these, we should use File::Spec's join() method; I just recently (i.e. yesterday) discovered this, so this is totally a nit that can be fixed later.

There's lots of good stuff in File::Spec.

>+    File::Find::find(
>+      sub { 
>+        my $dir = $File::Find::dir;
>+        if ($dir =~ /test/) { 
>+            die("Non-test directory found in $testDir/aus2.test: $dir");
>+        }
>+      }, $testDir
>+    );

I'm a huge fan of putting callbacks that span more than a line somewhere else.

Of course, you can make this one do that:

File::Find::find( sub { die("Non-test dir found: $File::Find::name") if $File::Find::name =~ /test/);

I'd also match against ::name, not ::dir.

>+    if ($branch and $files) {
>+        $cmd = $cvsCommand . ' -b ' . $tagName . ' ' . $files;
>+        $checkForOnly = '^B ';
>+    } elsif ($force and $files) {
>+        $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
>+    } elsif ($files) {
>+        $cmd = $cvsCommand . ' ' . $tagName . $files;
>+    } else {
>+        $cmd = $cvsCommand . ' ' . $tagName;
>+    }

This is *probably* ok for now, but... we should really fix this at some point.

$force and $branch have no dependency on whether $files exist, but the code above presents an implicit dependency.

Now, I think I understand what you're trying to do here ("You can't create a branch of the entire tree using this automation, and you can't force-tag the entire tree using this automation"), but I'd rather see those expressed as assert()ions that are hard failures, rather than unexpected behavior. For instance, if I pass a $branch option, but no $files, this code will create a non-branch tag named FOO_BRANCH (likely), and tag every file in the tree. Oops.

>+    my @dir = ("$skelDir/contrib");
>+    chown($uid, $gid, @dir) 
>+      or die "Cannot change ownership on $skelDir/contrib: $!";

I'm somewhat surprised this work, as most OS's restrict use of chown() to root; chgrp() to a group you're in is fine.

Was this run as cltbld? It may have worked because the $uid was a no-op. Might want to change these to chgrp() calls, though.

>+sub TrimCallback { 
>+    my $file = $File::Find::name;
>+    if (-f $file) {
>+        if (($file =~ /xforms\.xpi/) || 
>+            # ja-JP-mac is the JA locale for mac, do not ship ja
>+            ($file =~ /ja\.mac/) ||
>+            # ja is the JA locale for win32/linux, do not ship ja-JP-mac
>+            ($file =~ /ja-JP-mac\.win32/) ||
>+            ($file =~ /ja-JP-mac\.linux/) ||
>+            ($file =~ /^windows-xpi\/ja-JP-mac\.xpi$/) ||
>+            ($file =~ /^linux-xpi\/ja-JP-mac\.xpi$/) ||
>+            # MAR files are merged in later
>+            ($file =~ /\.mar$/) ||
>+            # ZIP files are not shipped
>+            ($file =~ /\.zip$/) ||
>+            # en-US is the default, do not ship langpack
>+            ($file =~ /en-US\.xpi$/)) {
>+            unlink($file) || die "Could not unlink $file: $!";
>+            $this->Log('msg' => "Unlinked $file");

Indentation.

>+        }
>+    } else { 
>+        chmod(0644, $file) 
>+          || die "Could not chmod $file to 0644: $!";
>+        $this->Log('msg' => "Changed mode of $file to 0644");

Is this right? You check for files above, so File::Find will give you directories, they'll get into this branch, and get chmod()'d with 644.

Also, $file is misleading; it's technically not a file, is a directory entry. It might be good to rename it so that's clear (it could be a file, directory, symlink, etc., etc., etc.)

Other than that, looks good.

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to comment #9)
> (From update of attachment 244968 [details] [diff] [review] [edit])
> >=== Bootstrap/Step/Repack.pm
> >+    File::Find::find(
> >+      sub { 
> >+        my $dir = $File::Find::dir;
> >+        if ($dir =~ /test/) { 
> >+            die("Non-test directory found in $testDir/aus2.test: $dir");
> >+        }
> >+      }, $testDir
> >+    );
> 
> I'm a huge fan of putting callbacks that span more than a line somewhere else.
> I'd also match against ::name, not ::dir.

Ok done and done.

> 
> >+    if ($branch and $files) {
> >+        $cmd = $cvsCommand . ' -b ' . $tagName . ' ' . $files;
> >+        $checkForOnly = '^B ';
> >+    } elsif ($force and $files) {
> >+        $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
> >+    } elsif ($files) {
> >+        $cmd = $cvsCommand . ' ' . $tagName . $files;
> >+    } else {
> >+        $cmd = $cvsCommand . ' ' . $tagName;
> >+    }
> 
> 
> Now, I think I understand what you're trying to do here ("You can't create a
> branch of the entire tree using this automation, and you can't force-tag the
> entire tree using this automation"), but I'd rather see those expressed as
> assert()ions that are hard failures, rather than unexpected behavior. For
> instance, if I pass a $branch option, but no $files, this code will create a
> non-branch tag named FOO_BRANCH (likely), and tag every file in the tree. Oops.

This is a good catch .. I broke it out like this:

    if ($branch and $files) {
        $cmd = $cvsCommand . ' -b ' . $tagName . ' ' . $files;
        $checkForOnly = '^B ';
    } elsif ($force and $files) {
        $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
    } else {
        die("Must specify files if branch or force option is used.");
    }

    if ($files) {
        $cmd = $cvsCommand . ' ' . $tagName . $files;
    } else {
        $cmd = $cvsCommand . ' ' . $tagName;
    }

Your reading of the intent is correct, hopefully this makes it more clear. I'll add some comments too, just to make it clearer since it might not be obvious to a more casual reading.


> >+    my @dir = ("$skelDir/contrib");
> >+    chown($uid, $gid, @dir) 
> >+      or die "Cannot change ownership on $skelDir/contrib: $!";
> 
> I'm somewhat surprised this work, as most OS's restrict use of chown() to root;
> chgrp() to a group you're in is fine.


chown() shouldn't be here.. not sure why I copied that in from the doc. This is run by cltbld, so I am just going to remove the chown() call.


> Was this run as cltbld? It may have worked because the $uid was a no-op. Might
> want to change these to chgrp() calls, though.
> 
> >+sub TrimCallback { 
> >+    my $file = $File::Find::name;
> >+    if (-f $file) {
> >+        if (($file =~ /xforms\.xpi/) || 
> >+            # ja-JP-mac is the JA locale for mac, do not ship ja
> >+            ($file =~ /ja\.mac/) ||
> >+            # ja is the JA locale for win32/linux, do not ship ja-JP-mac
> >+            ($file =~ /ja-JP-mac\.win32/) ||
> >+            ($file =~ /ja-JP-mac\.linux/) ||
> >+            ($file =~ /^windows-xpi\/ja-JP-mac\.xpi$/) ||
> >+            ($file =~ /^linux-xpi\/ja-JP-mac\.xpi$/) ||
> >+            # MAR files are merged in later
> >+            ($file =~ /\.mar$/) ||
> >+            # ZIP files are not shipped
> >+            ($file =~ /\.zip$/) ||
> >+            # en-US is the default, do not ship langpack
> >+            ($file =~ /en-US\.xpi$/)) {
> >+            unlink($file) || die "Could not unlink $file: $!";
> >+            $this->Log('msg' => "Unlinked $file");
> 
> Indentation.

Got it

> 
> >+        }
> >+    } else { 
> >+        chmod(0644, $file) 
> >+          || die "Could not chmod $file to 0644: $!";
> >+        $this->Log('msg' => "Changed mode of $file to 0644");
> 
> Is this right? You check for files above, so File::Find will give you
> directories, they'll get into this branch, and get chmod()'d with 644.

Yes.. these were several separate find commands in the original doc; a bunch to remove unwanted files, and one to chmod 644 all directories.

> Also, $file is misleading; it's technically not a file, is a directory entry.
> It might be good to rename it so that's clear (it could be a file, directory,
> symlink, etc., etc., etc.)

Made it "$dirent" instead.

Comment 11

[Robert Helmer [:rhelmer]]

Attachment: address concerns from previous patch

Landing this patch based on previous r=preed

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: proper group settings

One thing the last patch didn't address is doing the chgrp to $product and setting the mode properly for dirs versus files.

Here is a diff against CVS, how does this strike you?

Comment 13

[J. Paul Reed [:preed]]

Comment on attachment 245158 [details] [diff] [review]
proper group settings

>=== Bootstrap/Step/Stage.pm
>==================================================================
>--- Bootstrap/Step/Stage.pm	(/mirror/release/trunk)	(revision 1837)
>+++ Bootstrap/Step/Stage.pm	(/local/release/trunk)	(revision 1837)
>@@ -42,14 +42,16 @@
>           or die "Could not mkdir $skelDir/contrib: $!";
>     }
> 
>-    my ($pwname, $pass, $uid) = getpwnam('cltbld') 
>-      or die "Could not getpwname for cltbld: $!";
>     my ($grname, $passwd, $gid) = getgrnam($product)
>       or die "Could not getgrname for $product: $!";

If you're not using $grname and $passwd, you can do:

my (undef, undef, $gid). (I don't care so much about this; just wanted to share the trick).

>-        }
>-    } else { 
>-        chmod(0644, $dirent) 
>-          || die "Could not chmod $dirent to 0644: $!";
>+        } else {
>+            chmod(0755, $dirent) 
>+              || die "Could not chmod $dirent to 0644: $!";
>+            $this->Log('msg' => "Changed mode of $dirent to 0644");

Shouldn't this be 644?

>+       }
>+    } elsif (-d $dirent) { 
>+        chown(-1, $gid, @dir)
>+          or die "Cannot chgrp $dirent to $product: $!";
>+            $this->Log('msg' => "Changed mode of $dirent to 0644");
>+        chmod(0755, $dirent) 
>+          or die "Could not chmod $dirent to 0644: $!";
>         $this->Log('msg' => "Changed mode of $dirent to 0644");

In the above three log/die statements, you're not changing $dirent to 644.

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: proper group/mode settings, take 2

right on all counts, sorry about that! I like the undef suggestion too so threw that in there as well.

Comment 15

[Robert Helmer [:rhelmer]]

Attachment: misc. corrections

Mostly minor corrections that came up during testing like adding slashes properly, plus:

* implement logic for _CvsTag in Step/Tag.pm correctly
* do chmod/chown properly in Step/Stage.pm
* moved TrimCallback back into nested subroutine

The last one is because (AFAICT) there is no way to pass arguments to a callback function called by File::Find::find, and I'd like to pass in $gid (and not make it global). If you have any suggestions let me know.

Comment 16

[J. Paul Reed [:preed]]

Comment on attachment 246043 [details] [diff] [review]
misc. corrections

>=== Bootstrap/Step/Updates.pm
>==================================================================
>--- Bootstrap/Step/Updates.pm	(/mirror/release/trunk)	(revision 1882)
>+++ Bootstrap/Step/Updates.pm	(/local/release/trunk)	(revision 1882)
>@@ -21,7 +21,8 @@
> 
>     # Create updates area.
>     if (not -d $updateDir) {
>-        MkdirWithPath('dir' => $updateDir) or die "Cannot mkdir $updateDir: $!";
>+        MozBuild::Util::MkdirWithPath('dir' => $updateDir) 
>+          or die "Cannot mkdir $updateDir: $!";

This should not be necessary. MozBuild::Util needs to |use Exporter;| correctly.

I actually had a patch for this lying around, because I used RunShellCommand... somewhere. I should dig that up.

> sub TestAusCallback { 
>     my $dir = $File::Find::name;
>-    if ($dir =~ /test/) { 
>-        die("Non-test directory found in $testDir/aus2.test: $dir");
>+    if (($dir =~ /beta/) or ($dir =~ /release/)) {
>+        if (not $dir =~ /test/) { 
>+            die("Non-test directory found in $testDir/aus2.test: $dir");
>+        }

Did this change because of partner builds, or?

>-        MkdirWithPath('dir' => $releaseTagDir) 
>+        MozBuild::Util::MkdirWithPath('dir' => $releaseTagDir) 

Ditto Exporter use with all of these.

Also, I saw File::Move::{copy,move} a bunch; same thing there.

>@@ -239,16 +239,20 @@
>         $checkForOnly = '^B ';
>     } elsif ($force and $files) {
>         $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
>-    } else {
>+    } elsif ($force or $branch) {
>         die("Must specify files if branch or force option is used.");
>-    }
>-
>-    # regular tags can be applied to specific files or the whole tree
>-    # if no files are specified.
>-    if ($files) {
>-        $cmd = $cvsCommand . ' ' . $tagName . $files;
>+    } elsif ($force) {
>+        # regular tags can be applied to specific files or the whole tree
>+        # if no files are specified.
>+        $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;
>     } else {
>-        $cmd = $cvsCommand . ' ' . $tagName;
>+        # regular tags can be applied to specific files or the whole tree
>+        # if no files are specified.
>+        if ($files) {
>+            $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;
>+        } else {
>+            $cmd = $cvsCommand . ' ' . $tagName;
>+        }

Hrm... this logic seems a bit confusing to me.

In the elsif ($force) brnach, files will always be null, so I'm not sure why it's used, or why that branch exists. Also, it contradicts the die("Must specify files if branch or force option is used.");

Also, in the force branch, you're not actually forcing anything.

>     # Remove unshipped files and set proper mode on dirs
>-    File::Find::find(\&TrimCallback, $stageDir . '/batch1/prestage-trimmed/');
>+    File::Find::find(
>+      sub {
>+        my $dirent = $File::Find::name;

Regression?

Comment 17

[Robert Helmer [:rhelmer]]

(In reply to comment #16)
> (From update of attachment 246043 [details] [diff] [review] [edit])
> >=== Bootstrap/Step/Updates.pm
> >==================================================================
> >--- Bootstrap/Step/Updates.pm	(/mirror/release/trunk)	(revision 1882)
> >+++ Bootstrap/Step/Updates.pm	(/local/release/trunk)	(revision 1882)
> >@@ -21,7 +21,8 @@
> > 
> >     # Create updates area.
> >     if (not -d $updateDir) {
> >-        MkdirWithPath('dir' => $updateDir) or die "Cannot mkdir $updateDir: $!";
> >+        MozBuild::Util::MkdirWithPath('dir' => $updateDir) 
> >+          or die "Cannot mkdir $updateDir: $!";
> 
> This should not be necessary. MozBuild::Util needs to |use Exporter;|
> correctly.
> 
> I actually had a patch for this lying around, because I used RunShellCommand...
> somewhere. I should dig that up.
> 
> > sub TestAusCallback { 
> >     my $dir = $File::Find::name;
> >-    if ($dir =~ /test/) { 
> >-        die("Non-test directory found in $testDir/aus2.test: $dir");
> >+    if (($dir =~ /beta/) or ($dir =~ /release/)) {
> >+        if (not $dir =~ /test/) { 
> >+            die("Non-test directory found in $testDir/aus2.test: $dir");
> >+        }
> 
> Did this change because of partner builds, or?


Hm. This doesn't seem right.. I'll look over it, thanks.

 
> >-        MkdirWithPath('dir' => $releaseTagDir) 
> >+        MozBuild::Util::MkdirWithPath('dir' => $releaseTagDir) 
> 
> Ditto Exporter use with all of these.
> 
> Also, I saw File::Move::{copy,move} a bunch; same thing there.
> 
> >@@ -239,16 +239,20 @@
> >         $checkForOnly = '^B ';
> >     } elsif ($force and $files) {
> >         $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
> >-    } else {
> >+    } elsif ($force or $branch) {
> >         die("Must specify files if branch or force option is used.");
> >-    }
> >-
> >-    # regular tags can be applied to specific files or the whole tree
> >-    # if no files are specified.
> >-    if ($files) {
> >-        $cmd = $cvsCommand . ' ' . $tagName . $files;
> >+    } elsif ($force) {
> >+        # regular tags can be applied to specific files or the whole tree
> >+        # if no files are specified.
> >+        $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;
> >     } else {
> >-        $cmd = $cvsCommand . ' ' . $tagName;
> >+        # regular tags can be applied to specific files or the whole tree
> >+        # if no files are specified.
> >+        if ($files) {
> >+            $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;
> >+        } else {
> >+            $cmd = $cvsCommand . ' ' . $tagName;
> >+        }
> 
> Hrm... this logic seems a bit confusing to me.
> 
> In the elsif ($force) brnach, files will always be null, so I'm not sure why
> it's used, or why that branch exists. Also, it contradicts the die("Must
> specify files if branch or force option is used.");
> 
> Also, in the force branch, you're not actually forcing anything.


Yeah I think this function is pretty confusing too, I'll take a look at simplifying it and try to write a good unit test for it, since it seems like every time I fix one part I break another.


> >     # Remove unshipped files and set proper mode on dirs
> >-    File::Find::find(\&TrimCallback, $stageDir . '/batch1/prestage-trimmed/');
> >+    File::Find::find(
> >+      sub {
> >+        my $dirent = $File::Find::name;
> 
> Regression?
> 

Explanation in comment 15:

The last one is because (AFAICT) there is no way to pass arguments to a
callback function called by File::Find::find, and I'd like to pass in $gid (and
not make it global). If you have any suggestions let me know.

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to comment #17)
> (In reply to comment #16)
> > (From update of attachment 246043 [details] [diff] [review] [edit] [edit])
> > > sub TestAusCallback { 
> > >     my $dir = $File::Find::name;
> > >-    if ($dir =~ /test/) { 
> > >-        die("Non-test directory found in $testDir/aus2.test: $dir");
> > >+    if (($dir =~ /beta/) or ($dir =~ /release/)) {
> > >+        if (not $dir =~ /test/) { 
> > >+            die("Non-test directory found in $testDir/aus2.test: $dir");
> > >+        }
> > 
> > Did this change because of partner builds, or?

Actually this is ok, because it's returning every parent directory. This only matches on the ones with "release" or "beta" in the name, not (for example) "Firefox" or "1.5.0.8".


> > >@@ -239,16 +239,20 @@
> > >         $checkForOnly = '^B ';
> > >     } elsif ($force and $files) {
> > >         $cmd = $cvsCommand . ' -F ' . $tagName . ' ' . $files;
> > >-    } else {
> > >+    } elsif ($force or $branch) {
> > >         die("Must specify files if branch or force option is used.");
> > >-    }
> > >-
> > >-    # regular tags can be applied to specific files or the whole tree
> > >-    # if no files are specified.
> > >-    if ($files) {
> > >-        $cmd = $cvsCommand . ' ' . $tagName . $files;
> > >+    } elsif ($force) {
> > >+        # regular tags can be applied to specific files or the whole tree
> > >+        # if no files are specified.
> > >+        $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;
> > >     } else {
> > >-        $cmd = $cvsCommand . ' ' . $tagName;
> > >+        # regular tags can be applied to specific files or the whole tree
> > >+        # if no files are specified.
> > >+        if ($files) {
> > >+            $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;
> > >+        } else {
> > >+            $cmd = $cvsCommand . ' ' . $tagName;
> > >+        }
> > 
> > Hrm... this logic seems a bit confusing to me.
> > 
> > In the elsif ($force) brnach, files will always be null, so I'm not sure why
> > it's used, or why that branch exists. Also, it contradicts the die("Must
> > specify files if branch or force option is used.");
> > 
> > Also, in the force branch, you're not actually forcing anything.

I think that branch is just leftover from an earlier change to this section of the code. I just removed it for now.

Comment 19

[Robert Helmer [:rhelmer]]

Attachment: address concerns from previous patch

This addresses concerns from the previous review, and also uses the new Config object (from bug 352230).

Comment 20

[Robert Helmer [:rhelmer]]

Attachment: tested version of last patch

Ran this one through a full test run, found a few problems. I think the only thing we discussed that is missing here is a rewrite of the tag functionality, I believe that what is here now will work for the cases we care about, but I'd like to rewrite it as a separate patch (this has a lot of global changes like Exporter and the Config object so it's a bit heavy).

Comment 21

[J. Paul Reed [:preed]]

Comment on attachment 247203 [details] [diff] [review]
tested version of last patch

>     # regular tags can be applied to specific files or the whole tree
>     # if no files are specified.
>-    if ($files) {
>-        $cmd = $cvsCommand . ' ' . $tagName . $files;
>+    } elsif ($files) {
>+        $cmd = $cvsCommand . ' ' . $tagName . ' ' .$files;
>     } else {
>-        $cmd = $cvsCommand . ' ' . $tagName;
>+        # regular tags can be applied to specific files or the whole tree
>+        # if no files are specified.
>+        if ($files) {
>+            $cmd = $cvsCommand . ' ' . $tagName . ' ' . $files;

The above branch is unreachable.

>+        } else {
>+            $cmd = $cvsCommand . ' ' . $tagName;
>+        }

I'm fine with everything, but this tagging code is still unnecessarily byzantine.

I'd like to rewrite it before using it in production, because I still think it's somewhat confusing, and doesn't express its assumptions correctly.

Let's sit down together and go through it.

Comment 22

[Robert Helmer [:rhelmer]]

Attachment: rewrote CvsTag and made public

same as last time, except:

* rewrote CvsTag to assert conditions first, separate from implementation logic
* made CvsTag public (not _CvsTag)
* CvsTag takes an array of files instead of a scalar

Comment 23

[J. Paul Reed [:preed]]

Comment on attachment 247559 [details] [diff] [review]
rewrote CvsTag and made public

r=preed, with the scalar closing paren change.

Comment 24

[Robert Helmer [:rhelmer]]

Landed. Closing this bug as the basic implementation is there, I'll open other bugs for specific issues.