743243 - makeutils.mk: add functions isTarget() & isThisTarget()

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

Add two user functions isTarget() and isThisTarget() that can be used to identify goal types and conditionally parse/include content as needed.

ex:
 o Some logic should not load or define targets when 'clean' or 'distclean' is the requested action.
 o export, libs & tools -- no need to read and define content for the 'other common' targets when a single target was requested.  Can also be used to identify other targets like test, clean, install, ...


# Also a few minor edits in makeutils.mk
makeutils::banner - replace hardcoded arg list with $(getargv)

define subargv() - helper for user functions to slice function specific arguments from the parameter list.

Comment 1

[Joey Armstrong [:joey]]

Attachment: Add target rules to identify goal type -- support conditional processing

Define functions to identify targets by prefix like: clean, clobber, libs, export, tools, dist, install, etc.

For tier processing -- if 'export' is the only requested target no need for the overhead to parse and define rules & logic for libs, tools, etc.

If make is aware that 'clean' has *not* been requested -- no need to define clean related logic.

If isThisTarget('clean', 'clobber') were requested there would be no need to run configure, or define logic for install, content generation (xpidl, headers, etc) -- only clean actions/targets for those deps and goal types.

Comment 2

[(not currently active) Ted Mielczarek]

Comment on attachment 612901 [details] [diff] [review]
Add target rules to identify goal type -- support conditional processing

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

::: config/makefiles/makeutils.mk
@@ +34,5 @@
> +# Usage: $(call subargv,2,$(getargv))
> +subargv =$(wordlist $(1),$(words $(getargv)),$(getargv))
> +
> +###########################################################################
> +# Usage: $(call banner,BUILDING: foo bar tans)

Are we using this banner function anywhere? If not I'd rather you removed it until you found a need for it. (Generally our developers prefer less make output, not more.)

@@ +47,5 @@
> +# Intent: Test command goals for a target prefix, return 'true' or 'false'.
> +#         Function can be used to conditionally include or parse logic
> +#         based on target type (clean export, install, lib).
> +# -----------------------------------------------------------------------
> +#   isTarget    : true if any of the targets match (prefix% or %prefix)

This seems like it's likely to be misused. I wouldn't expect people to know that $(call isTarget,clean) would also match "distclean". Can we make this just a straight comparison instead of a pattern match?

Comment 3

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted] from comment #2)
> Comment on attachment 612901 [details] [diff] [review]
> Add target rules to identify goal type -- support conditional processing
> 
> Review of attachment 612901 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/makefiles/makeutils.mk
> @@ +34,5 @@
> > +# Usage: $(call subargv,2,$(getargv))
> > +subargv =$(wordlist $(1),$(words $(getargv)),$(getargv))
> > +
> > +###########################################################################
> > +# Usage: $(call banner,BUILDING: foo bar tans)
> 
> Are we using this banner function anywhere? If not I'd rather you removed it
> until you found a need for it. (Generally our developers prefer less make
> output, not more.)

So far I have mainly been using it for debugging more than output generation.

For ex you can wrap pre-reqs for a target in syntax like this:
my-preqs =\
  $(call banner,my-preqs-BEGIN)\
  preq-dep-1\
  preq-dep-2\
  $(call banner,my-preqs-END)\
  $(NULL)

the decorations make it very easy to to tell if a :: target was involved in a failure.

The macro can also be over-loaded to display debuging info when extra flags are passed
  % gmake mode=verbose
  % gmake mode=verbose watch=xpidl
  [ ... ]

It is just getting old re-creating the macro in several different sandboxes :)


> @@ +47,5 @@
> > +# Intent: Test command goals for a target prefix, return 'true' or 'false'.
> > +#         Function can be used to conditionally include or parse logic
> > +#         based on target type (clean export, install, lib).
> > +# -----------------------------------------------------------------------
> > +#   isTarget    : true if any of the targets match (prefix% or %prefix)
> 
> This seems like it's likely to be misused. I wouldn't expect people to know
> that $(call isTarget,clean) would also match "distclean". Can we make this
> just a straight comparison instead of a pattern match?

There will be another case for the logic to handle.  Wildcards will be needed at some level to cover name permutations or the target list used for matching will eventually become stale or incomplete.

How about this to try and handle all cases:
  o remove existing pattern/wildcard permutations, only test using arguments given.
  o declare a 2nd function 'isTargetPrefix' setup to call isTarget and better document the usage.  The call is made using a $(foreach ) loop that will iterate over wildcard patterns as declared in the patch.
  o for consumers of isTargetPrefix { clean, tools, export, libs, install-? } declare named functions like isCleanTarget, isToolsTarget, etc to wrap and document a list of targets and/or patterns used to identify them { also followed by a battery of unit tests to verify usage }.

Comment 4

[(not currently active) Ted Mielczarek]

That sounds totally reasonable.

Comment 5

[Joey Armstrong [:joey]]

minor edits:
 o name function 'isTargetStem' rather than 'isTargetPrefix'.
 o continue the trend for common target naming (for source alignment):
isTargetClean, isTargetExport, ...

Comment 6

[Joey Armstrong [:joey]]

Attachment: Add utility functions to identify makefile targets and classes of targets

Similar patch to the one before with deltas:
  o Document a debug usage case for the $(banner) function.
  o isTarget* functions replaced by argument driven is_XinY()
  o isTargetStem() added to detect targets that begin or end with a stem (clean, distclean clean-area-51)
  o isTargetStemClean, isTargetStemExport - added for detection of common/tier target goals.
  o Unit tests setup to validate functionality.
  o requiredfunction() - utility function used to verify proper makefile inclusion.
  o errorifneq - test helper that was generic enough to be listed in makeutils.mk

Comment 7

[Joey Armstrong [:joey]]

ping on code review

Comment 8

[Joey Armstrong [:joey]]

re-ping on code review

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 614100 [details] [diff] [review]
Add utility functions to identify makefile targets and classes of targets

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

::: config/makefiles/makeutils.mk
@@ +71,5 @@
> +isTargetStem       = $(sort $(foreach pat, $(1)% %$(1), $(call is_XinY,$(pat),${$(mcg_goals)})))
> +isTargetStemClean  = $(call isTargetStem,clean)
> +isTargetStemExport = $(call isTargetStem,export)
> +isTargetStemLibs   = $(call isTargetStem,libs)
> +isTargetStemTools  = $(call isTargetStem,tools)

Do we actually have any targets like export-*,libs-*,tools-* ?

@@ +76,5 @@
> +
> +#isTargetStemClean  = $(sort $(foreach pat, clean% %clean,   $(call is_XinY,$(pat),${$(mcg_goals)})))
> +#isTargetStemExport = $(sort $(foreach pat, export% %export, $(call is_XinY,$(pat),${$(mcg_goals)})))
> +#isTargetStemLibs   = $(sort $(foreach pat, libs% %libs,     $(call is_XinY,$(pat),${$(mcg_goals)})))
> +#isTargetStemTools  = $(sort $(foreach pat, tools% %tools,   $(call is_XinY,$(pat),${$(mcg_goals)})))

These seem extraneous. Just drop them? The declarations above are pretty self-evident.

Comment 10

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted] from comment #9)
> Comment on attachment 614100 [details] [diff] [review]
> Add utility functions to identify makefile targets and classes of targets
> 
> Review of attachment 614100 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/makefiles/makeutils.mk
> @@ +71,5 @@
> > +isTargetStem       = $(sort $(foreach pat, $(1)% %$(1), $(call is_XinY,$(pat),${$(mcg_goals)})))
> > +isTargetStemClean  = $(call isTargetStem,clean)
> > +isTargetStemExport = $(call isTargetStem,export)
> > +isTargetStemLibs   = $(call isTargetStem,libs)
> > +isTargetStemTools  = $(call isTargetStem,tools)
> 
> Do we actually have any targets like export-*,libs-*,tools-* ?

yes, ${target}-preqs would be an easy one.
*/locales/Makefile.in contains 'libs-%'

No export or tools yet but we should match them anyway to cover the general case { if new naming conventions come into play }.


 
> @@ +76,5 @@
> > +
> > +#isTargetStemClean  = $(sort $(foreach pat, clean% %clean,   $(call is_XinY,$(pat),${$(mcg_goals)})))
> > +#isTargetStemExport = $(sort $(foreach pat, export% %export, $(call is_XinY,$(pat),${$(mcg_goals)})))
> > +#isTargetStemLibs   = $(sort $(foreach pat, libs% %libs,     $(call is_XinY,$(pat),${$(mcg_goals)})))
> > +#isTargetStemTools  = $(sort $(foreach pat, tools% %tools,   $(call is_XinY,$(pat),${$(mcg_goals)})))
> 
> These seem extraneous. Just drop them? The declarations above are pretty
> self-evident.

Whups yes this is garbage from an earlier edit that needs to be removed.

Comment 11

[Joey Armstrong [:joey]]

Attachment: Add functions to detect classes of makefile targets

same patch as last time with old commented-out logic removed.
r=ted

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f0b589036e4

I had to un-bitrot config/makefiles/test/Makefile.in pretty heavily. Please look it over to make sure I didn't make any mistakes.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

http://hg.mozilla.org/mozilla-central/rev/4f0b589036e4