Closed Bug 915566 Opened 9 years ago Closed 8 years ago

get rid of some makefiles that only define include paths

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Details

Attachments

(8 files, 1 obsolete file)

No description provided.
Attachment #803556 - Flags: review?(mshal)
Attached patch rm Mkaefiles in content/ (obsolete) — Splinter Review
Attachment #803557 - Flags: review?(ted)
Comment on attachment 803556 [details] [diff] [review]
rm some makefiles in docshell/

># HG changeset patch
># User Trevor Saunders <trev.saunders@gmail.com>
>
>diff --git a/docshell/build/Makefile.in b/docshell/build/Makefile.in
>deleted file mode 100644
>index 4b2b8c1..0000000
>--- a/docshell/build/Makefile.in
>+++ /dev/null
>@@ -1,28 +0,0 @@
>-# 
>-# 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/.
>-
>-EXPORT_LIBRARY = 1
>-SHARED_LIBRARY_LIBS= \
>-		../base/$(LIB_PREFIX)basedocshell_s.$(LIB_SUFFIX) \
>-		$(DEPTH)/uriloader/base/$(LIB_PREFIX)uriloaderbase_s.$(LIB_SUFFIX) \
>-		$(DEPTH)/uriloader/exthandler/$(LIB_PREFIX)exthandler_s.$(LIB_SUFFIX) \
>-		$(DEPTH)/uriloader/prefetch/$(LIB_PREFIX)prefetch_s.$(LIB_SUFFIX) \
>-		../shistory/src/$(LIB_PREFIX)shistory_s.$(LIB_SUFFIX) \
>-		$(NULL)

Removing libdocshell and moving the SHARED_LIBRARY_LIBS to toolkit/library seems unrelated to this bug. Was this meant to be under a different bug? If not, can we make it part of a different bug? I don't think moving that is as straightforward as moving the includes to moz.build.

>diff --git a/docshell/build/moz.build b/docshell/build/moz.build
>index 3d9c503..8c6ecbc 100644
>--- a/docshell/build/moz.build
>+++ b/docshell/build/moz.build
>@@ -13,8 +13,18 @@ EXPORTS += [
> CPP_SOURCES += [
>     'nsDocShellModule.cpp',
> ]
> 
> LIBRARY_NAME = 'docshell'
> 
> LIBXUL_LIBRARY = True
> 
>+LOCAL_INCLUDES += [
>+    "../base",
>+    "../shistory/src/",
>+    "/uriloader/base",
>+    "/uriloader/exthandler",
>+    "/uriloader/prefetch",
>+    ]

nit: Most strings in mozbuild use single-quotes, not double-quotes.

>+if CONFIG["MOZ_WIDGET_TOOLKIT"] == "cocoa":
>+    LOCAL_INCLUDES += ["/uriloader/exthandler/mac"]

nit: Prefer using:

if CONFIG...
    LOCAL_INCLUDES += [
        '/uriloader/exthandler/mac',
    ]

So it is easier to add more entries later.

>diff --git a/docshell/shistory/src/moz.build b/docshell/shistory/src/moz.build
>index 4ca6d43..7bc1e6f 100644
>--- a/docshell/shistory/src/moz.build
>+++ b/docshell/shistory/src/moz.build
>@@ -20,8 +20,9 @@ CPP_SOURCES += [
> LIBRARY_NAME = 'shistory_s'
> 
> FAIL_ON_WARNINGS = True
> 
> LIBXUL_LIBRARY = True
> 
> MSVC_ENABLE_PGO = True
> 
>+LOCAL_INCLUDES += ["../../base"]

Similar nit here.
Attachment #803556 - Flags: review?(mshal) → feedback+
Comment on attachment 803557 [details] [diff] [review]
rm Mkaefiles in content/

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

I think we should probably set a guideline for when to use relative vs. "absolute" (topsrcdir-relative) paths in moz.build variables. My suggestion would be "one parent-dir (..) in the path is okay, any more and you should use a topsrcdir-absolute path".

::: content/xbl/src/moz.build
@@ +44,5 @@
>  
>  LIBRARY_NAME = 'gkconxbl_s'
>  
> +LOCAL_INCLUDES += [
> +    "../../xul/content/src",

Any reason this isn't an absolute path?

::: content/xul/document/src/moz.build
@@ +39,5 @@
> +    "/layout/generic",
> +    "/layout/style",
> +    "/layout/xul/base/src",
> +    "/xpcom/ds",
> +    ]

fix indentation

::: content/xul/templates/src/moz.build
@@ +44,5 @@
> +LOCAL_INCLUDES += [
> +    "../../content/src",
> +    "/content/base/src",
> +    "/dom/base",
> +"/layout/xul/tree/",

fix indentation
Attachment #803557 - Flags: review?(ted) → review+
> I think we should probably set a guideline for when to use relative vs.
> "absolute" (topsrcdir-relative) paths in moz.build variables. My suggestion
> would be "one parent-dir (..) in the path is okay, any more and you should
> use a topsrcdir-absolute path".

sure, I think I was using something like "whatever seems most readable", which I htink ended up being more or less the same thing, so that sounds fine to me.

> ::: content/xbl/src/moz.build
> @@ +44,5 @@
> >  
> >  LIBRARY_NAME = 'gkconxbl_s'
> >  
> > +LOCAL_INCLUDES += [
> > +    "../../xul/content/src",
> 
> Any reason this isn't an absolute path?

I'm not aware of one
> >deleted file mode 100644
> >index 4b2b8c1..0000000
> >--- a/docshell/build/Makefile.in
> >+++ /dev/null
> >@@ -1,28 +0,0 @@
> >-# 
> >-# 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/.
> >-
> >-EXPORT_LIBRARY = 1
> >-SHARED_LIBRARY_LIBS= \
> >-		../base/$(LIB_PREFIX)basedocshell_s.$(LIB_SUFFIX) \
> >-		$(DEPTH)/uriloader/base/$(LIB_PREFIX)uriloaderbase_s.$(LIB_SUFFIX) \
> >-		$(DEPTH)/uriloader/exthandler/$(LIB_PREFIX)exthandler_s.$(LIB_SUFFIX) \
> >-		$(DEPTH)/uriloader/prefetch/$(LIB_PREFIX)prefetch_s.$(LIB_SUFFIX) \
> >-		../shistory/src/$(LIB_PREFIX)shistory_s.$(LIB_SUFFIX) \
> >-		$(NULL)
> 
> Removing libdocshell and moving the SHARED_LIBRARY_LIBS to toolkit/library
> seems unrelated to this bug. Was this meant to be under a different bug? If
> not, can we make it part of a different bug? I don't think moving that is as
> straightforward as moving the includes to moz.build.

it is becauses its the easiest way of getting rid of that makefile.  Actually it is pretty trivial, it just effects effects what goes into which fake lib, which effects the exact order some stuff is linked into libxul, but not much more.  I'm hopeful we can nuke all this SHARED_LIBRARY_LIBS for stuff going into libxul rsn.

> >diff --git a/docshell/build/moz.build b/docshell/build/moz.build
> >index 3d9c503..8c6ecbc 100644
> >--- a/docshell/build/moz.build
> >+++ b/docshell/build/moz.build
> >@@ -13,8 +13,18 @@ EXPORTS += [
> > CPP_SOURCES += [
> >     'nsDocShellModule.cpp',
> > ]
> > 
> > LIBRARY_NAME = 'docshell'
> > 
> > LIBXUL_LIBRARY = True
> > 
> >+LOCAL_INCLUDES += [
> >+    "../base",
> >+    "../shistory/src/",
> >+    "/uriloader/base",
> >+    "/uriloader/exthandler",
> >+    "/uriloader/prefetch",
> >+    ]
> 
> nit: Most strings in mozbuild use single-quotes, not double-quotes.

erg, I forgot python is weird ;)

> 
> >+if CONFIG["MOZ_WIDGET_TOOLKIT"] == "cocoa":
> >+    LOCAL_INCLUDES += ["/uriloader/exthandler/mac"]
> 
> nit: Prefer using:
> 
> if CONFIG...
>     LOCAL_INCLUDES += [
>         '/uriloader/exthandler/mac',
>     ]
> 
> So it is easier to add more entries later.

I'm not convinced its worth the wasted space, but *shrug*
(In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > >deleted file mode 100644
> > >index 4b2b8c1..0000000
> > >--- a/docshell/build/Makefile.in
> > >+++ /dev/null
> > >@@ -1,28 +0,0 @@
> > >-# 
> > >-# 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/.
> > >-
> > >-EXPORT_LIBRARY = 1
> > >-SHARED_LIBRARY_LIBS= \
> > >-		../base/$(LIB_PREFIX)basedocshell_s.$(LIB_SUFFIX) \
> > >-		$(DEPTH)/uriloader/base/$(LIB_PREFIX)uriloaderbase_s.$(LIB_SUFFIX) \
> > >-		$(DEPTH)/uriloader/exthandler/$(LIB_PREFIX)exthandler_s.$(LIB_SUFFIX) \
> > >-		$(DEPTH)/uriloader/prefetch/$(LIB_PREFIX)prefetch_s.$(LIB_SUFFIX) \
> > >-		../shistory/src/$(LIB_PREFIX)shistory_s.$(LIB_SUFFIX) \
> > >-		$(NULL)
> > 
> > Removing libdocshell and moving the SHARED_LIBRARY_LIBS to toolkit/library
> > seems unrelated to this bug. Was this meant to be under a different bug? If
> > not, can we make it part of a different bug? I don't think moving that is as
> > straightforward as moving the includes to moz.build.
> 
> it is becauses its the easiest way of getting rid of that makefile. 
> Actually it is pretty trivial, it just effects effects what goes into which
> fake lib, which effects the exact order some stuff is linked into libxul,
> but not much more.  I'm hopeful we can nuke all this SHARED_LIBRARY_LIBS for
> stuff going into libxul rsn.

We already have mozbuild support for EXPORT_LIBRARY and SHARED_LIBRARY_LIBS though, right? Why not just move those over rather than reorganize how things are linked?
(In reply to Michael Shal [:mshal] from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> > > >deleted file mode 100644
> > > >index 4b2b8c1..0000000
> > > >--- a/docshell/build/Makefile.in
> > > >+++ /dev/null
> > > >@@ -1,28 +0,0 @@
> > > >-# 
> > > >-# 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/.
> > > >-
> > > >-EXPORT_LIBRARY = 1
> > > >-SHARED_LIBRARY_LIBS= \
> > > >-		../base/$(LIB_PREFIX)basedocshell_s.$(LIB_SUFFIX) \
> > > >-		$(DEPTH)/uriloader/base/$(LIB_PREFIX)uriloaderbase_s.$(LIB_SUFFIX) \
> > > >-		$(DEPTH)/uriloader/exthandler/$(LIB_PREFIX)exthandler_s.$(LIB_SUFFIX) \
> > > >-		$(DEPTH)/uriloader/prefetch/$(LIB_PREFIX)prefetch_s.$(LIB_SUFFIX) \
> > > >-		../shistory/src/$(LIB_PREFIX)shistory_s.$(LIB_SUFFIX) \
> > > >-		$(NULL)
> > > 
> > > Removing libdocshell and moving the SHARED_LIBRARY_LIBS to toolkit/library
> > > seems unrelated to this bug. Was this meant to be under a different bug? If
> > > not, can we make it part of a different bug? I don't think moving that is as
> > > straightforward as moving the includes to moz.build.
> > 
> > it is becauses its the easiest way of getting rid of that makefile. 
> > Actually it is pretty trivial, it just effects effects what goes into which
> > fake lib, which effects the exact order some stuff is linked into libxul,
> > but not much more.  I'm hopeful we can nuke all this SHARED_LIBRARY_LIBS for
> > stuff going into libxul rsn.
> 
> We already have mozbuild support for EXPORT_LIBRARY and SHARED_LIBRARY_LIBS
> though, right? Why not just move those over rather than reorganize how
> things are linked?

honestly it seems easier and like less of a waste of effort to do it this way.  That said I'm happy to make someone else who understands the fakelibs sillyness we have review it.
Attachment #804710 - Flags: review?(ted)
Attachment #803557 - Attachment is obsolete: true
Attachment #804712 - Flags: review?(mshal)
Attachment #804713 - Flags: review?(gps)
Attachment #804714 - Flags: review?(mh+mozilla)
Attachment #804715 - Flags: review?(mshal)
Attachment #804716 - Flags: review?(gps)
Comment on attachment 804710 [details] [diff] [review]
rm some makefiles in docshell/

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

::: docshell/shistory/src/moz.build
@@ +25,5 @@
>  
>  MSVC_ENABLE_PGO = True
>  
> +LOCAL_INCLUDES += [
> +    '../../base',

nit: would probably just make this absolute

::: toolkit/library/Makefile.in
@@ +176,5 @@
> +		$(DEPTH)/uriloader/exthandler/$(LIB_PREFIX)exthandler_s.$(LIB_SUFFIX) \
> +		$(DEPTH)/uriloader/prefetch/$(LIB_PREFIX)prefetch_s.$(LIB_SUFFIX) \
> +		$(DEPTH)/docshell/shistory/src/$(LIB_PREFIX)shistory_s.$(LIB_SUFFIX) \
> +		$(DEPTH)/docshell/build/$(LIB_PREFIX)docshell.$(LIB_SUFFIX) \
> +		$(NULL)

nit: two-space indent, not tabs.
Attachment #804710 - Flags: review?(ted) → review+
Comment on attachment 804712 [details] [diff] [review]
rm another makefile in content/

Looks good! Please just double-check on try if you haven't already, since going from INCLUDES to LOCAL_INCLUDES puts the -I flags in a different order (I think).
Attachment #804712 - Flags: review?(mshal) → review+
Comment on attachment 804715 [details] [diff] [review]
rm makefiles in intl/

Hooray!
Attachment #804715 - Flags: review?(mshal) → review+
Attachment #804713 - Flags: review?(gps) → review+
Comment on attachment 804716 [details] [diff] [review]
rm makefiles in dom/

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

At this point, what purpose does dom-config.mk serve? Surely it will be going away soon.

I'm actually surprised these files continue to build with dom-config.mk removed. It seems to me we got lazy in dom land and just cargo culted a massive LOCAL_INCLUDES around via dom-config.mk?
Attachment #804716 - Flags: review?(gps) → review+
> At this point, what purpose does dom-config.mk serve? Surely it will be
> going away soon.

afaict just adding a bunch of LOCAL_INCLUDES, and I sure hope /  expect it will die soon

> I'm actually surprised these files continue to build with dom-config.mk
> removed. It seems to me we got lazy in dom land and just cargo culted a

Well, a bunch of those dirs just have idl and don't actually compile anything.

> massive LOCAL_INCLUDES around via dom-config.mk?

seems so, unfortunately a lot of it is in b2g only dirs so fixing it is a bit annoying :/
Comment on attachment 804714 [details] [diff] [review]
rm  makefiles in xpcom/

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

::: xpcom/threads/moz.build
@@ +55,5 @@
>  
>  MSVC_ENABLE_PGO = True
>  
> +LOCAL_INCLUDES += [
> +    '../build',

what happened to ../components?
Attachment #804714 - Flags: review?(mh+mozilla) → review+
(In reply to Mike Hommey [:glandium] from comment #21)
> Comment on attachment 804714 [details] [diff] [review]
> rm  makefiles in xpcom/
> 
> Review of attachment 804714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/threads/moz.build
> @@ +55,5 @@
> >  
> >  MSVC_ENABLE_PGO = True
> >  
> > +LOCAL_INCLUDES += [
> > +    '../build',
> 
> what happened to ../components?

got lost somehow apparently (but it still builds atleast on linux)  think I should add it back anyway?
(In reply to Trevor Saunders (:tbsaunde) from comment #22)
> > what happened to ../components?
> 
> got lost somehow apparently (but it still builds atleast on linux)  think I
> should add it back anyway?

If it passes try, ok.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.