Closed Bug 863091 Opened 11 years ago Closed 11 years ago

Make auto clobber opt-in rather than opt-out

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla23

People

(Reporter: gps, Assigned: emorley)

References

Details

Attachments

(1 file)

+++ This bug was initially created as a clone of Bug #837323 +++

Auto clobber without any warning has irked some people. We should probably add a delay, big warning message, etc if we are to have auto clobber remain the default behavior. It's possible that we may change behavior so auto clobber is only enabled in automation. Although, that will annoy the fans of auto clobber. (We can't satisfy everyone here.)

My personal opinion is we should retain auto clobber everywhere but make it safer for local developers.

I propose a big warning message combined with a 10s delay if a build is performed on an interactive terminal and a clobber is required. If no user action is taken, we automatically clobber. The delay can optionally be disabled somehow (through mozconfig variable, etc).

If there are objections to this proposal, please propose an alternative.
People didn't like the clobber message because they type the command and left. I think the 10s delay will also be missed so IMO this isn't going to make things worse.

People will be unhappy as we move towards something better but I don't think we can please everyone. My 2cents is WONTFIX but take it wit a grain of salt. I'd much rather see you work on build performance :).
isn't going to make things better*
I entirely agree that a WONTFIX here is a better alternative.  We already have sanity checks in place to see if the objdir is the cwd (right?), and if somebody comes up with another idea along those lines, we can add it too.  But I really doubt that a 10s warning message can grab the attention of people more than your dev.platform posting did.
I'm also on board with WONTFIX. I haven't seen specific complaints, but I don't think grumbling should prevent forward progress towards a sane build system.
(In reply to Mike Connor [:mconnor] from comment #4)
> I'm also on board with WONTFIX. I haven't seen specific complaints, but I
> don't think grumbling should prevent forward progress towards a sane build
> system.

Arguably, a sane build system doesn't need clobber at all. ;)
My original intention was that hg log CLOBBER would tell you what needs fixing.
IMO we should either:
a) WONTFIX
b) Switch to opt-in rather than opt-out -- and make our automation opt-in + mention the mozconfig variable that needs to be set as part of the "needs clobber" message. That way after one "dammit! walked away and the stupid thing needed a clobber" moment, people can choose to opt-in.

Anything other than that (eg a delay that could be easily missed) doesn't seem worth the time/effort.
(In reply to Mike Hommey [:glandium] from comment #5)

> Arguably, a sane build system doesn't need clobber at all. ;)

Well, yes. But in the absence of full sanity, we should strive for mostly-sane. :)

Having now read the dev.platform thread, I'm even more strongly in favour of WONTFIX, but I think it's Greg's call as build system owner to decide what guarantees, or non-guarantees, should be made about the sanctity of the objdir.  If the choice is to treat the objdir as a volatile cache, we should make sure that's well documented.
(In reply to Mike Connor [:mconnor] from comment #8)
> If the choice is to treat the objdir as a volatile cache, we should
> make sure that's well documented.

I don't think that's realistic at this point; there are just too many people whose workflows are incompatible with doing that.
I am not on board with WONTFIX!

This is a horrible idea -- I don't want any script, no matter how well meaning, deleting anything from my local filesystem.  Having someone able to touch a file in our source repository that causes a large directory tree to get nuked on my local system is not cool.  I keep various transient but still useful things in there, and having them deleted underneath me would cause rage.  (Moot point; I work on Windows, so I'll never get autoclobber anyway because of the cwd thing, but still.)

Let people opt in to autoclobber; turn it on by default on the build machines to solve that problem.  Educate people on how to turn them it on if they want to, but give them the info so they know what will happen.

People who type a command and then immediately walk away without looking at the output that happens within the first second is not something we should optimize for; it's not like the clobber check happens late in the build process.
If wonder if the people holding the "it shouldn't delete anything" opinion realize that the build system today and for much of history has deleted large swaths of the object directory at various stages of the build.
I'm aware, but it's subtrees (e.g. dist/include) that you are unlikely to have anything stored in.  The toplevel objdir, dist/, and dist/bin are the ones that I'm most concerned about (dist/ for existing packaged builds, dist/bin for log files/dumps/whatever while running).
Is there any specific objection to making autoclobber opt-out? I should think that for most people autoclobber is no big deal, and an opt-out setting in mozconfig would let you do whatever bizarreness you wish in your objdir.
(In reply to Justin Dolske [:Dolske] from comment #13)
> Is there any specific objection to making autoclobber opt-out?

Autoclobber is already opt out, with the use of NO_AUTOCLOBBER in the mozconfig.
Summary: Make auto clobber experience better → Make auto clobber opt-in rather than opt-out
Attached patch Patch v1Splinter Review
I think we should just make this opt-in given feedback & principle of least surprise. Worst case devs who prefer opt-out have one build cycle where they walked off to get coffee and came back to the message informing them no auto-clobber was performed and what mozconfig pref they should add. More likely, most/many of them will see the newsgroup postings I'll make after this lands, and avoid that too.

--

* Makes auto clobber opt in rather than opt out, via the use of |mk_add_options AUTOCLOBBER| in the mozconfig.
* For people who have not opted in, clobber_reason states the necessary mozconfig pref.
* Sets AUTOCLOBBER in automation mozconfigs.
* Now tests the mozconfig pref being both set and not set.
* Removes now redundant part of test_objdir_clobber_older()
Assignee: nobody → emorley
Status: NEW → ASSIGNED
Attachment #741242 - Flags: review?(gps)
Comment on attachment 741242 [details] [diff] [review]
Patch v1

>+        # Check auto clobber opt-in works
>+        env['AUTOCLOBBER'] = 1

I've added the quotes locally, won't bother reattaching.
Comment on attachment 741242 [details] [diff] [review]
Patch v1

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

r=me with the following fixes.

::: build/mozconfig.common
@@ +9,5 @@
>  # architectures, though note that if you want to override options set in
>  # another mozconfig file, you'll need to use mozconfig.common.override instead
>  # of this file.
> +
> +mk_add_options AUTOCLOBBER

you need a value, otherwise that will fail ; like AUTOCLOBBER=1

::: python/mozbuild/mozbuild/controller/clobber.py
@@ +127,5 @@
>          # can work in some scenarios, we take the conservative approach and
>          # never try.
>          if not allow_auto:
> +            return True, False, self._message('Automatic clobbering is not '
> +                'enabled (add "mk_add_options AUTOCLOBBER" to your mozconfig).')

This message needs to be updated accordingly
Attachment #741242 - Flags: review?(gps) → review+
Thank you for the review :-)

Landed with nits fixed:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b49b5f42be79
We may also need to tweak the mozconfigs that are outside of the tree (eg the l10n ones in buildbot-configs), but that can happen in another bug.
https://hg.mozilla.org/mozilla-central/rev/b49b5f42be79
https://hg.mozilla.org/mozilla-central/rev/1264667a0854
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 867012
See Also: → 1100174
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.