Closed Bug 862986 Opened 11 years ago Closed 11 years ago

Move PROGRAM to moz.build

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla23

People

(Reporter: bokeefe, Assigned: bokeefe)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 12 obsolete files)

1.34 KB, patch
jcranmer
: review+
Details | Diff | Splinter Review
4.42 KB, patch
gps
: review+
Details | Diff | Splinter Review
6.69 KB, patch
bokeefe
: review+
Details | Diff | Splinter Review
47.52 KB, patch
bokeefe
: review+
Details | Diff | Splinter Review
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.
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.
Attachment #738664 - Flags: review?(gps)
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...
(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.
Attachment #738664 - Attachment is obsolete: true
Attachment #738664 - Flags: review?(gps)
Attachment #739157 - Flags: review?(Pidgeot18)
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.
Attachment #739157 - Flags: review?(Pidgeot18) → review?(gps)
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.
Attachment #739157 - Flags: review?(gps) → review?(Pidgeot18)
Attachment #739157 - Flags: review?(Pidgeot18) → review?(gps)
(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.
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.
(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.
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.
Attachment #739157 - Attachment is obsolete: true
Attachment #739232 - Attachment is obsolete: true
Attachment #739157 - Flags: review?(gps)
Attachment #739729 - Flags: review?(mh+mozilla)
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.
Attachment #739733 - Flags: review?(gps)
Removes the two includes of ipc/app/defs.mk from comm-central Makefile.ins.
Attachment #739787 - Flags: review?(bugspam.Callek)
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.
Attachment #739729 - Flags: review?(mh+mozilla) → review+
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.
Attachment #739733 - Flags: review?(gps) → review+
Attachment #739788 - Flags: review?(gps) → review+
Status: NEW → ASSIGNED
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.
Attachment #739787 - Flags: review?(bugspam.Callek) → review?(Pidgeot18)
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.
Attachment #739787 - Flags: review?(Pidgeot18) → review+
(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+
Attachment #739729 - Attachment is obsolete: true
Attachment #743563 - Flags: review+
(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.
Attachment #739733 - Attachment is obsolete: true
Attachment #743567 - Flags: review+
Keywords: checkin-needed
(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.
Attachment #743567 - Attachment is obsolete: true
Attachment #743643 - Flags: review?(gps)
The last patch lost the moz.build file for the test, for some reason. Sorry for the bugspam.
Attachment #743643 - Attachment is obsolete: true
Attachment #743643 - Flags: review?(gps)
Attachment #743646 - Flags: review?(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.
Attachment #743646 - Flags: review?(gps) → review+
Keywords: checkin-needed
Part 1a doesn't apply cleanly to inbound. Please rebase.
Keywords: checkin-needed
(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.
(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.
Attachment #743646 - Attachment is obsolete: true
Attachment #743886 - Flags: review+
Keywords: checkin-needed
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
(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.
Attachment #743886 - Attachment is obsolete: true
Attachment #744223 - Flags: review?(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...
Attachment #744223 - Flags: review?(gps) → review+
(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.
I'll push to try for you, but the patches don't apply cleanly on inbound :/
(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.
Attachment #744223 - Attachment is obsolete: true
Attachment #744581 - Flags: review?(gps)
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/
(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.
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
Attachment #744581 - Attachment is obsolete: true
Attachment #744581 - Flags: review?(gps)
Attachment #747382 - Flags: review?(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.
Attachment #747382 - Flags: review?(gps) → review+
(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.
Attachment #747382 - Attachment is obsolete: true
Attachment #747554 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/0bf5ed8e19a0
https://hg.mozilla.org/comm-central/rev/16f11f7df3c5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla23
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
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.