Last Comment Bug 757855 - makefiles: move xpidl* logic into a named makefile - batch #1
: makefiles: move xpidl* logic into a named makefile - batch #1
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Joey Armstrong [:joey]
:
: Gregory Szorc [:gps]
Mentors:
Depends on: 756443
Blocks: 668861 757967
  Show dependency treegraph
 
Reported: 2012-05-23 08:25 PDT by Joey Armstrong [:joey]
Modified: 2012-06-19 01:19 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move xpidl* logic into a named makefile - batch #1 (9.94 KB, patch)
2012-05-23 11:53 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review
move xpidl* logic into a named makefile - batch #1 (11.05 KB, patch)
2012-06-01 08:13 PDT, Joey Armstrong [:joey]
mh+mozilla: review+
Details | Diff | Splinter Review
move xpidl* logic into a named makefile - batch #1 (9.79 KB, patch)
2012-06-18 11:23 PDT, Joey Armstrong [:joey]
no flags Details | Diff | Splinter Review

Description Joey Armstrong [:joey] 2012-05-23 08:25:02 PDT

    
Comment 1 Joey Armstrong [:joey] 2012-05-23 11:53:06 PDT
Created attachment 626534 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1
Comment 2 Joey Armstrong [:joey] 2012-05-23 11:57:35 PDT
Start small, moved xpidl install targets for headers and source into config/makefiles/xpidl.mk.

Logic has been split into two parts.  Conditional, should work be performed and named targets to perform tasks when needed.

Added a check unit test for the xpidl logic.
Comment 3 Joey Armstrong [:joey] 2012-05-23 11:58:06 PDT
Comment on attachment 626534 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1

Start small, moved xpidl install targets for headers and source into config/makefiles/xpidl.mk.

Logic has been split into two parts.  Conditional, should work be performed and named targets to perform tasks when needed.

Added a check unit test for the xpidl logic.

A try job is pending
Comment 4 Joey Armstrong [:joey] 2012-05-24 10:50:57 PDT
try job https://tbpl.mozilla.org/php/getParsedLog.php?id=12027824&tree=Try

mac failure due to ssh/key problems during checksum/upload
Comment 5 Joey Armstrong [:joey] 2012-06-01 08:13:24 PDT
Created attachment 629194 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1
Comment 6 Joey Armstrong [:joey] 2012-06-01 08:15:38 PDT
Comment on attachment 629194 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1

Patch refresh.

try job: https://tbpl.mozilla.org/?tree=Try&rev=8b2b5a8aad7d
ran cleanly, no ssh/key problems reported this attempt.

Patch: move xpidl* logic into a named makefile w/unit tests.
Comment 7 Mike Hommey [:glandium] 2012-06-07 05:06:12 PDT
Comment on attachment 629194 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1

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

::: config/makefiles/test/Makefile.in
@@ +23,5 @@
>  check-tests =\
>    $(check-arglist) \
>    $(check-autotargets) \
>    $(check-XinY) \
> +  $(check-xpidl) \

This will conflict with bug 757828, you'll have to rebase one way or the other.

@@ +120,5 @@
> +  $(NULL)
> +$(check-xpidl): $(check-xpidl-preqs)
> +	$(MAKE) -f $(srcdir)/check-xpidl.mk check-xpidl $(check-xpidl-args)
> +	@$(TOUCH) $@
> +# </check-xpidl.mk>

it would be better to factor this, there's a lot of repetition with bug 757828.

::: config/makefiles/test/check_XinY.mk
@@ +23,5 @@
>  # verify strcmp('clean')
>  val  := clean
>  $(call errorifneq,$(zero),$(words $(call is_XinY,foo,$(val))))
>  $(call errorifneq,$(one),$(words $(call is_XinY,clean,$(val))))
> +$(call errorifneq,$(one),$(words $(call is_XinY,clean%,$(val))))

Not related to this bug, you may want to do that in a separate bug.

::: config/makefiles/xpidl.mk
@@ +44,5 @@
> +  $(XPIDLSRCS) \
> +  $(call mkdir_deps,$(IDL_DIR)) \
> +  $(NULL)
> +
> +# bug 756443 todo: replace .mkdir.done with $(call mkdir_stem)

Make this bug depend on that one, and do it now.

::: config/rules.mk
@@ +1355,5 @@
>  
>  
>  # General rules for exporting idl files.
>  $(IDL_DIR):
>  	$(NSINSTALL) -D $@

You probably want to change that.
Comment 8 Joey Armstrong [:joey] 2012-06-12 10:50:05 PDT
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 629194 [details] [diff] [review]
> move xpidl* logic into a named makefile - batch #1
> 
> Review of attachment 629194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: config/makefiles/test/Makefile.in
> @@ +23,5 @@
> >  check-tests =\
> >    $(check-arglist) \
> >    $(check-autotargets) \
> >    $(check-XinY) \
> > +  $(check-xpidl) \
> 
> This will conflict with bug 757828, you'll have to rebase one way or the
> other.
> 
> @@ +120,5 @@
> > +  $(NULL)
> > +$(check-xpidl): $(check-xpidl-preqs)
> > +	$(MAKE) -f $(srcdir)/check-xpidl.mk check-xpidl $(check-xpidl-args)
> > +	@$(TOUCH) $@
> > +# </check-xpidl.mk>
> 
> it would be better to factor this, there's a lot of repetition with bug
> 757828.

Yes this is not at all ideal but I've had problems with bugs being blocked on chains of bug dependencies.  To avoid potential blockers trying to submit standalone patches when possible.  Even when having to rebase after one of the patches-in-common have landed.


> ::: config/makefiles/test/check_XinY.mk
> @@ +23,5 @@
> >  # verify strcmp('clean')
> >  val  := clean
> >  $(call errorifneq,$(zero),$(words $(call is_XinY,foo,$(val))))
> >  $(call errorifneq,$(one),$(words $(call is_XinY,clean,$(val))))
> > +$(call errorifneq,$(one),$(words $(call is_XinY,clean%,$(val))))
> 
> Not related to this bug, you may want to do that in a separate bug.

A new bug was opened for this.
Comment 9 Joey Armstrong [:joey] 2012-06-12 14:19:15 PDT
(In reply to Mike Hommey [:glandium] from comment #7)
> Comment on attachment 629194 [details] [diff] [review]
> move xpidl* logic into a named makefile - batch #1
> 
> Review of attachment 629194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +120,5 @@
> > +  $(NULL)
> > +$(check-xpidl): $(check-xpidl-preqs)
> > +	$(MAKE) -f $(srcdir)/check-xpidl.mk check-xpidl $(check-xpidl-args)
> > +	@$(TOUCH) $@
> > +# </check-xpidl.mk>
> 
> it would be better to factor this, there's a lot of repetition with bug
> 757828.

Not sure where repetition is coming into play in here.  The unit test blocks have a similar format but each section is using a distinct set of dependencies for testing.

Individual tests will only run when a test .mk file or the makefile a test guards is modified.  Not all unit tests will be run when testing is needed so deps are handled separately.

If the repetition was for $(INSTALL) commands from xpidl and the target_export tests both are needed.  They will run independently when either xpidl.mk or target_export.mk are modified.
Comment 10 Mike Hommey [:glandium] 2012-06-12 22:03:30 PDT
The whole thing is pretty much the same, except for replacing xpidl with something else.
Comment 11 Joey Armstrong [:joey] 2012-06-13 06:43:33 PDT
(In reply to Mike Hommey [:glandium] from comment #10)
> The whole thing is pretty much the same, except for replacing xpidl with
> something else.

Sources and deps are distinct, this is not a simple string replace of xpidl.
Opening an enhancement bug to revisit refactoring in a followup patch.
Comment 12 Joey Armstrong [:joey] 2012-06-18 11:23:07 PDT
Created attachment 634120 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1
Comment 13 Joey Armstrong [:joey] 2012-06-18 12:22:44 PDT
Carry glandium=r+ forward from 06/01.  Attribute field cleared/not recorded in comments with the last 'bz export' upload.

  o bug 756443: patch landed so replace hardcoded .mkdir.done string replacement with $(call mkdir_stem).
  o config/makefiles/test/check_XinY.mk removed from the patch.  Edits moved to a new bug and already have landed.
Comment 14 Ed Morley [:emorley] 2012-06-19 01:19:42 PDT
https://hg.mozilla.org/mozilla-central/rev/30da794ae32c

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