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)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: rhelmer, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Priority: -- → P2
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).
(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.
Attachment #298617 - Flags: review?(nrthomas)
Whiteboard: waiting for review
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+
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
(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
(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 :/
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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 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-
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
Attachment #298617 - Attachment description: [checked in] assert empty verify dir → [backed out][checked in] assert empty verify dir
Whiteboard: waiting for review
per irc discussion, backing out until we have time to fix this right.
Priority: P2 → P3
Whiteboard: needs patch
Assignee: rhelmer → nobody
Status: REOPENED → NEW
Whiteboard: needs patch
Component: Release Engineering → Release Engineering: Future
QA Contact: build → release
Don't think this is worth the effort to fix at this point.
Status: NEW → RESOLVED
Closed: 16 years ago15 years ago
Resolution: --- → WONTFIX
Moving closed Future bugs into Release Engineering in preparation for removing the Future component.
Component: Release Engineering: Future → Release Engineering
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.