Closed Bug 372764 Opened 13 years ago Closed 13 years ago

Bootstrap - port groom-files[-partners] logic to release automation

Categories

(Release Engineering :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: preed, Assigned: coop)

References

Details

Attachments

(1 file, 3 obsolete files)

There's code in the mofo repo that lays out the ftp areas; it's in perl, and much of it is unused/old, so it'd be good to port this logic to Bootstrap so it's included directly in the release logic, as well as we get rid of the older code that's not really used anymore.

This may just be an exercise in moving perl code around.
Assignee: build → ccooper
Status: NEW → ASSIGNED
Attached patch GroomFiles perl module (obsolete) — Splinter Review
After discussing with rhelmer, I've rolled the regular groom-files script and the partners groom-files script into this single module. No sense maintaining two copies of this logic.
Attachment #260348 - Flags: review?(preed)
Comment on attachment 260348 [details] [diff] [review]
GroomFiles perl module


>      | \.zip            #

We shouldn't handle .zips anymore, because we don't ship them anyway.


>my $linux_installer_re =   # Linux installer file.
>  qr/ ^                    # From start.
>      $bin_prefix_re       # Match on our prefix.
>      \.(linux-i686)       #
>      \.installer          #
>      \.tar.gz             #

This is no longer true for trunk; Linux installers are now .tar.bz2. :-/


>      \.tar.bz2       #

Latent bug? \.tar\.bz2?

>      $               # To end.
>    /x;               #
>
>sub Execute {
>    my $this = shift;
>
>    if (not -d $this->{'dir'}) {
>        $this->Log(msg => "Can't find dir " . $this->{'dir'});
>        return 0;
>    }
>
>    my $config = new Bootstrap::Config();
>
>    my $start_dir = getcwd();
>    chdir($this->{'dir'}) or
>        die("Failed to chdir() to " . $this->{'dir'} . "\n");
>
>    my @files = GatherFiles();
>
>    #printf("%s", Data::Dumper::Dumper(\@files));
>    for my $original_name (@files) {
>        my $new_name = $original_name;
>
>        my $original_dir = dirname($original_name);
>        my $long_name = basename($original_name);
>
>        #printf("%s", Data::Dumper::Dumper($long_name));
>        my @pretty_names = GeneratePrettyName( name => $long_name );
>        #printf("%s", Data::Dumper::Dumper(\@pretty_names));
>
>        my $once = 0;
>        for my $pretty_name (@pretty_names) {
>#            printf("\t\"$long_name\"\n\t  -> \"$pretty_name\"\n\n");
>
>            my $pretty_dirname = dirname($pretty_name);
>            my $pretty_basename = basename($pretty_name);
>
>            if ( ! -e $pretty_name ) {
>                if (! -d $pretty_dirname) {
>                    MkdirWithPath(dir => $pretty_dirname) 
>                        or die "Cannot create $pretty_dirname: $!";
>                    $this->Log(msg => "Created directory $pretty_dirname");
>                }
>                copy($original_name, $pretty_name) or
>                    die("Could not copy $original_name to $pretty_name: $!");
>                $once = 1;
>            }
>        }

Coop: do you know why GeneratePrettyName() could return multiple names, i.e. historically why we do that? I don't think we do it anymore, and it makes this groom files stuff relatively complicated, when it doesn't need to be.

We should rip it out if it's not used anymore.

>    my $output = `find . -type f -o -name \"contrib*\" -prune`;

Minimally, should use RunShellCommand(). Preferably, would use File::Find, with a callback.

>    # Check for distro name.
>    #
>    # NOTE - we are redefining $name here, in order
>    # to take advantage of the regexes below.
>    #
>    if ($name =~ m/ $partial_update_distro_prefix_re /x or
>        $name =~ m/ $distro_prefix_re /x ) {
>        $distro = $4;
>        $name =~ s/\-$distro//g;
>    }
>
>    if (defined($distro)) {
>       $this->Log(msg => "Found distro: $distro");
>    }

We can rip the distro stuff out, I think, if we're not doing distro builds via the automation. Whenever we put the distro stuff in, it broke the non-distro stuff, but no one ever had time to take a good long look about why. That's why there were two scripts.

Something else, generally, to think about:

I'd like to create a MozBuild::Naming module, that would have methods like GetLocaleFromFilename() and GetTypeFromFilename(). It would be cool to make the port of groom files use that, since it'll be used other places, and would centralize a lot of this logic.

But in a crunch, I can port that later.
Attachment #260348 - Flags: review?(preed) → review-
(In reply to comment #3)
> Coop: do you know why GeneratePrettyName() could return multiple names, i.e.
> historically why we do that? I don't think we do it anymore, and it makes this
> groom files stuff relatively complicated, when it doesn't need to be.
> 
> We should rip it out if it's not used anymore.

It's still useful for the xpi langpack part of the conditional where we can return more than one locale xpi.
Attached patch GroomFiles perl module, v2 (obsolete) — Splinter Review
* Removed zip support
* Removed Linux installer support
* fixed Linux regexps to catch both gz and bz2
* use File::Find with Callback, similar to elsewhere in Bootstrap
* removed distro handling

I think MozBuild::Naming is a separate batch of work. Shall I file a separate bug?
Attachment #260348 - Attachment is obsolete: true
Attachment #260414 - Flags: review?(preed)
Comment on attachment 260414 [details] [diff] [review]
GroomFiles perl module, v2


>    $this->{'dir'} = $args{dir};
>    $this->{'newVersion'} = $args{newVersion};
>    $this->{'newVersionShort'} = $args{newVersionShort};
>    $this->{'files'} = ();
>    bless($this, $class);

Are dir, newVersion and newVersionShort optional?

If they're not, we should assert() that they're not.

Nit: on the rhs, $args{'dir'} (like the lhs?)

>    chdir($start_dir) or
>        die("Failed to chdir() to starting directory: " .
>            $start_dir . "\n");
>}

>    if ($#{$this->{'files'}} < 0) {

I've always found this construct hard to read; can we do something like "if (scalar(@{$this->{'files'}}) == 0) {" ? This appears a few times throughout the this patch.

>    ((($dev,$ino,$mode,$nlink,$uid,$gid) = lstat($dirent)) &&
>    -f _
>    ||
>    /^contrib.*\z/s &&
>    ($File::Find::prune = 1))
>        && push @{$this->{'files'}}, $dirent;

Ahh... find2perl.

Could you comment what this is supposed to be searching for, i.e. the original find command; find2perl output is sometimes hard to read.

This looks ok; I hate this logic, because I don't think we use the "return multiple name" logic anymore, even for XPIs.

We should test this logic heavily, though; have we done a test cycle on this?
Attachment #260414 - Flags: review?(preed) → review+
(In reply to comment #6)

> Are dir, newVersion and newVersionShort optional?
> 
> If they're not, we should assert() that they're not.
> 
> Nit: on the rhs, $args{'dir'} (like the lhs?)

Fixed, and fixed.

> I've always found this construct hard to read; can we do something like "if
> (scalar(@{$this->{'files'}}) == 0) {" ? This appears a few times throughout
> the this patch.

I don't find it any easier to read. This is simply trading one retarded perl construct for another. The only way to avoid the hideous perl internals would be to write a sub &array_is_empty(@).

Regardless, changed for the sake of consistency.

> Ahh... find2perl.
> 
> Could you comment what this is supposed to be searching for, i.e. the original
> find command; find2perl output is sometimes hard to read.

Duly commented.
 
> This looks ok; I hate this logic, because I don't think we use the "return
> multiple name" logic anymore, even for XPIs.

Sure, but never having actually run the original groom-files script myself, I have zero use-case context. If the XPI langpack code is no longer used or accurate, it can go away and we can simplify the path checking. The script will at least give us a starting point on which we can iterate.
 
> We should test this logic heavily, though; have we done a test cycle on this?

I've tested the individual bits of the GroomFile module to make sure the regexps and substitutions work, but haven't done end-to-end testing on a full file set yet. Recommended place to do this with a quick-n-dirty testing harness?
(In reply to comment #7)

> Sure, but never having actually run the original groom-files script myself, I
> have zero use-case context. If the XPI langpack code is no longer used or
> accurate, it can go away and we can simplify the path checking. The script will
> at least give us a starting point on which we can iterate.

Agreed. Just wanted to ask if anyone knew what that weirdness was for. :-)

> I've tested the individual bits of the GroomFile module to make sure the
> regexps and substitutions work, but haven't done end-to-end testing on a full
> file set yet. Recommended place to do this with a quick-n-dirty testing
> harness?

We sorta do. There's staging-linux-tbox; you could grab a prestage directory from stage, and then try to run the stage step and see what happens.

Attachment #260347 - Flags: review?(preed) → review+
At preed's suggestion, I've eliminated GroomFiles.pm as a substep and moved the logic up into Stage.pm.

This has been tested locally and on staging-linux-tbox.
Attachment #260347 - Attachment is obsolete: true
Attachment #260414 - Attachment is obsolete: true
Attachment #262263 - Flags: review?(preed)
Comment on attachment 262263 [details] [diff] [review]
Move the logic from GroomFiles.pm into Stage.pm

Looks good; we should move all the regexes and stuff out to a common place so others (patcher, etc.) can use it (I'm thinking MozBuild::Naming), but we can do that after we have this working.

r=preed.
Attachment #262263 - Flags: review?(preed) → review+
Checking in Stage.pm;
/cvsroot/mozilla/tools/release/Bootstrap/Step/Stage.pm,v  <--  Stage.pm
new revision: 1.11; previous revision: 1.10
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.