ReleaseUpdatesFactory fails (but doesn't halt) if the patcher config file has already been bumped

RESOLVED FIXED

Status

Release Engineering
General
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: bhearsum, Assigned: rail)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [simple])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
This is pretty unfortunate when we have to re-run the factory for some reason. In the past we've worked around it by undoing the changes it checked in so it can do it again - we should really just have it detect when it's been bumped, and let it skip doing it a second time if that's the case.
(Reporter)

Updated

9 years ago
Assignee: bhearsum → nobody
Component: Release Engineering → Release Engineering: Future
(Reporter)

Updated

9 years ago
Status: ASSIGNED → NEW
(Reporter)

Updated

9 years ago
Blocks: 478420
(Reporter)

Updated

9 years ago
No longer blocks: 433930

Comment 1

8 years ago
Just hit this again during the release process for 3.5.4. It's not fatal any longer, i.e. we just plow ahead, but the build step is still marked as failed.

Updated

8 years ago
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86 → All

Comment 2

8 years ago
Mass move of bugs from Release Engineering:Future -> Release Engineering. See
http://coop.deadsquid.com/2010/02/kiss-the-future-goodbye/ for more details.
Component: Release Engineering: Future → Release Engineering

Comment 3

8 years ago
Not sure if it is exactly the same problem what I have hit on 3.6.2.
http://production-master.build.mozilla.org:8010/builders/updates/builds/24

> perl ../tools/release/patcher-config-bump.pl -p firefox -r Firefox -v 3.6.2 -a 3.6.2 -o 3.6rc2 -b 3 -c patcher-configs/moz192-branch-patcher2.cfg -t stage-old.mozilla.org -f ftp.mozilla.org -d download.mozilla.org -l shipped-locales -u
...
> Use of uninitialized value in substitution (s///) at ../tools/release/patcher-config-bump.pl line 127.
> Use of uninitialized value in pattern match (m//) at ../tools/release/patcher-config-bump.pl line 129.
> Use of uninitialized value in pattern match (m//) at ../tools/release/patcher-config-bump.pl line 129.
> Use of uninitialized value in concatenation (.) or string at ../tools/release/patcher-config-bump.pl line 140.
> ASSERT: BumpFilePath() - Unknown file type for '' at ../tools/release/patcher-config-bump.pl line 140.
That's completely unrelated.

Updated

8 years ago
Whiteboard: [simple]
(Reporter)

Updated

8 years ago
Priority: P3 → P5
(Reporter)

Comment 5

8 years ago
(In reply to comment #1)
> Just hit this again during the release process for 3.5.4. It's not fatal any
> longer, i.e. we just plow ahead, but the build step is still marked as failed.

Adjusting the summary based on this.

Hard to know what to do. We don't want to set failOnFailure/warnOnFailure = False because that could mask real failures.
Summary: ReleaseUpdatesFactory fails if the patcher config file has already been bumped → ReleaseUpdatesFactory fails (but doesn't halt) if the patcher config file has already been bumped
(Assignee)

Updated

8 years ago
Assignee: nobody → rail
(Assignee)

Comment 6

8 years ago
Does anybody remember the reason why it fails?

IIRC, it fails because the CVS tree has uncommitted changes (not committed during the previous failed run). In this case cleaning up the CVS directory will solve the problem.
(Reporter)

Comment 7

8 years ago
(In reply to comment #6)
> Does anybody remember the reason why it fails?
> 
> IIRC, it fails because the CVS tree has uncommitted changes (not committed
> during the previous failed run). In this case cleaning up the CVS directory
> will solve the problem.

I can't remember exactly, or find record of a run that failed like this but I'm pretty sure it was due to the diff or commit step returning non-zero
(Assignee)

Comment 8

8 years ago
(In reply to comment #7)
> I can't remember exactly, or find record of a run that failed like this but I'm
> pretty sure it was due to the diff or commit step returning non-zero

Yeah, the second bump doesn't actually bump (zero diff). diff_patcher_config fails if the exit code is 0 (ignoreCodes=[1]).

As a possible solution we can set ignoreCodes=[0,1]. In this case we'll ignore errors of the first run which should exit with 1, not 0.

Another solution is play around self.build.reason (parse it somehow and decide if this is the first run or forced from web) and doStepIf.

Comment 9

8 years ago
I'm pretty sure I had "failures" because of an empty diff. I guess a better fail criteria would be if the after-bump file doesn't contain the right version anywhere or so, but checking for a non-empty diff is not a too good error criteria.
(Assignee)

Updated

8 years ago
Priority: P5 → P3
(Assignee)

Comment 10

8 years ago
Created attachment 466583 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

(In reply to comment #9)
> I'm pretty sure I had "failures" because of an empty diff. I guess a better
> fail criteria would be if the after-bump file doesn't contain the right version
> anywhere or so, but checking for a non-empty diff is not a too good error
> criteria.

Good point. Probably grepping for the current version if diff returns 0 is not a bad idea.

Please take a look at the patch.
Attachment #466583 - Flags: feedback?
(Assignee)

Updated

8 years ago
Attachment #466583 - Flags: feedback? → feedback?(kairo)

Comment 11

8 years ago
Comment on attachment 466583 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

Basically, this looks good, but the "firefox" in there is wrong, we have a variable for that so other apps (or rebranded alphas) work as well :)
f+ for the idea of this as far as I can decipher the code but please use the var (this.appName?) in a final patch.
Attachment #466583 - Flags: feedback?(kairo) → feedback+
(Assignee)

Comment 12

8 years ago
Created attachment 466947 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

(In reply to comment #11)

> Basically, this looks good, but the "firefox" in there is wrong, we have a
> variable for that so other apps (or rebranded alphas) work as well :)
> f+ for the idea of this as far as I can decipher the code but please use the
> var (this.appName?) in a final patch.

Good point. self.productName is what you want.

Decipher: 
if cvs diff returns 0 (no changes):
  # probably second run
  if grep returns 0:
    # already have needed  version
    do nothing, return 0 which is ignored
  else:
    # something wrong!
    return 2, which is not good and not ignored (fail!)
else:
  # we have local changes
  cvs diff returns 1, which is good and ignored
Attachment #466583 - Attachment is obsolete: true
Attachment #466947 - Flags: review?(catlee)
(Assignee)

Updated

8 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Priority: P3 → P2
Comment on attachment 466947 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

I don't think this will work...did you test this? Doing string interpolation with WithProperties the way you're doing it will result in weirdness.

'cvs diff -u "%s"' % WithProperties(self.patcherConfigFile) will result in something like
'cvs diff -u "<WithProperties instance at 0x12345678>"'
Attachment #466947 - Flags: review?(catlee) → review-
(Assignee)

Comment 14

8 years ago
Created attachment 467408 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

(In reply to comment #13)
> Comment on attachment 466947 [details] [diff] [review]
> Grep for the current complete MAR if diff returns 0
> 
> I don't think this will work...did you test this? Doing string interpolation
> with WithProperties the way you're doing it will result in weirdness.

I didn't test it at all. Just wanted to get some feedback/review before doing something in staging. Sometimes it may took a lot of time (like today) to get everything ready for staging run, especially for a release related factory.
 
> 'cvs diff -u "%s"' % WithProperties(self.patcherConfigFile) will result in
> something like
> 'cvs diff -u "<WithProperties instance at 0x12345678>"'

Ouch. Yeah, interpolation, then WithProperties. 

I also replaced '%', which should be quoted twice and become '%%%%' (for interpolation and withProperties), with '.'.

Test run output is here: http://pastebin.mozilla.org/771392
Attachment #466947 - Attachment is obsolete: true
Attachment #467408 - Flags: review?(catlee)

Updated

8 years ago
Attachment #467408 - Flags: review?(catlee)
(Assignee)

Comment 15

8 years ago
Created attachment 469453 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

No need to use WithProperties for self.patcherConfigFile, it's a plain string.
Attachment #467408 - Attachment is obsolete: true
Attachment #469453 - Flags: review?(catlee)

Updated

8 years ago
Attachment #469453 - Flags: review?(catlee) → review+
(Assignee)

Comment 16

8 years ago
Comment on attachment 469453 [details] [diff] [review]
Grep for the current complete MAR if diff returns 0

http://hg.mozilla.org/build/buildbotcustom/rev/7c13fa68818f
Attachment #469453 - Flags: checked-in+
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.