Closed
Bug 717372
Opened 12 years ago
Closed 11 years ago
The clobber information should live in the tree
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla20
People
(Reporter: ehsan.akhgari, Assigned: BenWa)
References
Details
(Whiteboard: [sheriff-want])
Attachments
(1 file, 1 obsolete file)
2.88 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
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?
Updated•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Yes, this is more or less what was suggested in bug 660015 comment 4.
Comment 2•12 years ago
|
||
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.
Comment 3•12 years ago
|
||
(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•12 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.
Comment 5•12 years ago
|
||
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•12 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•12 years ago
|
||
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 | ||
Comment 8•12 years ago
|
||
Attachment #605434 -
Attachment is obsolete: true
Attachment #605434 -
Flags: review?(khuey)
Attachment #605436 -
Flags: review?(khuey)
Reporter | ||
Comment 9•12 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! :-)
Attachment #605436 -
Flags: review?(khuey) → review+
Comment 10•12 years ago
|
||
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•12 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)
Comment 12•12 years ago
|
||
(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•12 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.
Comment 14•12 years ago
|
||
my reading of comment 11 is that you were waiting for Callek to run some tests. Callek, are you testing?
Comment 15•12 years ago
|
||
(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.
Comment 16•12 years ago
|
||
(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.
Assignee | ||
Comment 18•12 years ago
|
||
Turning green so far: https://tbpl.mozilla.org/?tree=Try&rev=70295c9292be
Updated•12 years ago
|
Whiteboard: [sheriff-want]
Comment 19•12 years ago
|
||
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•12 years ago
|
||
Hmm, why was this not landed before? Was there a problem with the patch, Benoit?
Assignee | ||
Comment 21•12 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 :)
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8cab8f873f84
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 23•11 years ago
|
||
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 → ---
Comment 24•11 years ago
|
||
(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: 12 years ago → 11 years ago
Resolution: --- → FIXED
Comment 25•11 years ago
|
||
(I'll be posting on dev.platform about this tomorrow :-))
Updated•11 years ago
|
Flags: needinfo?(bugspam.Callek)
Updated•9 years ago
|
Flags: needinfo?(bugspam.Callek)
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•