Closed Bug 500471 Opened 11 years ago Closed 11 years ago

release automation needs to support buildNumber == 1 w/ relbranchOverride

Categories

(Release Engineering :: General, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bhearsum, Assigned: bhearsum)

References

Details

Attachments

(1 file, 4 obsolete files)

Bootstrap was able to do this fine, but the new Mercurial automation (specifically, ReleaseTaggingFactory) has issues with it. We've been known to require this for firedrill point releases, and it would've been useful for 3.5rc2.
Do you remember what those issues are Ben ? One problem we have with Bootstrap is that it can't bump the version on a reused relbranch, so it'd be great to fix version-bump.pl here too.
Component: Release Engineering: Future → Release Engineering
Priority: -- → P2
(In reply to comment #1)
> Do you remember what those issues are Ben ? One problem we have with Bootstrap
> is that it can't bump the version on a reused relbranch, so it'd be great to
> fix version-bump.pl here too.

That's the main problem here - we're calling version-bump.pl and it fails to find,eg, 1.9.1.2pre on the 1.9.1.1 branch. I'm sure we can fix version-bump.pl and/or our calling of it.
I'm still in the middle of testing this but I wouldn't mind some feedback on the approach. I'm a bit worried that ReleaseTaggingFactory is getting (even more) unreadable. I spent a little bit of time trying to find a better way to do, but there's just so many different cases too handle I couldn't improve it.

I'm not 100% happy with the addition of oldMilestone, just from a maintenance standpoint. The other way to do it would be to not explicitly check for the oldVersion in version-bump.pl, and change it to the new one regardless of the contents. That would avoid the need for oldMilestone. But....I *think* that's its a good thing to be explicit, as it might catch an incorrect branch or changeset early. What do you guys think? Is there a case where this explicitness would not work when it should?
Attachment #390318 - Flags: review?(nthomas)
Attachment #390318 - Flags: review?(catlee)
Alright, so this is basically the same as the first patch, but without bugs :-). I'm more happy with the explicit regexes now. Specifically, they will catch a case where we want to ship off of an existing relbranch, but use the wrong one in the config. It seems unlikely to happen, but...stranger things, etc, etc. If we have a situation where these regexes are making wrong assumptions we can revisit then.
Assignee: nobody → bhearsum
Attachment #390318 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #390462 - Flags: review?(nthomas)
Attachment #390318 - Flags: review?(nthomas)
Attachment #390318 - Flags: review?(catlee)
Comment on attachment 390462 [details] [diff] [review]
fully tested, working build1 + relbranch patch

Kairo, I'm sure you hit these codepaths too, I assume this will work the same for you but you should probably have a look anyways.
Attachment #390462 - Flags: review?(kairo)
Comment on attachment 390462 [details] [diff] [review]
fully tested, working build1 + relbranch patch

No changes to hand those to the tagging factory in release_master.py?

Fore the configs, we probably want the change in seamonkey/release_config.py as well (even though we actually never bump it, but the class would fail if we hand one but not the other), but I can do that when this has landed.

What I wonder a bit about is that we're using oldAppVersion in two slightly different ways, one time for the latest version to update to this one and one time for the version in the source pre-tagging. They probably will usually fall together in the relbranchOverride case at least for Firefox, but it makes it impossible for SeaMonkey and Thunderbird to ride on the same relbranch in comm-central, as one of us needs to set a relbranchOverride and still bump from 'pre' while the old version for updates is something else.
Comment on attachment 390462 [details] [diff] [review]
fully tested, working build1 + relbranch patch

What KaiRo said about mozilla2{,-staging}/release_master.py never passing the new arguments to ReleaseTaggingFactory. Otherwise looks fine for Firefox.
Attachment #390462 - Flags: review?(nthomas) → review-
Attachment #390462 - Attachment is obsolete: true
Attachment #390462 - Flags: review?(kairo)
Comment on attachment 390462 [details] [diff] [review]
fully tested, working build1 + relbranch patch

I'm going to think this through a bit more, but I suspect the only reasonable solution may be to replace anything that matches any version number.
I tested this in all the scenarios mentioned in the block comment. The output is a bit nasty now, but it works.
Attachment #391182 - Flags: review?(nthomas)
Attachment #391182 - Flags: review?(kairo)
Attachment #391182 - Flags: review?(nthomas) → review+
Comment on attachment 391182 [details] [diff] [review]
try #2 - be less picky about the versions

Lets me careful out there.
Comment on attachment 391182 [details] [diff] [review]
try #2 - be less picky about the versions

Looks good to me, this is a reasonable approach, IMHO.
Attachment #391182 - Flags: review?(kairo) → review+
Comment on attachment 391182 [details] [diff] [review]
try #2 - be less picky about the versions

changeset:   333:8afb25990ce6
Attachment #391182 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
This broke 3.6a1 tagging. I think the problem is: ((a|b|(rc))\d+)|(pre)) - which doesn't appear to work for alphas or betas at all (doesn't match a|b *and* pre)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 391182 [details] [diff] [review]
try #2 - be less picky about the versions

backed out due to bustage in changeset:   337:f64f338d9e16
Attachment #391182 - Flags: checked-in+ → checked-in-
I actually backed it out in changeset:   339:d3cf47d835d5
Attached patch fixed version bumping (obsolete) — Splinter Review
While fixing this up I realized that we actually never have 'rc' in the version files, so I removed that part of the regex.

I tested the regex separately with a bunch of scenarios:
bitters-2:tmp bhearsum$ cat test.pl
#!/usr/bin/perl

my $VERSION_REGEXP = '[\d\.]+' # A version number 
                   . '((a|b)\d+)?' # Might be an alpha or beta
                   . '(pre)?'; # Might be pre

if ($ARGV[0] =~ m/^$VERSION_REGEXP$/) {
    print "$ARGV[0]: matched!\n";
}
bitters-2:tmp bhearsum$ perl test.pl 3
3: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6
3.6: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6a1
3.6a1: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6b532
3.6b532: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6rc
bitters-2:tmp bhearsum$ perl test.pl 3.6rc3
bitters-2:tmp bhearsum$ perl test.pl 3.6pre
3.6pre: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6.9.2
3.6.9.2: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6.9.2pre
3.6.9.2pre: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6.9.2b34
3.6.9.2b34: matched!
bitters-2:tmp bhearsum$ perl test.pl 3.6.9.2b34pre
3.6.9.2b34pre: matched!

And I re-bumped mozilla-central with it, with success:
bitters-2:checkouts bhearsum$ perl ~/Mozilla/checkouts/tools/release/version-bump.pl -w mozilla-central -t FIREFOX_3_6a1_RELEASE -a browser -v 3.6a1 -m 1.9.2a1 config/milestone.txt js/src/config/milestone.txt browser/config/version.txt
mozilla-central/config/milestone.txt: ^[\d\.]+((a|b)\d+)?(pre)?$ found
mozilla-central/config/milestone.txt: ^[\d\.]+((a|b)\d+)?(pre)?$ replaced with 1.9.2a1
mozilla-central/js/src/config/milestone.txt: ^[\d\.]+((a|b)\d+)?(pre)?$ found
mozilla-central/js/src/config/milestone.txt: ^[\d\.]+((a|b)\d+)?(pre)?$ replaced with 1.9.2a1
mozilla-central/browser/config/version.txt: ^[\d\.]+((a|b)\d+)?(pre)?$ found
mozilla-central/browser/config/version.txt: ^[\d\.]+((a|b)\d+)?(pre)?$ replaced with 3.6a1
Attachment #391182 - Attachment is obsolete: true
Attachment #393170 - Flags: review?(nthomas)
Attachment #393170 - Flags: review?(nthomas) → review-
Comment on attachment 393170 [details] [diff] [review]
fixed version bumping

>diff -r 3bdeff59b68e release/version-bump.pl
>+my $VERSION_REGEXP = '[\d\.]+' # A version number 
>+                   . '((a|b)\d+)?' # Might be an alpha or beta
>+                   . '(pre)?'; # Might be pre
>+

Have we ever needed to match a single digit like '3' ? Can't recall a case of that, usually it's at least '3.0' in length. Changing the first part to 
   '\d\.\d[\.\d]*'
still matches all the other input. Would be nice to line up the # characters too.
Status: REOPENED → ASSIGNED
(In reply to comment #17)
> (From update of attachment 393170 [details] [diff] [review])
> >diff -r 3bdeff59b68e release/version-bump.pl
> >+my $VERSION_REGEXP = '[\d\.]+' # A version number 
> >+                   . '((a|b)\d+)?' # Might be an alpha or beta
> >+                   . '(pre)?'; # Might be pre
> >+
> 
> Have we ever needed to match a single digit like '3' ? Can't recall a case of
> that, usually it's at least '3.0' in length.

Yeah, I just confirmed this going back to 1.0.

> Changing the first part to 
>    '\d\.\d[\.\d]*'
> still matches all the other input. Would be nice to line up the # characters
> too.

That sounds fine, I'll do those.
Attachment #393170 - Attachment is obsolete: true
Attachment #394510 - Flags: review?(nthomas)
Attachment #394510 - Flags: review?(nthomas) → review+
Comment on attachment 394510 [details] [diff] [review]
slightly better regex, comments aligned

changeset:   353:c24c3c80e0ed

Hopefully we don't need to back this out again :S.
Attachment #394510 - Flags: checked-in+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
(In reply to comment #20)
> (From update of attachment 394510 [details] [diff] [review])
> changeset:   353:c24c3c80e0ed
> 
> Hopefully we don't need to back this out again :S.

I didn't actually push this until today, apparently.
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.