865673 - Convert objs.mk files to moz.build

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Michael Shal [:mshal]]

In addition to a Makefile.in, some directories provide an objs.mk file to share variables with other directories. These need to be converted to moz.build equivalents and included from the moz.build file. Any suggestions on a standard name for these?

This is slightly complicated by the fact that when an objs.mk file is used to compile the same set of files in different directories, there is a custom export rule in each directory to copy/symlink the files (eg: xpcom/build/ contains symlinks to a bunch of .cpp files in xpcom/glue/, where xpcom/glue/ contains the objs.mk file). It would be nice if we could avoid this copy and compile the files in place, or at least standardize it so each Makefile does not need a custom export rule.

Comment 1

[(not currently active) Ted Mielczarek]

I've never been very clear on why we need the "copy the sources" step. That may just exist to work around some deficiency of the build system. I think we should try to fix it so we can just compile the sources directly from the srcdir.

re: naming, objs.mozbuild? Precedent:
http://mxr.mozilla.org/mozilla-central/source/toolkit/toolkit.mozbuild

Comment 2

[Brian O'Keefe [:bokeefe]]

Attachment: Convert objs.mk to objs.mozbuild

This looked simple enough, so I took a stab at it. Since it's possible to compile source files from other directories now, I switched to absolute paths in the objs.mozbuild files, which let me get rid of the export rules instead of trying to update them.

The only objs.mk file I couldn't get rid of was media/mtransport/objs.mk, because it adds defines and includes along with the source files. I also didn't touch the crashreporter ones.

Feedback to gps and/or Ted, mostly for the approach: I followed the same approach as objs.mk, and created a *_lcppsrcs with just the file names, and used a generator to add the path for *_cppsrcs. I noticed after the fact that there was discussion on this in bug 900974. My approach happens to not follow any of those (although is halfway between #1 and #2 really).

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 797850 [details] [diff] [review]
Convert objs.mk to objs.mozbuild

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

Works for me. A bit hacky no matter how you cut it. But moz.build wins the tiebreaker over make files.

Comment 4

[Brian O'Keefe [:bokeefe]]

Attachment: Convert objs.mk to objs.mozbuild, v2

This version actually builds locally. The previous version failed because toolkit/mozapps/update/updater/archivereader.cpp helpfully tries to include one of the nsVersionComparator.cpps that was installed into the objdir. I added xpcom/glue to the include path so it could stop doing that.

I also moved the DEFINES/LOCAL_INCLUDES from media/mtransport/objs.mk to the two Makefile.ins that included it, which makes them longer, but it let me get rid of the objs.mk file so it wasn't the only one left in the tree.

I'll push this to try later tonight for sanity checking.

Comment 5

[Brian O'Keefe [:bokeefe]]

Comment on attachment 800338 [details] [diff] [review]
Convert objs.mk to objs.mozbuild, v2

Try came back green: https://tbpl.mozilla.org/?tree=Try&rev=2841b20d8a26

Comment 6

[Gregory Szorc [:gps]]

Comment on attachment 800338 [details] [diff] [review]
Convert objs.mk to objs.mozbuild, v2

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

Oy. Thank you for doing this. Unfortunately the sands of time have bit rotted some of the work, as LOCAL_INCLUDES is now in moz.build files.

Once that's moved over, this should get r+. I promise a speedier review next time. Please hold me accountable to it.

::: intl/unicharutil/util/internal/Makefile.in
@@ -23,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
> -
> -$(INTL_UNICHARUTIL_UTIL_LCPPSRCS): %: $(srcdir)/../%
> -	$(INSTALL) $^ .

Removing this rule seems wrong.

::: media/mtransport/build/Makefile.in
@@ +11,2 @@
>  
> +LOCAL_INCLUDES += \

LOCAL_INCLUDES is supported in moz.build.

Comment 7

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #6)
> ::: intl/unicharutil/util/internal/Makefile.in
> @@ -23,5 @@
> >  
> >  include $(topsrcdir)/config/rules.mk
> > -
> > -$(INTL_UNICHARUTIL_UTIL_LCPPSRCS): %: $(srcdir)/../%
> > -	$(INSTALL) $^ .
> 
> Removing this rule seems wrong.

I think it's fine - similar to the other custom INSTALL rules removed by the patch, it is just there to get a copy of the .cpp files in the local directory so they could be compiled. Since bug 888016 has landed, we don't need to do that anymore and can just compile ../bar/foo.cpp into ./foo.o. Was there some reason you think this particular INSTALL rule needs to stay?

Comment 8

[Michael Shal [:mshal]]

Comment on attachment 800338 [details] [diff] [review]
Convert objs.mk to objs.mozbuild, v2

>diff --git a/intl/unicharutil/util/objs.mozbuild b/intl/unicharutil/util/objs.mozbuild
>new file mode 100644
>--- /dev/null
>+++ b/intl/unicharutil/util/objs.mozbuild
>@@ -0,0 +1,17 @@
>+# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
>+# vim: set filetype=python:
>+# This Source Code Form is subject to the terms of the Mozilla Public
>+# License, v. 2.0. If a copy of the MPL was not distributed with this
>+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
>+
>+intl_unicharutil_util_lcppsrcs = [
>+    'nsBidiUtils.cpp',
>+    'nsSpecialCasingData.cpp',
>+    'nsUnicharUtils.cpp',
>+    'nsUnicodeProperties.cpp',
>+]
>+
>+intl_unicharutil_util_cppsrcs = [
>+    '%s/intl/unicharutil/util/%s' % (TOPSRCDIR, s) \
>+        for s in intl_unicharutil_util_lcppsrcs
>+]

Can we make it so that '/intl/unicharutil/util/foo.cpp' in CPP_SOURCES is automatically relative to TOPSRCDIR similar to LOCAL_INCLUDES? I don't think that should block this from landing, but it would be nice if we supported that consistently for all variables that could make use of it.

Comment 9

[Gregory Szorc [:gps]]

(In reply to Michael Shal [:mshal] from comment #7)
> (In reply to Gregory Szorc [:gps] from comment #6)
> > ::: intl/unicharutil/util/internal/Makefile.in
> > @@ -23,5 @@
> > >  
> > >  include $(topsrcdir)/config/rules.mk
> > > -
> > > -$(INTL_UNICHARUTIL_UTIL_LCPPSRCS): %: $(srcdir)/../%
> > > -	$(INSTALL) $^ .
> > 
> > Removing this rule seems wrong.
> 
> I think it's fine - similar to the other custom INSTALL rules removed by the
> patch, it is just there to get a copy of the .cpp files in the local
> directory so they could be compiled. Since bug 888016 has landed, we don't
> need to do that anymore and can just compile ../bar/foo.cpp into ./foo.o.
> Was there some reason you think this particular INSTALL rule needs to stay?

OK. Don't worry about it then.

Comment 10

[Brian O'Keefe [:bokeefe]]

Attachment: Convert objs.mk to objs.mozbuild, v3

(In reply to Gregory Szorc [:gps] from comment #6)
> Oy. Thank you for doing this. Unfortunately the sands of time have bit
> rotted some of the work, as LOCAL_INCLUDES is now in moz.build files.
> 
> Once that's moved over, this should get r+. I promise a speedier review next
> time. Please hold me accountable to it.

Fair enough. Fixing those wasn't too bad.

> ::: media/mtransport/build/Makefile.in
> @@ +11,2 @@
> >  
> > +LOCAL_INCLUDES += \
> 
> LOCAL_INCLUDES is supported in moz.build.

I moved these to the corresponding moz.build files. It didn't seem like they really belonged in the objs.mozbuild file, but I can move them if you'd prefer.

I reordered the includes so they were alphabetical. It doesn't look like that will hurt anything, but I pushed to try to be sure: https://tbpl.mozilla.org/?tree=Try&rev=eb22490e9e24

(In reply to Gregory Szorc [:gps] from comment #9)
> (In reply to Michael Shal [:mshal] from comment #7)
> > (In reply to Gregory Szorc [:gps] from comment #6)
> > > ::: intl/unicharutil/util/internal/Makefile.in
> > > @@ -23,5 @@
> > > >  
> > > >  include $(topsrcdir)/config/rules.mk
> > > > -
> > > > -$(INTL_UNICHARUTIL_UTIL_LCPPSRCS): %: $(srcdir)/../%
> > > > -	$(INSTALL) $^ .
> > > 
> > > Removing this rule seems wrong.
> > 
> > I think it's fine - similar to the other custom INSTALL rules removed by the
> > patch, it is just there to get a copy of the .cpp files in the local
> > directory so they could be compiled. Since bug 888016 has landed, we don't
> > need to do that anymore and can just compile ../bar/foo.cpp into ./foo.o.
> > Was there some reason you think this particular INSTALL rule needs to stay?
> 
> OK. Don't worry about it then.

Alright, I left the deletion.

Comment 11

[Brian O'Keefe [:bokeefe]]

Attachment: Interdiff between v2 and v3

(Almost) an interdiff between the (rebased) previous version and the latest, for easier reviewing. Interdiff doesn't really work on Windows, so it's actually just a diff of the two patches, but it's simple enough.

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 803502 [details] [diff] [review]
Convert objs.mk to objs.mozbuild, v3

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

This was an extremely difficult patch to review. Thanks Splinter! I wouldn't be surprised if something flew under the radar. But given the nature of the patch, I would think that any conversion errors would manifest obviously. So, I'm not overly concerned.

Comment 13

[Brian O'Keefe [:bokeefe]]

(In reply to Gregory Szorc [:gps] from comment #12)
> This was an extremely difficult patch to review. Thanks Splinter! I wouldn't
> be surprised if something flew under the radar. But given the nature of the
> patch, I would think that any conversion errors would manifest obviously.
> So, I'm not overly concerned.

Some of those sections ended up in a really odd order; sorry about that. Thanks for the review! I'll keep an eye on the trees after this lands for things that I might have broken.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d87616d0acf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0d87616d0acf