Closed Bug 837323 Opened 7 years ago Closed 7 years ago

Updating CLOBBER file should clobber automatically, not just tell users to clobber

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla22

People

(Reporter: jruderman, Assigned: gps)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, sheriffing-P3)

Attachments

(1 file, 2 obsolete files)

No description provided.
Thank you for filing this :-)
Keywords: sheriffing-P3
Why?
So apparently some people put valuable things in their objdir, and they would object to their objdir suddenly going away without any explicit action...
Oh, it's not the bug I thought it was going to be, a releng automation bug to have buildslaves clobber automatically.
Yeah I'm not convinced this is a good idea.  I think it's better to require explicit action/consent before rm -rfing stuff.
Filed bug 837439 for buildbot doing it automatically.
> Why?

Because I often type "hg pull -u && make -f client.mk" and walk away from my computer.

> So apparently some people put valuable things in their objdir

Why are people doing that?

Would it make sense for auto-clobbering to be a mozconfig option?
As a word of warning I had people annoyed at me because they were seeing the CLOBBER message so I can imagine deleting the objdir might also lead to irrational complaints.
This would be a perfect setting for mach if mach ever grows config files back.
(In reply to comment #7)
> > So apparently some people put valuable things in their objdir
> 
> Why are people doing that?

Because they don't know better.  ;-)

> Would it make sense for auto-clobbering to be a mozconfig option?

SGTM.
(In reply to Justin Dolske [:Dolske] from comment #2)
> Why?

Because on our buildbot builds the clobberer page has to be used for every tree after it has merged in a change to the CLOBBER file.

Agree this may be suboptimal for local builds, so shouldn't be enabled by default. (but anyone for whom it would be useful locally, eg people bisecting, can still enable it via their local mozconfig)

We should:
a) Add a configure option MOZ_AUTO_CLOBBER, which makes https://hg.mozilla.org/integration/mozilla-inbound/file/02822f5df3c8/configure.in#l129 auto-clobber instead of erroring out. (Not enabled by default).
b) Mention MOZ_AUTO_CLOBBER in the existing error message, to increase awareness.
c) Add MOZ_AUTO_CLOBBER to all of our in-tree mozconfigs (used by buildbot).
Duplicate of this bug: 837439
That won't help with bisecting, unless the bisection's starting point falls after the changeset in which this bug gets fixed.  ;-)
(In reply to :Ehsan Akhgari (Away 2/7-2/15) from comment #13)
> That won't help with bisecting, unless the bisection's starting point falls
> after the changeset in which this bug gets fixed.  ;-)

No reason not to fix it now, so bisecting won't be less painful in 6 months time...
Duplicate of this bug: 840571
Are there really a significant number of people who want to preserve their objdirs after hg up, such that we should cater to them by default?

I would suggest that in the general case objdirs should be treated as a cache, rather than anything where we care about preserving data at the expense of usability and speed (every time someone doesn't catch that their ./mach build failed due to a clobber after they walked away, we lose developer time to a build that should be done/well in progress).
(In reply to comment #16)
> Are there really a significant number of people who want to preserve their
> objdirs after hg up, such that we should cater to them by default?

I don't think so personally.
I agree. We can announce this to dev.platform, wait a few days and fix this.
I agree with the sentiment that we should treat objdirs as a cache. I would go further and say we should treat them more like black boxes. However, developers are used to working from the objdir (running |make| etc), so this will be a long battle.

I generally agree auto clobber should be the default. However, before we make it the default, I would like to see:

1) A backdoor so people can disable auto clobber in mozconfigs or elsewhere. People insist on control and they /really/ don't like changing their development workflow.

2) Detection that cwd is not inside the objdir. If we blow out the cwd via auto clobber, the build will not be happy (although the rm will fail on Windows because the shell in the directory counts as an open file IIRC).

I have a lesser concern over the "clobber really not needed" scenario. As I discovered a few days ago, if you change CLOBBER, push, then back out, Mercurial will touch the CLOBBER file even if your update passes over any actual changes to CLOBBER. This causes make to think a clobber is needed. We should consider performing stronger, content-based validation instead of merely mtime and not insist on a clobber if CLOBBER hasn't actually been changed.
I think choosing a more conservative default behavior if cwd is a descendent of the objdir makes a ton of sense.
There was another instance of the tree being burnt today, since it wasn't clear that by changing the CLOBBER file, it also required use of the clobberer.

{
philor: only half wrong - if you touch CLOBBER, you have to touch https://secure.pub.build.mozilla.org/clobberer/?branch=mozilla-inbound too
smaug: what?
smaug hopes that is a joke
philor: but then, if you do it during the day, really you have to close the tree, wait until every pending build is picked up, yell at people to not retrigger any builds, touch the clobberer, and then land
philor: nope
philor: more like "the lurching, halting, random drunken way we roll"
Jesse: why are "wait until every pending build is picked up, yell at people to not retrigger any builds" needed?
philor: because not only does CLOBBER not clobber, but the clobberer only clobbers the next job that each slave runs, whether that's on a push before the one you want clobbered or not
smaug: so how is one supposed to know that https://secure.pub.build.mozilla.org/clobberer/?branch=mozilla-inbound should be used?
philor: so you hit the clobberer, my push finally gets a build, someone triggers a build ten down, you push, and those two slaves do builds ten and thirty pushes later, but they already clobbered
philor: one is supposed to know by having this pleasant discussion with me, I think
}

I've landed bug 852341 to try and increase awareness, however it still doesn't help with the pending jobs/push races story.
Okay, that just reinforces my sense that we should fix this ASAP.  Can we agree on the following?

1) If MOZ_NO_AUTO_CLOBBER defined in the mozconfig, show the current message and require explicit invocation of clobber.
2) If cwd is inside the objdir, fail out and tell a user to change to the top-level to proceed.
3) Otherwise, clobber automagically.

I think that given comment 21 we need to just move forward on this.
Although I am very excited and agree with comment 22, given comment 21 I hope that that this is documented both in the interim and beyond (very casually looking at the first few results of https://developer.mozilla.org/en-US/search?q=clobber doesn't give me anything that looks like what I would care about).
(In reply to comment #22)
> Okay, that just reinforces my sense that we should fix this ASAP.  Can we agree
> on the following?
> 
> 1) If MOZ_NO_AUTO_CLOBBER defined in the mozconfig, show the current message
> and require explicit invocation of clobber.
> 2) If cwd is inside the objdir, fail out and tell a user to change to the
> top-level to proceed.
> 3) Otherwise, clobber automagically.
> 
> I think that given comment 21 we need to just move forward on this.

r=me.
Complication: CLOBBER is detected from configure. configure is always run from the object directory. We'll need to move the invocation of the CLOBBER detection into client.mk (and mach by extension). This will mean separating the CLOBBER logic into a standalone script or executable Python file. Good times.

Anyway, I think I can tackle this before I get on a bus to go home...
Status: NEW → ASSIGNED
(In reply to Ed Morley [:edmorley UTC+0] from comment #21)
> smaug: so how is one supposed to know that

The person reviewing the patch shouldn't review something that they don't understand to be fair. But I agree we need to make this better.
(In reply to Mike Connor [:mconnor] from comment #22)
> 2) If cwd is inside the objdir, fail out and tell a user to change to the
> top-level to proceed.

(In reply to Gregory Szorc [:gps] from comment #25)
> Complication: CLOBBER is detected from configure. configure is always run
> from the object directory. We'll need to move the invocation of the CLOBBER
> detection into client.mk (and mach by extension). This will mean separating
> the CLOBBER logic into a standalone script or executable Python file. Good
> times.

If cwd is the objdir (not a subdirectory), could we clear out the contents of the objdir without removing the objdir itself?

That case will become more common because http://gregoryszorc.com/blog/2013/03/03/omnipresent-mach/ is making it more pleasant.
Attached patch auto clobber, v1 (obsolete) — Splinter Review
I need to get on a bus. This partially works. I may or may not see it to the finish line.
(In reply to Jesse Ruderman from comment #27)
> If cwd is the objdir (not a subdirectory), could we clear out the contents
> of the objdir without removing the objdir itself?

Possibly. Complicates things slightly, but should be doable.
 
> That case will become more common because
> http://gregoryszorc.com/blog/2013/03/03/omnipresent-mach/ is making it more
> pleasant.

I've also said many places that I think the objdir should be treated more and more as a black box. I'd rather people work from the srcdir than the objdir. As someone who hacks on the build system, I want the flexibility to make the build system faster and better without worrying about those changes disrupting existing workflows. That's why I've been trying to build abstractions layers like mach: the interface can remain the same while the implementation completely changes. "Every problem in computer science can be solved by another level of indirection."
Which is why I solved the problem of only being able to build one objdir per srcdir by adding an extra level of indirection, i.e. building indirectly from the objdir instead of directly in the srcdir.
Attached patch auto clobber, v2 (obsolete) — Splinter Review
So, one of my long-term plans is to eventually move the logic from client.mk into Python so we have a nice Python API into building the tree. Since I was writing the logic for clobbering in Python, I decided to bite the bullet and put the "is clobber required" code in mozbuild. As a bonus, it should be easier to write tests for this!

So, the attached patch works. Kinda. client.mk seems to work fine. Although mach doesn't work. I believe the reason is mach will chdir() as part of invoking client.mk. Under mach, getcwd() always returns topsrcdir. Oy.

Furthermore, mozfile isn't available when run under client.mk. I'd *really* like to get mozfile working through client.mk because it will result in a free performance win for buildbot jobs (since their clobber logic is suboptimal - see bug 851270).
Attachment #726447 - Attachment is obsolete: true
Comment on attachment 726963 [details] [diff] [review]
auto clobber, v2

>+    auto = False if os.environ.get('NO_AUTOCLOBBER', False) else True

Nit: The naming of this var makes things double-negative to me, would suggest:

    auto = True if os.environ.get('MOZ_DO_AUTOCLOBBER', False) else False

Unless you *really* do intend to make AUTOCLOBBER the default? instead of an environ var or mk flag sourced from mozconfig. in which case I disagree with the choice.
I do intend to make auto clobber the default. It's a usability win and makes trees "just work" without any mozconfig changes.

If people get bit by it, they can disable it locally in their mozconfigs.

Is this not what was decided in this bug?
Attached patch Auto clobber, v3Splinter Review
OK. I think this patch accomplishes what we want. It even has tests!

When building through client.mk we will always check if a clobber is required. If it is, we automatically clobber unless:

1) The cwd is a subdirectory of topobjdir
2) mk_add_options NO_AUTOCLOBBER is defined in the mozconfig

#1 should only occur for people doing |make -f ../../client.mk| (or some variant) or for people with mach in their $PATH who do something like |mach build .|. #1 is a defense against Windows, where we can't remove directories we are executing under. We could code this to be Windows specific, but meh (it's an edge case).

Please note that we now check clobber status in client.mk instead of in configure. If people were bypassing client.mk (they shouldn't be doing this - even mach uses client.mk), configure will no longer detect a clobber is needed. Honestly, I don't care about supporting these people. The build system is more complicated than "./configure && make" and if people have opted to ignore this fact, they have chosen to remove the safety and they should live with the consequences of living with a footgun with a very sensitive trigger.
Assignee: nobody → gps
Attachment #726963 - Attachment is obsolete: true
Attachment #731019 - Flags: review?(ted)
Comment on attachment 731019 [details] [diff] [review]
Auto clobber, v3

Review of attachment 731019 [details] [diff] [review]:
-----------------------------------------------------------------

Excuse the drive-by... :-)

::: python/mozbuild/mozbuild/controller/clobber.py
@@ +113,5 @@
> +
> +          - Whether a clobber was/is required.
> +          - Whether a clobber was performed.
> +          - The reason why the clobber failed or could not be performed. This
> +            will be None if no clobber is required.

nit: "This will be None for success or if no clobber was required."
Comment on attachment 731019 [details] [diff] [review]
Auto clobber, v3

Review of attachment 731019 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, just a few nits and one suggestion.

Could we just add an error condition like we have for when configure is out of date?
http://mxr.mozilla.org/mozilla-central/source/Makefile.in#46

That would stop people who configure and make manually from shooting themselves in the foot, at least.

::: python/mozbuild/mozbuild/controller/clobber.py
@@ +12,5 @@
> +
> +try:
> +    from mozfile.mozfile import rmtree
> +except ImportError:
> +    rmtree = None

This is always going to fail in the normal case (running from client.mk), isn't it? That's kind of unfortunate. It would be nice if we could hook up the fast path (at least on Windows) to work in the common case.

@@ +84,5 @@
> +        Each line is stripped of leading and trailing whitespace.
> +        """
> +        with open(self.src_clobber, 'rt') as fh:
> +            lines = [l.strip() for l in fh.readlines()]
> +            lines = [l for l in lines if len(l) and not l.startswith('#')]

I usually just write "if l". Also you could just return this list comprehension directly.

@@ +163,5 @@
> +    def _rmtree(self, path):
> +        if rmtree:
> +            rmtree(path)
> +        else:
> +            subprocess.check_call(['rm', '-rf', path])

Why not just call shutil.rmtree?

::: python/mozbuild/mozbuild/test/controller/test_clobber.py
@@ +118,5 @@
> +        self.assertGreaterEqual(os.path.getmtime(c.obj_clobber),
> +            os.path.getmtime(c.src_clobber))
> +
> +    def test_objdir_is_srcdir(self):
> +        """If topobjdir is the topobjdir, refuse to clobber."""

I assume this is supposed to read "If topobjdir is the topsrcdir."
Attachment #731019 - Flags: review?(ted) → review+
I addressed all review feedback with the exception of sys.path kludging for mozfile. IMO shutil.rmtree is a good enough fallback. mozfile may be a little faster and may not fail in some scenarios. If shutil.rmtree lets us down, we can file a follow-up to try to use mozfile.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a61aa08ab4ac
Target Milestone: --- → mozilla22
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/a61aa08ab4ac
https://hg.mozilla.org/mozilla-central/rev/a6ab3e11f721
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 731019 [details] [diff] [review]
Auto clobber, v3

My workflow involves having multiple object directories with a symlink pointing to the one I'm currently using. That way I just point everything (mozconfigs, gdb config files, etc.) to the symlink and it all just works (TM). I haven't tried out this autoclobber thing yet, but looking at the patch, it looks like this part:

>+        print('Automatically clobbering %s' % self.topobjdir, file=fh)
>+        try:
>+            if cwd == self.topobjdir:
>+                for entry in os.listdir(self.topobjdir):
>+                    full = os.path.join(self.topobjdir, entry)
>+
>+                    if os.path.isdir(full):
>+                        self._rmtree(full)
>+                    else:
>+                        os.unlink(full)
>+
>+            else:
>+                self._rmtree(self.topobjdir)

will probably blow away my symlink instead of the actual objdir, and then create a new objdir where the symlink used to be. I realize my workflow is an edge case, but it could probably be handled by removing the "else" part of the above block and just using the "if" body always. I'm not saying we should actually do that yet, but I wanted to document this somewhere so that if other people have other reasons to do this as well, then perhaps we can consider it.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #39)
> Comment on attachment 731019 [details] [diff] [review]
> Auto clobber, v3
> 
> My workflow involves having multiple object directories with a symlink
> pointing to the one I'm currently using. That way I just point everything
> (mozconfigs, gdb config files, etc.) to the symlink and it all just works
> (TM). I haven't tried out this autoclobber thing yet, but looking at the
> patch, it looks like this part:
> 
> >+        print('Automatically clobbering %s' % self.topobjdir, file=fh)
> >+        try:
> >+            if cwd == self.topobjdir:
> >+                for entry in os.listdir(self.topobjdir):
> >+                    full = os.path.join(self.topobjdir, entry)
> >+
> >+                    if os.path.isdir(full):
> >+                        self._rmtree(full)
> >+                    else:
> >+                        os.unlink(full)
> >+
> >+            else:
> >+                self._rmtree(self.topobjdir)
> 
> will probably blow away my symlink instead of the actual objdir, and then
> create a new objdir where the symlink used to be. I realize my workflow is
> an edge case, but it could probably be handled by removing the "else" part
> of the above block and just using the "if" body always. I'm not saying we
> should actually do that yet, but I wanted to document this somewhere so that
> if other people have other reasons to do this as well, then perhaps we can
> consider it.

Please file a follow-up to handle symlinked objdirs if it's an issue for you.
Depends on: 858752
Backed out due to bug 858752.
https://hg.mozilla.org/integration/mozilla-inbound/rev/27211859cb34
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Sadness. Is there not a middle ground here? I would argue that autoclobber on local dev machines with breakage on builders is better than no autoclobber at all. If we keep landing and backing this out, it's going to keep changing developers' workflow. Whereas if we say "autoclobber can't be trusted - clobber before you push," the only people affected are those who push (a smaller audience). Annoying, sure. But, I'd argue its at the same level we were before where people forgot to clobber before a push.
Autoclobber failed on my local tree this morning.  It's not just automation that has a problem ...
Having multiple build failures *per-push* is hardly an acceptable situation for our builders to be in.
And clobbering the buildslaves is not a solution, autoclobber was hitting bug 858752 and causing the build to fail on clobberer-clobbered runs (thus the talk in the bug about NSS creating unwriteable exports, though apparently it "always" has, so perhaps we've "never" been able to clobber successfully on Windows, and we just didn't know enough to care until now).
How about defining the clobber command in the mozconfig?

Mac/Linux mozconfigs could use MOZ_AUTO_CLOBBER='rm -rf $OBJDIR'
Windows mozconfigs could use MOZ_AUTO_CLOBBER='rd //s //q $OBJDIR'

Symlinked objdirs could use MOZ_AUTO_CLOBBER='rm -rf $OBJDIR/*'

And I could use MOZ_AUTO_CLOBBER='cp $SRCDIR/CLOBBER $OBJDIR' ;-)
Depends on: 859349
(In reply to Phil Ringnalda (:philor) from comment #46)
> (thus the talk in the bug about NSS creating unwriteable exports, though
> apparently it "always" has, so perhaps we've "never" been able to clobber
> successfully on Windows, and we just didn't know enough to care until now).

We've never used python's rmtree to clobber until landing this bug.
https://hg.mozilla.org/integration/mozilla-inbound/rev/962ec303ced2

Now with 100% guarantee of using mozfile and hopefully avoiding the permissions issue (since mozfile's rmtree employs light magic beyond shutil's). IRC review by glandium.
https://hg.mozilla.org/mozilla-central/rev/962ec303ced2
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 863091
Blocks: clobber
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.