Closed Bug 937594 Opened 6 years ago Closed 6 years ago

add ./mach clobber to B2G's mach

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: julienw, Assigned: ahal)

Details

Attachments

(1 file)

./mach clobber would:

* rm -rf out
* rm -rf $GECKO_OBJDIR (from .userconfig)

$GECKO_OBJDIR can itself result from other env variables, eg:

  GECKO_OBJDIR=$GECKO_PATH/objdir-gonk-$VARIANT
We shouldn't need to worry about the objdir, the bootstrap script should detect it automatically using the mozconfig (which I assume in turn uses the various env variables).
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
I haven't tested out all the objdir edge cases locally, but it should work (if it doesn't there's a bug in mozbuild somewhere).
Attachment #831743 - Flags: review?(jgriffin)
Julien, do you know if it's possible for the out directory to be located elsewhere? Or is it guaranteed to be at $B2G_HOME/out?
Flags: needinfo?(felash)
see https://github.com/mozilla-b2g/platform_build/blob/master/core/envsetup.mk#L6 and https://github.com/mozilla-b2g/platform_build/blob/master/core/envsetup.mk#L135-L141

we can probably assume it will be "out" in all practical situations, but it you can get OUT_DIR it could be useful.

From envsetup.sh, this var can be read from the root directory using:

  CALLED_FROM_SETUP=true BUILD_SYSTEM=build/core make --no-print-directory -f build/core/config.mk dumpvar-abs-PRODUCT_OUT

Thanks for your work!
Flags: needinfo?(felash)
Comment on attachment 831743 [details] [diff] [review]
Patch 1.0 - add mach clobber to b2g

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

Looks good to me.
Attachment #831743 - Flags: review?(jgriffin) → review+
Pull request: https://github.com/mozilla-b2g/B2G/pull/296

(In reply to Julien Wajsberg [:julienw] from comment #4)
> From envsetup.sh, this var can be read from the root directory using:
> 
>   CALLED_FROM_SETUP=true BUILD_SYSTEM=build/core make --no-print-directory
> -f build/core/config.mk dumpvar-abs-PRODUCT_OUT

Good to know! For now I wouldn't want to put this in the bootstrap script, because that would mean invoking a make target everytime ./mach <anything> is run. We could potentially put it in the clobber command itself, but then we'd have to duplicate code if other commands wanted to get it. Would it be possible to somehow get OUT_DIR stored in the .config? Mach already sources load-config.sh each time.

I'd also like to just land the above patch for now because:
a) this adds 0.5 seconds to calculate
b) it returns out/target/product/<device_name> instead of the root out directory. We'd still need to make some assumptions about where the out directory is relative to that
ah yeah, PRODUCT_OUT is not good, use OUT_DIR instead :)

CALLED_FROM_SETUP=true BUILD_SYSTEM=build/core make --no-print-directory -f build/core/config.mk dumpvar-abs-OUT_DIR

But yeah, otherwise, I agree with you. I don't know who could help with having it in .config instead of a hardcoded default somewhere, maybe Dave?
Flags: needinfo?(dhylands)
I've landed ahal's patch, but will leave this open to discuss improvements:

https://github.com/mozilla-b2g/B2G/commit/e0a2b68c1da5508e13c15b921ac98a3dd419b6dc
You could put the following:

$(shell echo "OUT_DIR=$(OUT_DIR)" > $(TOP)/.var.mach)

At the end of gonk-misc/Android.mk (so that it runs during pass 1 of the Makefile)

Stashing OUT_DIR at config.sh time doesn't make sense. You want whatever the last build used, and the builds are influenced by .userconfig, which can be edited on a per-build basis.

Putting the above in the Android.mk should add very little time to the build (not like the 1/2 second by invoking make).

If mach can't find the .var.mach file, then it can assume that the build hasn't run yet.
Flags: needinfo?(dhylands)
Hmm I don't think this is appropriate. We should be addressing cases where we need to clobber rather than making it easier to clobber. There's really not much of a difference between mach clobber and rm -rf out objdir-gecko. Not enough to justify adding a command, especially when a correct shell script that does this takes literally four lines.
(In reply to Michael Wu [:mwu] from comment #10)
> Hmm I don't think this is appropriate. We should be addressing cases where
> we need to clobber rather than making it easier to clobber. There's really
> not much of a difference between mach clobber and rm -rf out objdir-gecko.
> Not enough to justify adding a command, especially when a correct shell
> script that does this takes literally four lines.

Michael, I would find it useful, and Julien obviously finds it useful as well otherwise he wouldn't have filed this bug. Unless you can give me a concrete reason why this changeset would be harmful, I'd ask that you please not back it out. No one is stopping you from using shell scripts or doing it manually.

Thanks,
Andrew
Julien, what cases are you clobbering in? I'd like to understand your use cases.
(In reply to Dave Hylands [:dhylands] from comment #9)
> You could put the following:
> 
> $(shell echo "OUT_DIR=$(OUT_DIR)" > $(TOP)/.var.mach)
> 
> At the end of gonk-misc/Android.mk (so that it runs during pass 1 of the
> Makefile)
> 
> Stashing OUT_DIR at config.sh time doesn't make sense. You want whatever the
> last build used, and the builds are influenced by .userconfig, which can be
> edited on a per-build basis.
> 
> Putting the above in the Android.mk should add very little time to the build
> (not like the 1/2 second by invoking make).
> 
> If mach can't find the .var.mach file, then it can assume that the build
> hasn't run yet.

Ok, I think until mach has a greater use for knowing the out dir we are probably over engineering a bit here. I would recommend shelving the conversation until this becomes an actual problem. Any objections to closing the bug and filing a follow up if we need to?
Not including a clobber command was an intentional decision. I have reverted this commit and am resolving this WONTFIX to make this clear.

https://github.com/mozilla-b2g/B2G/commit/675bb9136bc27b1b3db4c9f662a34d5668051e1e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
(In reply to Michael Wu [:mwu] from comment #10)
> Hmm I don't think this is appropriate. We should be addressing cases where
> we need to clobber rather than making it easier to clobber. There's really
> not much of a difference between mach clobber and rm -rf out objdir-gecko.
> Not enough to justify adding a command, especially when a correct shell
> script that does this takes literally four lines.

I use a objdir in the tree I'm building, depending on the variant I'm building. To make it concrete, here is my .userconfig:

  VARIANT=userdebug
  #VARIANT=eng

  export CC=gcc-4.6 CXX=g++-4.6
  GECKO_PATH=/home/julien/travail/hg/mozilla-central
  GECKO_OBJDIR=$GECKO_PATH/objdir-gonk-$VARIANT

This make it easy to switch trees without needing rebuild everything. However, this makes it more difficult to actually rm -rf the objdir directory, as it is in another tree.

(In reply to Michael Wu [:mwu] from comment #12)
> Julien, what cases are you clobbering in? I'd like to understand your use
> cases.

Well, when I sometimes update my tree, the build scripts want me to clobber. In a usual mozilla-central tree, this is "./mach clobber" (note that "rm -rf obj-*" is just as easy, but still a "./mach clobber" command was made), so it makes sense to have the same in B2G's mach. This is for consistency, for ease of use when we use external gecko directories as explained before, and this is also what the moz-central build script is outputting when a clobber has to made because of a specific change.

I also sometimes get some build issues that I resolve doing a clobber. That must be resolved, I agree with you for this, but this is not the only reason.

I agree we should not overengineer this. I also don't care about removing the previous build's out directories, I find actually a lot more predictable to remove the directories that are currently configured.

I'd say we could safely use $OUT_DIR if the env variable is defined, and "out" if not.

If you resolve this WONTFIX again, of course I could do my own shell file, but this would be unfortunate.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(In reply to Michael Wu [:mwu] from comment #10)
> Hmm I don't think this is appropriate. We should be addressing cases where
> we need to clobber rather than making it easier to clobber.

Those are two separate issues. You can address cases where you need to clobber *and* make it easier to clobber. Clobbering will never go away completely.

> not much of a difference between mach clobber and rm -rf out objdir-gecko.
> Not enough to justify adding a command, especially when a correct shell
> script that does this takes literally four lines.

Mach hooks into the gecko build system and thus automatically knows where the objdir is. This means the target will never have to be updated, even if the underlying environment variables/shell scripts do.

Michael, so far every one of your arguments against mach has been a vague statement about "not needing it". This bug isn't the first time I've been asked to implement commands in mach, so it seems even if it isn't needed, it's still wanted.
I'm not sure we're going anywhere now, and I actually haven't needed this for some time now, so I'm closing it. I'll reopen if I see that I need this again in the future.

Still, thanks Andrew for your work!
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.