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

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Comment 1

[Joey Armstrong [:joey]]

Attachment: move xpidl logic into a named makefile - batch #2

Comment 2

[Joey Armstrong [:joey]]

Attachment: move xpidl* logic into a named makefile

Comment 3

[Joey Armstrong [:joey]]

Comment on attachment 660055 [details] [diff] [review]
move xpidl* logic into a named makefile

move xpidl logic out of rules.mk and into config/makefiles/xpidl.mk.
localized macros added to replace long, common, inlined logic and sequences.
lowercase/localize some XPIDL macros used by targets and deps.

Added a test to verify LIBXUL_DIST is set before using XPIDL_DEPS.

Separated target logic into export & libs sections and made conditional so they are only visible when the appropriate target is being processed.

Comment 4

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=3192b8f97a2b

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 660055 [details] [diff] [review]
move xpidl* logic into a named makefile

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

Please separate this into 2 patches:

Part 1: Move content verbatim or as close to verbatim from rules.mk into standalone file
Part 2: Refactor content in new location

If Bugzilla had a better interface for doing code review, this may not be necessary. But, I like to do code review inside splinter and with the way it is, it is impossible to get a side-by-side/before-and-after view since the refactoring is part of the moving patch.

Comment 6

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #5)
> Comment on attachment 660055 [details] [diff] [review]
> move xpidl* logic into a named makefile
> 
> Review of attachment 660055 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please separate this into 2 patches:
> 
> Part 1: Move content verbatim or as close to verbatim from rules.mk into
> standalone file
> Part 2: Refactor content in new location
> 
> If Bugzilla had a better interface for doing code review, this may not be
> necessary. But, I like to do code review inside splinter and with the way it
> is, it is impossible to get a side-by-side/before-and-after view since the
> refactoring is part of the moving patch.

Rather than trash a tested/working patch how about just applying the patch in a sandbox and perform the diff there ?  Most editors would be able to perform a side-by-side diff of the two files.

I can submit two patches but that may increase the chance of breaking code that is working.

Comment 7

[Joey Armstrong [:joey]]

Attachment: patch #1 - move verbatim xpidl logic into config/makefiles/makefile.xpidl

Patch split to accomodate inadequate tools.
Move verbatim xpidl logic out of rules.mk and into config/makefiles/xpidl.mk

https://tbpl.mozilla.org/?tree=Try&rev=b442e4fa2bd8

Comment 8

[Gregory Szorc [:gps]]

Attachment: Part 2: Refactor xpidl logic

This is the incremental patch. Will conduct review on it.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 660506 [details] [diff] [review]
Part 2: Refactor xpidl logic

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

I don't like that there was a net increase in the number of lines of make syntax. But, the end state looks equivalent AFAICT and it seems to be a bit more robust. I say land it if it passes Try.

If you take the patch I submitted as part 2, please note that the author is wrongly attributed to me.

::: config/makefiles/xpidl.mk
@@ +28,2 @@
>  
> +    xpidl-module-xpt = $(XPIDL_GEN_DIR)/$(XPIDL_MODULE).xpt

It seems weird that XPIDL_MODULE is used here but then validated below. I know because of deferred assignment this works. But it is hard to read.

@@ +70,5 @@
> +    _xpidl-todo-libs_ += xpidl-gen-xpt
> +
> +    ifndef NO_DIST_INSTALL
> +      ifndef NO_INTERFACES_MANIFEST
> +		_xpidl-todo-libs_ += xpidl-gen-manifest

Replace tabs with spaces.

::: config/rules.mk
@@ +1200,5 @@
>  endif
>  endif
>  
>  ################################################################################
> +# Export the elements of $(XPIDLSRCS)

Your "patch #1" moved this block to xpidl.mk whereas the previous patch didn't. That's why this block is showing up here.

Should or should not this block be in xpidl.mk?

Comment 10

[Joey Armstrong [:joey]]

Attachment: xpidl - original patch logic merged into verbatim logic move into xpidl.mk

tested locally with an obj directory diff. waiting on try results.

Comment 11

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=c03dcf601c21

Comment 12

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 660506 [details] [diff] [review]
> Part 2: Refactor xpidl logic
> 
> Review of attachment 660506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't like that there was a net increase in the number of lines of make
> syntax. But, the end state looks equivalent AFAICT and it seems to be a bit
> more robust. I say land it if it passes Try.

Most of the additions were comments, adding vars to replace duplicate inlined paths and a conditional block to make export:: and libs:: elements visible only when those targets are being processed.

Other than these, build logic is pretty much the same.


> If you take the patch I submitted as part 2, please note that the author is
> wrongly attributed to me.

I'll check and see if it is in the patch

> 
> ::: config/makefiles/xpidl.mk
> @@ +28,2 @@
> >  
> > +    xpidl-module-xpt = $(XPIDL_GEN_DIR)/$(XPIDL_MODULE).xpt
> 
> It seems weird that XPIDL_MODULE is used here but then validated below. I
> know because of deferred assignment this works. But it is hard to read.

The assignment and conditional can move up that's not a problem.



> @@ +70,5 @@
> > +    _xpidl-todo-libs_ += xpidl-gen-xpt
> > +
> > +    ifndef NO_DIST_INSTALL
> > +      ifndef NO_INTERFACES_MANIFEST
> > +		_xpidl-todo-libs_ += xpidl-gen-manifest
> 
> Replace tabs with spaces.
> 
> ::: config/rules.mk
> @@ +1200,5 @@
> >  endif
> >  endif
> >  
> >  ################################################################################
> > +# Export the elements of $(XPIDLSRCS)
> 
> Your "patch #1" moved this block to xpidl.mk whereas the previous patch
> didn't. That's why this block is showing up here.
> 
> Should or should not this block be in xpidl.mk?

Casualty from breaking up the patch.  Macro names (xpidl-idl2h) do help document this somewhat but the comment should be included for 'moving them to the appropriate places.'

Comment 13

[Joey Armstrong [:joey]]

Comment on attachment 660520 [details] [diff] [review]
xpidl - original patch logic merged into verbatim logic move into xpidl.mk

Patch #2 - try results
https://tbpl.mozilla.org/?tree=Try&rev=c03dcf601c21

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 660520 [details] [diff] [review]
xpidl - original patch logic merged into verbatim logic move into xpidl.mk

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

Looks good. Not sure why those Windows builds failed on try. Perhaps you should try again, just to be sure? Or, you can always push to inbound and risk it :)

Also, I lost track of how all these patches apply. I trust you know what to do wrt pushing.

Comment 15

[Joey Armstrong [:joey]]

(In reply to Gregory Szorc [:gps] from comment #8)
> Created attachment 660506 [details] [diff] [review]
> Part 2: Refactor xpidl logic
> 
> This is the incremental patch. Will conduct review on it.

If you will be uploading a modified version of a patch in progress could you ping the person about it ?  The 2nd patch for this bug will need to be re-generated and re-tested to include these edits.

Thanks

Comment 16

[Joey Armstrong [:joey]]

patch pushed to b-s

https://tbpl.mozilla.org/?tree=Try&rev=594c3ba848b2

Looking into the windows failure around automation.py and
Running RegistrationOrder tests...
TEST-UNEXPECTED-FAIL | TestRegular FAILED - cannot create core service
TEST-UNEXPECTED-FAIL | TestJar FAILED - cannot create core service

Comment 17

[Joey Armstrong [:joey]]

xpidlsrcs migrating into mozbuild files so no longer need to refactor

Comment 18

[Gregory Szorc [:gps]]

I wouldn't just to this conclusion so fast. We will likely still produce .mk files containing the existing Makefile variables and this code will go through the existing Makefile logic. So, I think there may be some value to this bug.

Comment 19

[Joey Armstrong [:joey]]

ted	mshal: i take it a lot of that ugliness would go away if we moved XPIDLSRCS into moz.build?
	mshal	sorry I'm not sure I understand what you mean about static make files - here I'm just using pymake to parse the Makefile.in file
	mshal	if the Makefile.in file changes, tup runs pymake on that Makefile.in again to get the new set of XPIDLSRCS
	mshal	ted: yeah, the long-term idea would be the tup_xpidl.py script doesn't use pymake, but just includes moz.build directly

Comment 20

[Joey Armstrong [:joey]]

https://tbpl.mozilla.org/?tree=Try&rev=f72c2485e934

Comment 21

[Joey Armstrong [:joey]]

logic migrating to mozbuild files