Closed
Bug 413178
Opened 16 years ago
Closed 15 years ago
bootstrap should assert that verify dirs do not exist at start
Categories
(Release Engineering :: General, defect, P3)
Release Engineering
General
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: rhelmer, Unassigned)
References
Details
Attachments
(1 file, 3 obsolete files)
1.34 KB,
patch
|
Details | Diff | Splinter Review |
update and l10n verify currently do not check to ensure that the work are is clean before starting. This caused problems in the 3.0b2 release (because a verify was run on the production servers before release) and obscured a regression in bug 412454 (see bug 412454 comment 14 specifically). We should assert at the start of updateverify/l10nverify execute that the working directory does not exist. The staging environments should clean up this area between runs (add to Makefile and call in prestage target?). The only tricky thing is that the clean target would need to run on the slaves, not sure of the best way to handle that.
Reporter | ||
Updated•16 years ago
|
Priority: -- → P2
Comment 1•16 years ago
|
||
The working area you mean is /builds/verify/$app-$version/ ? If so, we'll need to put the check_updates.sh hack into CVS (somehow), because cleaning the workdir will prevent local patches like the 2/4 arguments problem (bug 401608).
Reporter | ||
Comment 2•16 years ago
|
||
(In reply to comment #1) > The working area you mean is /builds/verify/$app-$version/ ? If so, we'll need > to put the check_updates.sh hack into CVS (somehow), because cleaning the > workdir will prevent local patches like the 2/4 arguments problem (bug 401608). Yes, that's the directory I mean. Just as tag and updates error out, so should this. Should be landed on 1.8 branch now, according to that bug! Although in general yes I think we should land stuff in CVS and not do local patches :) Makes it easier to understand and reproduce what happened in previous releases.
Reporter | ||
Comment 3•16 years ago
|
||
Attachment #298617 -
Flags: review?(nrthomas)
Reporter | ||
Updated•16 years ago
|
Whiteboard: waiting for review
Comment 4•16 years ago
|
||
Comment on attachment 298617 [details] [diff] [review] [backed out][checked in] assert empty verify dir (In reply to comment #2) > Should be landed on 1.8 branch now, according to that bug! Although in general > yes I think we should land stuff in CVS and not do local patches :) Makes it > easier to understand and reproduce what happened in previous releases. Yeah, the problem is that the updater in 2.0.0.11 will still expect 4 arguments, so 2.0.0.11 --> 2.0.0.12 update verification will be problematic with this patch. I don't know what the impact of checking that in permanently is. Which reminds me, we should probably pull the update/l10n verify code from a tag too.
Attachment #298617 -
Flags: review?(nrthomas) → review+
Reporter | ||
Comment 5•16 years ago
|
||
Comment on attachment 298617 [details] [diff] [review] [backed out][checked in] assert empty verify dir Bootstrap/Step/Repack.pm Checking in Bootstrap/Step/Updates.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Updates.pm,v <-- Updates.pm new revision: 1.35; previous revision: 1.34 done Checking in Bootstrap/Step/Repack.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Repack.pm,v <-- Repack.pm new revision: 1.23; previous revision: 1.22 done
Attachment #298617 -
Attachment description: assert empty verify dir → [checked in] assert empty verify dir
Reporter | ||
Comment 6•16 years ago
|
||
(In reply to comment #4) > (From update of attachment 298617 [details] [diff] [review]) > (In reply to comment #2) > > Should be landed on 1.8 branch now, according to that bug! Although in general > > yes I think we should land stuff in CVS and not do local patches :) Makes it > > easier to understand and reproduce what happened in previous releases. > > Yeah, the problem is that the updater in 2.0.0.11 will still expect 4 > arguments, so 2.0.0.11 --> 2.0.0.12 update verification will be problematic > with this patch. I don't know what the impact of checking that in permanently > is. Hmm, I'm confused :( When that patch landed on trunk, the verification worked again. Let's followup in that bug, if there is still a problem. > Which reminds me, we should probably pull the update/l10n verify code from a > tag too. Good point, I wonder what else we pull from trunk instead of tag.. I'll see what else I can find and file some bugs.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 7•16 years ago
|
||
(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 298617 [details] [diff] [review] [details]) > > (In reply to comment #2) > > > Should be landed on 1.8 branch now, according to that bug! Although in general > > > yes I think we should land stuff in CVS and not do local patches :) Makes it > > > easier to understand and reproduce what happened in previous releases. > > > > Yeah, the problem is that the updater in 2.0.0.11 will still expect 4 > > arguments, so 2.0.0.11 --> 2.0.0.12 update verification will be problematic > > with this patch. I don't know what the impact of checking that in permanently > > is. > > Hmm, I'm confused :( When that patch landed on trunk, the verification worked > again. Let's followup in that bug, if there is still a problem. Oh, ok, sorry I get it. We shipped with the problem in 2.0.0.11, so we have to live with it for that release; got it. Yeah, that's unfortunate. I think we should probably take the 4-arg patch for this round of releases, and revert it later :/
Reporter | ||
Comment 8•16 years ago
|
||
Since we share the /builds/verify/product-version dir, the previous patch will break on Mac where we do l10n verify and update verify on the same machine (it'd be ideal to do l10n per-platform too). The current patch does not check to make sure that common is removed, we need to rethink the way we do verify checkout if we want to do that (maybe we should just have separate /builds/verify/product-version/{l10n, updates} with a common for each?)
Attachment #298617 -
Attachment is obsolete: true
Attachment #299586 -
Flags: review?(nrthomas)
Reporter | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 9•16 years ago
|
||
Comment on attachment 299586 [details] [diff] [review] look for more specific verify dir I'm confused :-/ Is this patch intended to check for the existence of /builds/verify/$app-$version/l10n/ /builds/verify/$app-$version/updates/ If so, the calls to catfile need fixing up (use comma instead of concatenating). Or if the idea is to change the checkout dir for each verification, then append to $verifyDirVersion instead of making a new var ? That seems like a pretty contained change, but I totally understand if you don't want to make it right before a release. Probably the best move in the long-term though.
Reporter | ||
Comment 10•16 years ago
|
||
No, I did mean comma not dot :) Do you think we should take something like this, or just back the patch out? Most of our confusion has come from common, anyway :/
Attachment #299586 -
Attachment is obsolete: true
Attachment #299767 -
Flags: review?(nrthomas)
Attachment #299586 -
Flags: review?(nrthomas)
Comment 11•16 years ago
|
||
Comment on attachment 299767 [details] [diff] [review] look for more specific verify dir r-. As we agreed, lets back out attachment 298617 [details] [diff] [review] instead, and look at using completely separate directories after the current round of releases.
Attachment #299767 -
Flags: review?(nrthomas) → review-
Reporter | ||
Comment 12•16 years ago
|
||
Checking in Bootstrap/Step/Updates.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Updates.pm,v <-- Updates.pm new revision: 1.36; previous revision: 1.35 done Checking in Bootstrap/Step/Repack.pm; /cvsroot/mozilla/tools/release/Bootstrap/Step/Repack.pm,v <-- Repack.pm new revision: 1.24; previous revision: 1.23 done
Attachment #299767 -
Attachment is obsolete: true
Reporter | ||
Updated•16 years ago
|
Attachment #298617 -
Attachment description: [checked in] assert empty verify dir → [backed out][checked in] assert empty verify dir
Reporter | ||
Updated•16 years ago
|
Whiteboard: waiting for review
Reporter | ||
Comment 13•16 years ago
|
||
per irc discussion, backing out until we have time to fix this right.
Reporter | ||
Updated•16 years ago
|
Priority: P2 → P3
Reporter | ||
Updated•16 years ago
|
Whiteboard: needs patch
Reporter | ||
Updated•16 years ago
|
Assignee: rhelmer → nobody
Status: REOPENED → NEW
Whiteboard: needs patch
Updated•16 years ago
|
Component: Release Engineering → Release Engineering: Future
QA Contact: build → release
Comment 16•15 years ago
|
||
Don't think this is worth the effort to fix at this point.
Status: NEW → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → WONTFIX
Comment 17•14 years ago
|
||
Moving closed Future bugs into Release Engineering in preparation for removing the Future component.
Component: Release Engineering: Future → Release Engineering
Assignee | ||
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
You need to log in
before you can comment on or make changes to this bug.
Description
•