Last Comment Bug 777379 - Ensure that default is always the default target
: Ensure that default is always the default target
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-25 09:30 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2013-05-07 19:34 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Ensure that default is always the default target (2.85 KB, patch)
2012-08-05 23:59 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Ensure that default is always the default target (3.11 KB, patch)
2012-08-07 00:20 PDT, Mike Hommey [:glandium]
ted: review+
Details | Diff | Splinter Review
Set .DEFAULT_GOAL unconditionally, override with OVERRIDE_DEFAULT_GOAL, and fix pymake to be on par with GNU make when handling .DEFAULT_GOAL. (4.61 KB, patch)
2013-05-03 23:53 PDT, Mike Hommey [:glandium]
gps: review+
Details | Diff | Splinter Review
Additional .DEFAULT_GOAL test for pymake (569 bytes, patch)
2013-05-05 01:23 PDT, Mike Hommey [:glandium]
gps: review+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2012-07-25 09:30:44 PDT
Currently we rely on ordering to make default the default make target. This is broken in a number of places. We should instead set .DEFAULT_GOAL = default to ensure that this is always the default target.
Comment 1 Mike Hommey [:glandium] 2012-08-05 23:38:15 PDT
(In reply to Ted Mielczarek [:ted] from comment #0)
> We should instead set .DEFAULT_GOAL = default to ensure that this is always the default target.

Pymake doesn't support .DEFAULT_GOAL, so this would need to be added, too.
Comment 2 Mike Hommey [:glandium] 2012-08-05 23:59:44 PDT
Created attachment 649198 [details] [diff] [review]
Ensure that default is always the default target
Comment 3 Ted Mielczarek [:ted.mielczarek] 2012-08-06 06:19:51 PDT
Comment on attachment 649198 [details] [diff] [review]
Ensure that default is always the default target

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

Excellent!
Comment 4 :Ms2ger 2012-08-06 06:24:36 PDT
Comment on attachment 649198 [details] [diff] [review]
Ensure that default is always the default target

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

::: build/pymake/pymake/data.py
@@ +1541,5 @@
>          np = self.gettarget('.NOTPARALLEL')
>          if len(np.rules):
>              self.context = process.getcontext(1)
>  
> +	flavor, source, value = self.variables.get('.DEFAULT_GOAL')

Tab
Comment 6 Mike Hommey [:glandium] 2012-08-06 06:29:36 PDT
(In reply to :Ms2ger from comment #4)
> > +	flavor, source, value = self.variables.get('.DEFAULT_GOAL')
> 
> Tab

That's interesting... python didn't complain.
Comment 8 Mike Hommey [:glandium] 2012-08-06 11:22:27 PDT
Backed out the rules.mk parts (the pymake parts can stay in)
https://hg.mozilla.org/integration/mozilla-inbound/rev/870b30638837
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-06 17:11:39 PDT
https://hg.mozilla.org/mozilla-central/rev/7bc9fc61d58f
Comment 10 Ed Morley [:emorley] 2012-08-06 17:16:04 PDT
This was partially backed out, reopening.
(Commit message didn't match m-cMerge regexes, hence still being closed after backout; though not sure there's much that could be done differently to avoid it really).
Comment 11 Ed Morley [:emorley] 2012-08-06 17:17:27 PDT
> though not sure there's much that could be done differently
> to avoid it really).

Other than [leave open] being put in the whiteboard post-partial-backout, I guess.
Comment 12 Mike Hommey [:glandium] 2012-08-07 00:20:41 PDT
Created attachment 649556 [details] [diff] [review]
Ensure that default is always the default target


This is green on try.

A tree-wide automated check revealed that (once bug 780824 is fixed):
- 305 directories default to libs
- 7 default to check (!)
- 2 default to export
- 1 defaults to installer (browser/installer/windows, in the patch)
- 2 default to publish (in nspr, so it doesn't matter)
- 1 defaults to full-update (tools/update-packaging, in the patch)
- 2 default to all
- 1 defaults to test_bug292789.html
- 1 defaults to asm_com_offsets.asm
- 1 defaults to ISimpleDOMNode.h

I don't think any of those can suffer from a change of the default target to be 'default', besides the two changed in this patch (both required to turn green).
Comment 14 Ed Morley [:emorley] 2012-08-08 09:27:39 PDT
https://hg.mozilla.org/mozilla-central/rev/0aff1ee2a4fd
Comment 15 Gregory Szorc [:gps] 2013-05-03 16:35:00 PDT
Using ?= for DEFAULT_GOAL is wrong. Per the GNU make manual:

.DEFAULT_GOAL
    Sets the default goal to be used if no targets were specified on the command line (see Arguments to Specify the Goals). The .DEFAULT_GOAL variable allows you to discover the current default goal, restart the default goal selection algorithm by clearing its value, or to explicitly set the default goal. The following example illustrates these cases:

              # Query the default goal.
              ifeq ($(.DEFAULT_GOAL),)
                $(warning no default goal is set)
              endif
              
              .PHONY: foo
              foo: ; @echo $@
              
              $(warning default goal is $(.DEFAULT_GOAL))
              
              # Reset the default goal.
              .DEFAULT_GOAL :=
              
              .PHONY: bar
              bar: ; @echo $@
              
              $(warning default goal is $(.DEFAULT_GOAL))
              
              # Set our own.
              .DEFAULT_GOAL := foo

    This makefile prints:

              no default goal is set
              default goal is foo
              default goal is bar
              foo

It appears GNU make sets .DEFAULT_GOAL automatically when encountering a target if there is no current value to .DEFAULT_GOAL.

We should be using DEFAULT_GOAL := default and we should consider fixing pymake to behave like GNU make.
Comment 16 Mike Hommey [:glandium] 2013-05-03 23:53:10 PDT
Created attachment 745498 [details] [diff] [review]
Set .DEFAULT_GOAL unconditionally, override with OVERRIDE_DEFAULT_GOAL, and fix pymake to be on par with GNU make when handling .DEFAULT_GOAL.

For whoever comes first.
Comment 17 Mike Hommey [:glandium] 2013-05-03 23:54:53 PDT
https://tbpl.mozilla.org/?tree=Try&rev=2652b6bcc450
Comment 18 Gregory Szorc [:gps] 2013-05-04 10:32:41 PDT
Comment on attachment 745498 [details] [diff] [review]
Set .DEFAULT_GOAL unconditionally, override with OVERRIDE_DEFAULT_GOAL, and fix pymake to be on par with GNU make when handling .DEFAULT_GOAL.

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

Works for me!
Comment 19 :Ehsan Akhgari 2013-05-04 13:40:10 PDT
Can we check this in please?
Comment 22 Mike Hommey [:glandium] 2013-05-05 01:23:01 PDT
Created attachment 745625 [details] [diff] [review]
Additional .DEFAULT_GOAL test for pymake

I had forgotten to hg add this test.
Comment 23 Phil Ringnalda (:philor, back in August) 2013-05-05 17:18:35 PDT
https://hg.mozilla.org/mozilla-central/rev/dc4345aeb64c
Comment 26 Ryan VanderMeulen [:RyanVM] 2013-05-07 19:34:16 PDT
https://hg.mozilla.org/mozilla-central/rev/ac70913833fd

Note You need to log in before you can comment on or make changes to this bug.