862986 - Move PROGRAM to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Brian O'Keefe [:bokeefe]]

I had some spare cycles, and was going to take a crack at moving GENERATED_DIRS, but ipc/app/Makefile.in uses PROGRAM as part of a conditional, so it was easier to move that first.

I'll attach a patch in a moment, and a second patch to cover comm-central.

Comment 1

[Brian O'Keefe [:bokeefe]]

Attachment: Migrate PROGRAM to moz.build files

This was mostly automatic, but then I had to change $(BIN_SUFFIX) to CONFIG['BIN_SUFFIX'], so it ended up being almost entirely manual changes.

I haven't updated the blacklist in rules.mk yet - I'll cover that after comm-central is done.

Comment 2

[Joshua Cranmer [:jcranmer]]

Wouldn't it be smarter to have mach auto-add the $(BIN_SUFFIX)? Except for the ipc/app/defs.mk case, everyone either has the BIN_SUFFIX or it ought to have it...

Comment 3

[Brian O'Keefe [:bokeefe]]

Attachment: Migrate PROGRAM to moz.build files, v2

(In reply to Joshua Cranmer [:jcranmer] from comment #2)
> Wouldn't it be smarter to have mach auto-add the $(BIN_SUFFIX)? Except for
> the ipc/app/defs.mk case, everyone either has the BIN_SUFFIX or it ought to
> have it...

That sounded good to me, so I went ahead and did that. I didn't do anything special for the ipc/app/defs.mk case - for android, BIN_SUFFIX isn't defined anyway, and BIN_SUFFIX gets added in the non-android case. I copied defs.mk to a moz.build include-friendly defs.build, and changed that one.

I added a test for the new behavior while I was at it.

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 739157 [details] [diff] [review]
Migrate PROGRAM to moz.build files, v2

Despite what others may think, a build peer I am not.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 739157 [details] [diff] [review]
Migrate PROGRAM to moz.build files, v2

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

::: ipc/app/defs.mk
@@ +11,5 @@
> +    moz_child_process_name = 'lib/libplugin-container.so'
> +else:
> +    moz_child_process_name = 'plugin-container'
> +
> +moz_child_process_bundle = 'plugin-container.app/Contents/MacOS/'

The whole point of this file is to have shared values used in both ipc/app and ipc/glue. You might as well remove defs.mk and inline the values in ipc/glue/Makefile.in. Or, alternatively, define MOZ_CHILD_PROCESS_NAME and MOZ_CHILD_PROCESS_BUNDLE as AC_SUBSTs in configure.in.

Comment 6

[Brian O'Keefe [:bokeefe]]

(In reply to Mike Hommey [:glandium] from comment #5)
> ::: ipc/app/defs.mk
>
> The whole point of this file is to have shared values used in both ipc/app
> and ipc/glue. You might as well remove defs.mk and inline the values in
> ipc/glue/Makefile.in. Or, alternatively, define MOZ_CHILD_PROCESS_NAME and
> MOZ_CHILD_PROCESS_BUNDLE as AC_SUBSTs in configure.in.

It's actually included in quite a few other files, which is why I left defs.mk alone for now. Would you be okay with a follow-up to move all of those includes over to moz.build files, and remove defs.mk? (I'll put the BIN_SUFFIX part back and fix up the PROGRAM value in ipc/app/moz.build, if so.)

There's an include in toolkit/mozapps/installer/packager.mk that I'm not sure how to fix, other than inlining MOZ_CHILD_PROCESS_NAME.

Comment 7

[Brian O'Keefe [:bokeefe]]

Attachment: Migrate PROGRAM to moz.build files (c-c)

Patch for comm-central, which is much simpler than the m-c one.

I'll hold off requesting r? until the m-c patch is finished.

Comment 8

[Mike Hommey [:glandium]]

(In reply to Brian O'Keefe from comment #6)
> There's an include in toolkit/mozapps/installer/packager.mk that I'm not
> sure how to fix, other than inlining MOZ_CHILD_PROCESS_NAME.

ah, i only looked under ipc/ ; you might as well move it to configure.in then.

Comment 9

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1a: Convert ipc/app/defs.mk to AC_SUBSTs

This pulls MOZ_CHILD_PROCESS_NAME and MOZ_CHILD_PROCESS_BUNDLE out of ipc/app/defs.mk, and makes them AC_SUBSTs in configure.in. I also pulled the rule out of ipc/glue/Makefile.in to rebuild if defs.mk changed.

Comm-central will need a patch to go along with this. I'll do that once hg clone finishes.

Comment 10

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version

Moves PROGRAM out of Makefile.ins into moz.build. While I was at it, PROGRAM now automatically appends BIN_SUFFIX only if it wasn't already appended. This is mostly to stop plugin-container.exe.exe on Windows.

Comment 11

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1b: Remove ipc/app/defs.mk includes from c-c

Removes the two includes of ipc/app/defs.mk from comm-central Makefile.ins.

Comment 12

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2b: Move PROGRAM to moz.build files, c-c version

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 739729 [details] [diff] [review]
Part 1a: Convert ipc/app/defs.mk to AC_SUBSTs

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

::: CLOBBER
@@ +17,5 @@
>  #
>  # Modifying this file will now automatically clobber the buildbot machines \o/
>  #
> +Bug 862986: Needs a clobber because it adds AC_SUBSTs to configure.in, and
> +    removes ipc/app/defs.mk, which is included in several Makefiles

Actually, i don't think this requires a clobber.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 739733 [details] [diff] [review]
Part 2a: Move PROGRAM to moz.build files, m-c version

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

Awesome patch.

::: python/mozbuild/mozbuild/frontend/data.py
@@ +149,5 @@
>          SandboxDerived.__init__(self, sandbox)
>          self.exports = exports
> +
> +class Program(SandboxDerived):
> +    """Sandbox container object for PROGRAM, which is a unicode string.

Long term I imagine we'll be normalizing all binary-related variables down to a class like this. I'm tempted to call YAGNI and to use variable passthru for PROGRAM. But, meh. This is really the only complaint I have with this patch and it's easy enough to change down the road.

::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
@@ +169,5 @@
>  
> +    'PROGRAM' : (unicode, "",
> +        """Compiled executable name.
> +
> +        If the configuration token 'BIN_SUFFIX' is set, its value will be 

Nit: trailing whitespace.

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 739787 [details] [diff] [review]
Part 1b: Remove ipc/app/defs.mk includes from c-c

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

I'm sorry for the delay here, but I'll defer to joshua on this.

My gut is r+ if the m-c defines are already in c-c, r- if not. I can't recall top of my head the state there, but I know joshua has done a lot around that.

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 739787 [details] [diff] [review]
Part 1b: Remove ipc/app/defs.mk includes from c-c

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

We should pick up the AC_SUBST change automatically.

Comment 17

[Brian O'Keefe [:bokeefe]]

Attachment: Part 1a: Convert ipc/app/defs.mk to AC_SUBSTs, for m-c checkin (r=glandium)

(In reply to Mike Hommey [:glandium] from comment #13)
> Actually, i don't think this requires a clobber.

Now with 100% less clobbering. Carried forward r+

Comment 18

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, for check-in (r=gps)

(In reply to Gregory Szorc [:gps] from comment #14)
> ::: python/mozbuild/mozbuild/frontend/data.py
> @@ +149,5 @@
> >          SandboxDerived.__init__(self, sandbox)
> >          self.exports = exports
> > +
> > +class Program(SandboxDerived):
> > +    """Sandbox container object for PROGRAM, which is a unicode string.
> 
> Long term I imagine we'll be normalizing all binary-related variables down
> to a class like this. I'm tempted to call YAGNI and to use variable passthru
> for PROGRAM. But, meh. This is really the only complaint I have with this
> patch and it's easy enough to change down the road.

I was going to use variable passthru, but I thought it looked a little cleaner to put the suffix appending part off by itself. I'm not particularly attached to the separate class, but I left it as-is.

> ::: python/mozbuild/mozbuild/frontend/sandbox_symbols.py
> Nit: trailing whitespace.

Yeah, I saw that after I attached the patch. Fixed.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e9d5bb4a9af
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c7ba1d3d04

In my queue to land on c-c once this hits m-c.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c7297fe7ed

https://tbpl.mozilla.org/php/getParsedLog.php?id=22411491&tree=Mozilla-Inbound

Comment 21

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, v3 (r=gps)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Backed out for test failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b2c7297fe7ed
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=22411491&tree=Mozilla-
> Inbound

I should learn to spell, apparently.
This replaces '.prgm'/'.pgrm' (which I added) in test_emitter.py with '.prog', because it's easier to type.

Comment 22

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, v3

The last patch lost the moz.build file for the test, for some reason. Sorry for the bugspam.

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 743646 [details] [diff] [review]
Part 2a: Move PROGRAM to moz.build files, m-c version, v3

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

Ugh, typos.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Part 1a doesn't apply cleanly to inbound. Please rebase.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> Part 1a doesn't apply cleanly to inbound. Please rebase.

Speaking of typos, that should have been 2a.

Comment 26

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, v4 (r=gps)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> > Part 1a doesn't apply cleanly to inbound. Please rebase.
> 
> Speaking of typos, that should have been 2a.

Alright, let's try that again. I carried forward r+ since this was just context fixes - feel free to disagree.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8db9b6bd61
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7937fb5f4bb

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage. Please confirm that this works locally or on Try before requesting checkin again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5db4e09eeef

Logs:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c7937fb5f4bb

Comment 29

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, v5

(In reply to Ryan VanderMeulen [:RyanVM] from comment #28)
> Backed out for bustage. Please confirm that this works locally or on Try
> before requesting checkin again.

My apologies. I was able to build it locally, but it appears the patch lost the emitter.py changes which were in my working copy.

This version actually builds on a fresh clone of inbound.

Comment 30

[Gregory Szorc [:gps]]

Comment on attachment 744223 [details] [diff] [review]
Part 2a: Move PROGRAM to moz.build files, m-c version, v5

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

Looks good to me.

If you are confident it was just patch state that caused this to break, land in inbound. But, if it breaks both you and I will likely take some heat from the sheriffs. Perhaps a Try push would be best...

Comment 31

[Brian O'Keefe [:bokeefe]]

(In reply to Gregory Szorc [:gps] from comment #30)
> Comment on attachment 744223 [details] [diff] [review]
> Part 2a: Move PROGRAM to moz.build files, m-c version, v5
> 
> Review of attachment 744223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> If you are confident it was just patch state that caused this to break, land
> in inbound. But, if it breaks both you and I will likely take some heat from
> the sheriffs. Perhaps a Try push would be best...

I'm fairly certain our was just patch state, but a try run sounds like a better plan. I don't have commit access, so I'll need some assistance with that.

Comment 32

[Gregory Szorc [:gps]]

I'll push to try for you, but the patches don't apply cleanly on inbound :/

Comment 33

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, v6

(In reply to Gregory Szorc [:gps] from comment #32)
> I'll push to try for you, but the patches don't apply cleanly on inbound :/

Alright, let's try that again. This + part 1 apply on inbound on top of bf354dfbd689, which is tip for the moment.

Comment 34

[(not currently active) Ted Mielczarek]

Also, if you'd like to get Level 1 commit access (to push to try), I think one of us could certainly vouch for you.
http://www.mozilla.org/hacking/committer/

Comment 35

[Brian O'Keefe [:bokeefe]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #34)
> Also, if you'd like to get Level 1 commit access (to push to try), I think
> one of us could certainly vouch for you.
> http://www.mozilla.org/hacking/committer/

Thanks, that would be immensely helpful. :-) I'll run through those directions in the morning.

Comment 36

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, v6 (updated to tip)

This version applies to the latest inbound tip, and had a green try run, modulo Android 2.2 test timeouts:

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

Comment 37

[Gregory Szorc [:gps]]

Comment on attachment 747382 [details] [diff] [review]
Part 2a: Move PROGRAM to moz.build files, m-c version, v6 (updated to tip)

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

Looks good!

::: python/mozbuild/mozbuild/frontend/emitter.py
@@ +11,5 @@
>      DirectoryTraversal,
>      VariablePassthru,
>      Exports,
>      ReaderSummary,
> +    Program,

Nit: Sort by name.

Comment 38

[Brian O'Keefe [:bokeefe]]

Attachment: Part 2a: Move PROGRAM to moz.build files, m-c version, (v6) for check-in (r=gps)

(In reply to Gregory Szorc [:gps] from comment #37)
> ::: python/mozbuild/mozbuild/frontend/emitter.py
> @@ +11,5 @@
> >      DirectoryTraversal,
> >      VariablePassthru,
> >      Exports,
> >      ReaderSummary,
> > +    Program,
> 
> Nit: Sort by name.

Fixed.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0726f9659154
https://hg.mozilla.org/integration/mozilla-inbound/rev/557f1d26fb71

*fingers crossed*

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0726f9659154
https://hg.mozilla.org/mozilla-central/rev/557f1d26fb71

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0bf5ed8e19a0
https://hg.mozilla.org/comm-central/rev/16f11f7df3c5

Comment 42

[gorf4673]

This change seems to have broken a stand alone xulrunner build as the ipc/app folder doesn't have a Makefile generated and when "Make -C ipc export" runs it doesn't have a rule to generate missing dependency Makefile. 
(seeing as revision 142374 in beta)

To workaround, reverting Makefile.in to have
  include $(topsrcdir)/ipc/app/defs.mk
  PROGRAM = $(MOZ_CHILD_PROCESS_NAME)
and recovering the last defs.mk  seemed to allow progress, but not familiar enough with build know if that is ok or if something better to do.

Using Mozilla-build latest as of release for 23-beta9
Windows build - make -f client.mk clean build