The clobber information should live in the tree

RESOLVED FIXED in mozilla20

Status

defect
RESOLVED FIXED
8 years ago
Last year

People

(Reporter: Ehsan, Assigned: BenWa)

Tracking

unspecified
mozilla20
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sheriff-want])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

8 years ago
I'm sick and tired of people saying "Please clobber when you pull in revision xxx".  We need to do better.

One thing which comes to my mind is to have a counter variable somewhere early in the build system, and cache its previous value somewhere in the objdir file.  Then when starting a build, we can compare the value to the cached one, and if they are different, we'll clobber.

This way every time somebody lands something which requires clobber, they can increment the counter and everybody who pulls that change (both developers and builders) will get the correct behavior.

Does this make sense?
OS: Linux → All
Hardware: x86_64 → All
Yes, this is more or less what was suggested in bug 660015 comment 4.
Doing this as part of the build system seems tricky, but it would have the nice side effect of working on developer machines as well as tinderboxes, which would be cool. We really would need to blow away the objdir here, so we'd have to handle this in client.mk.
(In reply to Ted Mielczarek [:ted, :luser] from comment #2)
> Doing this as part of the build system seems tricky, but it would have the
> nice side effect of working on developer machines as well as tinderboxes,
> which would be cool. We really would need to blow away the objdir here, so
> we'd have to handle this in client.mk.

and like all things handled in client.mk we'd need an OMG STOP error message in the objectdir when you make form @objdir@/Makefile as well (like we do for configure changes)
Assignee

Comment 4

7 years ago
I'm felling inclined to work on this. I'm thinking of having a file such as CLOBBER with a UUID instead of a counter (since the counter could be +1'ed on both branch, thus the second merge wouldn't cause a clobber.

Currently it's a pain because you we don't store clobbers in our history so it makes bisecting difficult. I'm tired of running into this every few months.
Yeah, that's an interesting side-benefit. If you bisect across a clobber, your builds should auto-clobber for you. I'm fine with a UUID inside the file, or just a textual annotation of why we're clobbering (with a bug number) or whatever.
Assignee

Comment 6

7 years ago
I think I like a context description better as well. Bug numbers are great UUID. Also it gives us a great way to track what changes require a clobber and to add a proper dependency later.

We could be strict jerks and require that patch which need a clobber to land with a script change that adds the proper dependency but that wont happen :).
Assignee

Comment 7

7 years ago
Posted patch patch (obsolete) — Splinter Review
Introduce a CLOBBER file to the srcdir/objdir. If these file differ we give an error indicating a clobber is required.

The CLOBBER file should contain a textual description with a bug number so that the reason of the clobber can be traced. This has the benefit of letting us use 'hg log CLOBBER' to see what changes our build system fails to account for at a later date to address them.

The textual description is printed as an error message:
	***
	*	CLOBBER has been modified indicating a clobber is required:
	*	Bug 717372 - The clobber information should live in the tree .
	***

I put the check in configure.in near our check for 'objdir build if a srcdir build exists' because that felt more straight forward then putting it in client.mk. As a result configure.in now depends on CLOBBER.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #605434 - Flags: review?(khuey)
Assignee

Comment 8

7 years ago
Posted patch patchSplinter Review
Attachment #605434 - Attachment is obsolete: true
Attachment #605434 - Flags: review?(khuey)
Attachment #605436 - Flags: review?(khuey)
Reporter

Comment 9

7 years ago
A good next step would be to automatically remove the object directory if the mozconfig specifies a special variable, like MOZ_AUTO_CLOBBER.  This way we can add that to the buildbot mozconfigs, and fix this problem on our build machines once and for all!  :-)
Comment on attachment 605436 [details] [diff] [review]
patch

>+dnl Do not allow building if a clobber is required
>+dnl ==============================================================
>+dnl TODO Make this better, ideally this would clobber automaticially
>+if test -e $_objdir/CLOBBER; then
>+  if test $_topsrcdir/CLOBBER -nt $_objdir/CLOBBER; then

Before this lands can someone tell me what _objdir and _topsrcdir are in a *comm-central* (seamonkey or TB) build?

Expectation comm-central/mozilla/CLOBBER and @objdir@/mozilla/CLOBBER to be the only correct answer.
Attachment #605436 - Flags: feedback?(bgirard)
Assignee

Comment 11

7 years ago
Comment on attachment 605436 [details] [diff] [review]
patch

I don't know. I can hold off landing this patch if you'd like to run some tests to confirm it doesn't cause breakage in SM.
Attachment #605436 - Flags: feedback?(bgirard)
(In reply to Benoit Girard (:BenWa) from comment #11)
> Comment on attachment 605436 [details] [diff] [review]
> patch
> 
> I don't know. I can hold off landing this patch if you'd like to run some
> tests to confirm it doesn't cause breakage in SM.

has this testing happened?
Assignee

Comment 13

7 years ago
Nope, mobile first. I would love someone to take a look at this. Otherwise I'll take a look after the fennec release.
my reading of comment 11 is that you were waiting for Callek to run some tests. 

Callek, are you testing?
(In reply to Brad Lassey [:blassey] from comment #14)
> my reading of comment 11 is that you were waiting for Callek to run some
> tests. 
> 
> Callek, are you testing?

I was not aware the BenWa was expecting me to test it at first, I won't have time for at least the next few days, but we can try landing this and see how it comes out for c-c and backout if need be, imo. CC'ing Mark incase he has time to test/check that.
(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> A good next step would be to automatically remove the object directory if
> the mozconfig specifies a special variable, like MOZ_AUTO_CLOBBER.  This way
> we can add that to the buildbot mozconfigs, and fix this problem on our
> build machines once and for all!  :-)

This needs to be implemented before this can land, otherwise this will prevent all builds from completing once the CLOBBER file is added/updated.
Not necessarily.  It would just tell people that they need to use the buildbot clobberer, as opposed to right now where people merge, see some weird week old message, and ask around on #developers for 20 minutes before they figure out what's up.
Whiteboard: [sheriff-want]
Something else needing-clobber is landing today, resulting in yet another  #developers "please tell everyone to clobber". We should just land this and see how we go.

I've unbitrotted it & tweaked the comment slightly (just minor wording/wrapping fixes), and landed as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cab8f873f84

Callek knows and will be keeping an eye on c-c; setting needinfo on him per IRC request.
Flags: needinfo?(bugspam.Callek)
Target Milestone: --- → mozilla20
Reporter

Comment 20

7 years ago
Hmm, why was this not landed before?  Was there a problem with the patch, Benoit?
Assignee

Comment 21

7 years ago
We wanted to check some compatibility with SeaMonkey and then I decided I would wait until the weekend and it dropped off. I was just afraid to break the tree with this one.

I'm do hope it sticks however :)
https://hg.mozilla.org/mozilla-central/rev/8cab8f873f84
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I think this is going to go badly on the machines putting results on TBPL. Jobs will fail in configure after the CLOBBER file is updated, a false red, then someone will need to go to clobberer and set a clobber, and then rebuild the jobs on TBPL. catlee also mentions this in comment #16, as does Ted in comment #2.
Status: RESOLVED → REOPENED
Flags: needinfo?(bugspam.Callek)
Resolution: FIXED → ---
(In reply to Nick Thomas [:nthomas] from comment #23)
> I think this is going to go badly on the machines putting results on TBPL.
> Jobs will fail in configure after the CLOBBER file is updated, a false red,
> then someone will need to go to clobberer and set a clobber, and then
> rebuild the jobs on TBPL. catlee also mentions this in comment #16, as does
> Ted in comment #2.

This is intentional. Whilst the sheriffs would also like the clobber to eventually be automatic, this patch still makes failures less transparent than they were before.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
(I'll be posting on dev.platform about this tomorrow :-))
Flags: needinfo?(bugspam.Callek)

Updated

7 years ago
Blocks: 822487

Updated

7 years ago
Blocks: 837323
Blocks: 837439
Depends on: 852341
Flags: needinfo?(bugspam.Callek)

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.