Convert objs.mk files to moz.build

RESOLVED FIXED in mozilla26

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: mshal, Assigned: bokeefe)

Tracking

(Blocks 2 bugs)

Trunk
mozilla26
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
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
Depends on: 875244
See Also: → 870376
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).
Attachment #797850 - Flags: feedback?(ted)
Attachment #797850 - Flags: feedback?(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.
Attachment #797850 - Flags: feedback?(ted)
Attachment #797850 - Flags: feedback?(gps)
Attachment #797850 - Flags: feedback+
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.
Assignee: nobody → bokeefe
Attachment #797850 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #800338 - Flags: review?(gps)
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 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.
Attachment #800338 - Flags: review?(gps) → feedback+
(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 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.
(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.
(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.
Attachment #800338 - Attachment is obsolete: true
Attachment #803502 - Flags: review?(gps)
(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 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.
Attachment #803502 - Flags: review?(gps) → review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d87616d0acf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Duplicate of this bug: 786534
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.