The default bug view has changed. See this FAQ.

makefiles: move xpidl* logic into a named makefile - batch #1

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: joey, Assigned: joey)

Tracking

unspecified
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → joey
Blocks: 668861
Depends on: 757828
(Assignee)

Updated

5 years ago
No longer depends on: 757828
(Assignee)

Comment 1

5 years ago
Created attachment 626534 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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
Attachment #626534 - Flags: review?(ted.mielczarek)
(Assignee)

Updated

5 years ago
Blocks: 757967
(Assignee)

Comment 4

5 years ago
try job https://tbpl.mozilla.org/php/getParsedLog.php?id=12027824&tree=Try

mac failure due to ssh/key problems during checksum/upload
(Assignee)

Updated

5 years ago
Attachment #626534 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 5

5 years ago
Created attachment 629194 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1
(Assignee)

Updated

5 years ago
Attachment #626534 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
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.
Attachment #629194 - Flags: review?(mh+mozilla)
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.
Attachment #629194 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Updated

5 years ago
Depends on: 756443
(Assignee)

Comment 9

5 years ago
(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.
The whole thing is pretty much the same, except for replacing xpidl with something else.
(Assignee)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
Created attachment 634120 [details] [diff] [review]
move xpidl* logic into a named makefile - batch #1
(Assignee)

Updated

5 years ago
Attachment #629194 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/30da794ae32c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in before you can comment on or make changes to this bug.