875013 - Eliminate VPATH usage

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Gregory Szorc [:gps]]

Ted, Joey, Mike, and I just decided we all don't like VPATH and should eliminate it from the build system.

Comment 1

[Gregory Szorc [:gps]]

To clarify, we want to eliminate it from user-facing Makefile.in. Obviously we still need VPATH = $(srcdir) somewhere to facilitate objdir builds :)

Path forward is likely to eliminate the |VPATH = @srcdir@| boilerplate, adding it to rules.mk. Eventually we'll chip away at all the remaining multi-uses of VPATH. Also, we may carve out few exceptions (like for l10n foo) where VPATH might make sense.

The impetus behind this is VPATH is confusing for some build backends and will hamper moz.build conversion.

Comment 2

[Benjamin Smedberg]

Actually, I'm not sure we do need VPATH=srcdir. We should be able to assume that most CPPSRCS live in $(srcdir)/name and for generated files use a separate rule GENERATED_CPPSRCS or something.

I think that VPATH=srcdir is bad enough that we should try to remove it with everything else.

Comment 3

[Michael Shal [:mshal]]

Attachment: Eliminate VPATH usage (part 1 - XPIDL_SOURCES)

This patch removes VPATH usage from XPIDL_SOURCES, which appears to only be a single moz.build file. The rules are adjusted to support XPIDL_SOURCES with subdirectories - there might be a better way to do this, but this seems happy so far in try.

Comment 4

[Michael Shal [:mshal]]

Adding [leave open] since this will be done in parts - it's a lot to do all at once.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 767934 [details] [diff] [review]
Eliminate VPATH usage (part 1 - XPIDL_SOURCES)

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

I already fixed this in bug 850380, so this just bit rots me. Let's fry a different fish.

Comment 6

[Michael Shal [:mshal]]

Attachment: Eliminate VPATH usage (part 1 - EXPORTS)

Ok, I'll ignore XPIDL_SOURCES for now :). Here's a different patch for getting rid of VPATH with EXPORTS. Right now this just changes the EXPORTS to use relative paths rather than relying on VPATHS - it doesn't actually remove the VPATH definitions from Makefile.in yet. We'll need to clean those up after CPP_SOURCES and such are converted as well.

One weird thing I noticed is that in toolkit/components/protobuf, there are two package_info.h files. Due to the ordering of VPATH, the one in google/protobuf/ gets installed into the protobuf.google.protobuf.io namespace, not the one in google/protobuf/io. All other files in that namespace are from google/protobuf/io. Not sure if that was intentional, but this patch preserves the existing rules.

Comment 7

[Joey Armstrong [:joey]]

Comment on attachment 768590 [details] [diff] [review]
Eliminate VPATH usage (part 1 - EXPORTS)

dom/mobilemessage/src/moz.build
===============================
     EXPORTS.mozilla.dom.mobilemessage += [
-        'MmsService.h',
-        'MobileMessageDatabaseService.h',
+        mmsdir + 'MmsService.h',
+        mmsdir + 'MobileMessageDatabaseService.h',

The convention has been to use output formatting in place of string concatenation {which also makes the directory slash visible}:
  "%s/%s" % (mmsdir, fyl)

add list comprehension and files-per-directory can be inlined.


Is a try/build job on the way for this patch ?

Comment 8

[Michael Shal [:mshal]]

Current try results are here:

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

I'll update the patch to use %s/%s.

Comment 9

[Michael Shal [:mshal]]

Attachment: Eliminate VPATH usage (part 1 - EXPORTS)

Updated to use %s/%s. Carrying forward r=joey.

Latest try results: https://tbpl.mozilla.org/?tree=Try&rev=496a359945a8

Comment 10

[Michael Shal [:mshal]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/25be544ccd04

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25be544ccd04

Comment 12

[Ed Morley [:emorley]]

Sadly still a fair number remaining:

http://mxr.mozilla.org/mozilla-central/search?string=VPATH&case=1&find=Makefile\.in&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Found 180 matching lines in 102 files

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Eliminate VPATH usage (mobilemessage)

Completely remove VPATH usage in dom/mobilemessage.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 802263 [details] [diff] [review]
Eliminate VPATH usage (mobilemessage)

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

Comment 15

[Joey Armstrong [:joey]]

Comment on attachment 802263 [details] [diff] [review]
Eliminate VPATH usage (mobilemessage)

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

The patch looks good but there is one minor edit to make that will simplify future maintenance:

 #include "mozilla/dom/ContentParent.h"
+#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
 #include "mozilla/dom/mobilemessage/SmsTypes.h"

Using long/explicit paths would force having to edit source if/when a directory is renamed.


moz.build::LOCAL_INCLUDE can be used to shorten the inculde path and store the common substring in one place.  If a unique path is needed to include the correct Constants.h s only specify down to dom/:

LOCAL_INCLUDES += [
    'mozilla/dom',                # include "mobilemessage/Constants.h"
    'mozilla/dom/mobilemessage',  # include "Constants.h"
]

Comment 16

[Joey Armstrong [:joey]]

(In reply to Joey Armstrong [:joey] from comment #15)
> Comment on attachment 802263 [details] [diff] [review]
> Eliminate VPATH usage (mobilemessage)
> 
> Review of attachment 802263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good but there is one minor edit to make that will simplify
> future maintenance:
> 
>  #include "mozilla/dom/ContentParent.h"
> +#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
>  #include "mozilla/dom/mobilemessage/SmsTypes.h"
> 
> Using long/explicit paths would force having to edit source if/when a
> directory is renamed.
> 
> 
> moz.build::LOCAL_INCLUDE can be used to shorten the inculde path and store
> the common substring in one place.  If a unique path is needed to include
> the correct Constants.h s only specify down to dom/:
> 
> LOCAL_INCLUDES += [
>     'mozilla/dom',                # include "mobilemessage/Constants.h"
>     'mozilla/dom/mobilemessage',  # include "Constants.h"
> ]

The variable should be plural LOCAL_INDLUDES (mozbuild::LOCAL_INCLUDE):
  python/mozbuild/mozbuild/frontend/sandbox_symbols.py:    'LOCAL_INCLUDES':

Comment 17

[Gregory Szorc [:gps]]

(In reply to Joey Armstrong [:joey] from comment #15)
> Comment on attachment 802263 [details] [diff] [review]
> Eliminate VPATH usage (mobilemessage)
> 
> Review of attachment 802263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good but there is one minor edit to make that will simplify
> future maintenance:
> 
>  #include "mozilla/dom/ContentParent.h"
> +#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
>  #include "mozilla/dom/mobilemessage/SmsTypes.h"
> 
> Using long/explicit paths would force having to edit source if/when a
> directory is renamed.
> 
> 
> moz.build::LOCAL_INCLUDE can be used to shorten the inculde path and store
> the common substring in one place.  If a unique path is needed to include
> the correct Constants.h s only specify down to dom/:
> 
> LOCAL_INCLUDES += [
>     'mozilla/dom',                # include "mobilemessage/Constants.h"
>     'mozilla/dom/mobilemessage',  # include "Constants.h"
> ]

I'm pretty sure our C++ people would frown upon this practice. There are some basenames that exist in multiple directories and having multiple $(DIST)/include subdirectories on the include search path would only introduce ambiguity. Not to mention it makes the compiler work harder to find files.

Comment 18

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #17)
> I'm pretty sure our C++ people would frown upon this practice. There are
> some basenames that exist in multiple directories and having multiple
> $(DIST)/include subdirectories on the include search path would only
> introduce ambiguity. Not to mention it makes the compiler work harder to
> find files.

I agree - although it does seem handy to only change a single -I flag (per directory) if an include directory is renamed, I think those changes can easily be scripted when needed. If we really want a flat namespace, we should put the .h files into a single directory rather than use -I to give the appearance of one. I would rather see fewer LOCAL_INCLUDES, not more.

Comment 19

[Trevor Saunders (:tbsaunde)]

LOCAL_INCLUDES should generally be avoided, and using it like that would be an unacceptable hack, not even considering that doesn't really make anything easier if someone were to go crazy and decide such a file should be exported someplace else.

Comment 20

[Mike Hommey [:glandium]]

Note the mozilla/foo/bar type of include paths reflect C++ namespaces, not directories.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Joey Armstrong [:joey] from comment #15)
> Review of attachment 802263 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good but there is one minor edit to make that will simplify
> future maintenance:
> 
>  #include "mozilla/dom/ContentParent.h"
> +#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
>  #include "mozilla/dom/mobilemessage/SmsTypes.h"
> 
> Using long/explicit paths would force having to edit source if/when a
> directory is renamed.

The reason I have to export "Constants.h" to 'mozilla/dom/mobilemessage' is we have other "Constants.h" files in 'mfbt', 'dom/battery' and 'dom/network/src'.  We have to either rename that file to some unique name or, like what's done here, export it to an unique namespace.

Comment 22

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Mike Hommey [:glandium] from comment #20)
> Note the mozilla/foo/bar type of include paths reflect C++ namespaces, not
> directories.

It is.  We use mozilla::dom::mobilemessage for non-DOM classes like IPDL parent/child, factory, etc.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Sounds like there are other opinions about using LOCAL_INCLUDES as a replacement, ni for further work to do.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> The reason I have to export "Constants.h" to 'mozilla/dom/mobilemessage' is
> we have other "Constants.h" files in 'mfbt', 'dom/battery' and
> 'dom/network/src'.  We have to either rename that file to some unique name
> or, like what's done here, export it to an unique namespace.

BTW, 'mfbt/Constants.h' is exported as 'mozilla/Constants.h', 'dom/battery/Constants.h' as 'mozilla/dom/battery/Constants.h', and 'dom/network/src' as 'mozilla/dom/network/Constants.h'.

Comment 25

[(no longer active)]

(In reply to Gregory Szorc [:gps] from comment #17)
> (In reply to Joey Armstrong [:joey] from comment #15)
> > Comment on attachment 802263 [details] [diff] [review]
> > Eliminate VPATH usage (mobilemessage)
> > 
> > Review of attachment 802263 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > The patch looks good but there is one minor edit to make that will simplify
> > future maintenance:
> > 
> >  #include "mozilla/dom/ContentParent.h"
> > +#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
> >  #include "mozilla/dom/mobilemessage/SmsTypes.h"
> > 
> > Using long/explicit paths would force having to edit source if/when a
> > directory is renamed.
> > 
> > 
> > moz.build::LOCAL_INCLUDE can be used to shorten the inculde path and store
> > the common substring in one place.  If a unique path is needed to include
> > the correct Constants.h s only specify down to dom/:
> > 
> > LOCAL_INCLUDES += [
> >     'mozilla/dom',                # include "mobilemessage/Constants.h"
> >     'mozilla/dom/mobilemessage',  # include "Constants.h"
> > ]
> 
> I'm pretty sure our C++ people would frown upon this practice. There are
> some basenames that exist in multiple directories and having multiple
> $(DIST)/include subdirectories on the include search path would only
> introduce ambiguity. Not to mention it makes the compiler work harder to
> find files.

Yeah, let me reflect what others have said so far as well!  Please don't do this, we really want these #include paths to reflect C++ namespaces, and to avoid name clashes with other files in the tree as Vicamo mentions.

Comment 26

[Joey Armstrong [:joey]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)
> (In reply to Gregory Szorc [:gps] from comment #17)
> > (In reply to Joey Armstrong [:joey] from comment #15)
> > > Comment on attachment 802263 [details] [diff] [review]
> > > Eliminate VPATH usage (mobilemessage)
> > > 
> > > Review of attachment 802263 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > The patch looks good but there is one minor edit to make that will simplify
> > > future maintenance:
> > > 
> > >  #include "mozilla/dom/ContentParent.h"
> > > +#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
> > >  #include "mozilla/dom/mobilemessage/SmsTypes.h"
> > > 
> > > Using long/explicit paths would force having to edit source if/when a
> > > directory is renamed.
> > > 
> > > 
> > > moz.build::LOCAL_INCLUDE can be used to shorten the inculde path and store
> > > the common substring in one place.  If a unique path is needed to include
> > > the correct Constants.h s only specify down to dom/:
> > > 
> > > LOCAL_INCLUDES += [
> > >     'mozilla/dom',                # include "mobilemessage/Constants.h"
> > >     'mozilla/dom/mobilemessage',  # include "Constants.h"
> > > ]
> > 
> > I'm pretty sure our C++ people would frown upon this practice. There are
> > some basenames that exist in multiple directories and having multiple
> > $(DIST)/include subdirectories on the include search path would only
> > introduce ambiguity. Not to mention it makes the compiler work harder to
> > find files.
> 
> Yeah, let me reflect what others have said so far as well!  Please don't do
> this, we really want these #include paths to reflect C++ namespaces, and to
> avoid name clashes with other files in the tree as Vicamo mentions.

Ambiguous usually implies not having a fully isolated hierarchy to including only what you want when you want it.  Circular deps, deps between directories, etc.  If we have chosen to address that problem by coding explicit include paths into every source file with the expectation of having to modify source and rebuild all dependent content whenever directories are moved I'm fine with that.  The suggestion to use LOCAL_INCLUDES was only to reduce rebuilding and this type of maintenance.

Comment 27

[(no longer active)]

(In reply to comment #26)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #25)
> > (In reply to Gregory Szorc [:gps] from comment #17)
> > > (In reply to Joey Armstrong [:joey] from comment #15)
> > > > Comment on attachment 802263 [details] [diff] [review]
> > > > Eliminate VPATH usage (mobilemessage)
> > > > 
> > > > Review of attachment 802263 [details] [diff] [review]:
> > > > -----------------------------------------------------------------
> > > > 
> > > > The patch looks good but there is one minor edit to make that will simplify
> > > > future maintenance:
> > > > 
> > > >  #include "mozilla/dom/ContentParent.h"
> > > > +#include "mozilla/dom/mobilemessage/Constants.h" // For MessageType
> > > >  #include "mozilla/dom/mobilemessage/SmsTypes.h"
> > > > 
> > > > Using long/explicit paths would force having to edit source if/when a
> > > > directory is renamed.
> > > > 
> > > > 
> > > > moz.build::LOCAL_INCLUDE can be used to shorten the inculde path and store
> > > > the common substring in one place.  If a unique path is needed to include
> > > > the correct Constants.h s only specify down to dom/:
> > > > 
> > > > LOCAL_INCLUDES += [
> > > >     'mozilla/dom',                # include "mobilemessage/Constants.h"
> > > >     'mozilla/dom/mobilemessage',  # include "Constants.h"
> > > > ]
> > > 
> > > I'm pretty sure our C++ people would frown upon this practice. There are
> > > some basenames that exist in multiple directories and having multiple
> > > $(DIST)/include subdirectories on the include search path would only
> > > introduce ambiguity. Not to mention it makes the compiler work harder to
> > > find files.
> > 
> > Yeah, let me reflect what others have said so far as well!  Please don't do
> > this, we really want these #include paths to reflect C++ namespaces, and to
> > avoid name clashes with other files in the tree as Vicamo mentions.
> 
> Ambiguous usually implies not having a fully isolated hierarchy to including
> only what you want when you want it.  Circular deps, deps between directories,
> etc.  If we have chosen to address that problem by coding explicit include
> paths into every source file with the expectation of having to modify source
> and rebuild all dependent content whenever directories are moved I'm fine with
> that.  The suggestion to use LOCAL_INCLUDES was only to reduce rebuilding and
> this type of maintenance.

Moving directories is a very uncommon operation, and I don't think we need to optimize for that.  Also, the problem of circular dependencies in C/C++ is very commonly handled by #include guards and forward declarations, so that's also not a problem which we need to solve here.

Comment 28

[Joey Armstrong [:joey]]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> LOCAL_INCLUDES should generally be avoided, and using it like that would be
> an unacceptable hack, not even considering that doesn't really make anything
> easier if someone were to go crazy and decide such a file should be exported
> someplace else.

Why would having an expectation for proper hierarchy and imports be a 'hack' ?  If/when content is moved around or exported to another directory explicit paths in source will pretty much require having to rebulid unrelated. content.

Comment 29

[Joey Armstrong [:joey]]

Comment on attachment 802263 [details] [diff] [review]
Eliminate VPATH usage (mobilemessage)

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

not sure why this review was redirected elsewhere.

Comment 30

[Joey Armstrong [:joey]]

Comment on attachment 802263 [details] [diff] [review]
Eliminate VPATH usage (mobilemessage)

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

Patch is fine if consensus is to use explicit paths in source rather than LOCAL_INCLUDES

Comment 31

[Joshua Cranmer [:jcranmer]]

(In reply to Joey Armstrong [:joey] from comment #28)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > LOCAL_INCLUDES should generally be avoided, and using it like that would be
> > an unacceptable hack, not even considering that doesn't really make anything
> > easier if someone were to go crazy and decide such a file should be exported
> > someplace else.
> 
> Why would having an expectation for proper hierarchy and imports be a 'hack'
> ?  If/when content is moved around or exported to another directory explicit
> paths in source will pretty much require having to rebulid unrelated.
> content.

The directory names are meant to be the namespace of all declarations within the header (as enforced by social peer pressure). If you move the header, that means that you are changing the namespace of everyone within the header, and you need to modify everyone who includes it anyways.

Comment 32

[Joey Armstrong [:joey]]

Oh one thought - it might be good to document some of the content from this discussion in a wiki page for posterity.  If the topic surfaces again the page can be referenced and content added.

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/fa066cc5c1f9

Comment 34

[Trevor Saunders (:tbsaunde)]

(In reply to Joey Armstrong [:joey] from comment #28)
> (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > LOCAL_INCLUDES should generally be avoided, and using it like that would be
> > an unacceptable hack, not even considering that doesn't really make anything
> > easier if someone were to go crazy and decide such a file should be exported
> > someplace else.
> 
> Why would having an expectation for proper hierarchy and imports be a 'hack'
> ?  If/when content is moved around or exported to another directory explicit
> paths in source will pretty much require having to rebulid unrelated.
> content.

and if you encode those paths in Makefiles you'll end up rebuilding the all the content in every directory that has a file including  the moved header, because you'll need to update the LOCAL_INCLUDES which will mean the Makefile is newer than any of the object files it produced

Comment 35

[Joey Armstrong [:joey]]

(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> (In reply to Joey Armstrong [:joey] from comment #28)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #19)
> > > LOCAL_INCLUDES should generally be avoided, and using it like that would be
> > > an unacceptable hack, not even considering that doesn't really make anything
> > > easier if someone were to go crazy and decide such a file should be exported
> > > someplace else.
> > 
> > Why would having an expectation for proper hierarchy and imports be a 'hack'
> > ?  If/when content is moved around or exported to another directory explicit
> > paths in source will pretty much require having to rebulid unrelated.
> > content.
> 
> and if you encode those paths in Makefiles you'll end up rebuilding the all
> the content in every directory that has a file including  the moved header,
> because you'll need to update the LOCAL_INCLUDES which will mean the
> Makefile is newer than any of the object files it produced

Ok so again why would this be a hack ?  The behavior seems pretty standard - building is forced through a dependency change.  The only delta I can see compared with the bug patch is having the include path stored in one place -vs- being distributed through the source files.

Comment 36

[Gregory Szorc [:gps]]

I consider Ehsan's statement in comment #25 to be the authority on this matter: don't abuse LOCAL_INCLUDES to perform C++/header "namespacing" that should be performed in source files.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa066cc5c1f9

Comment 38

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH-from-hal.patch

Try results: https://tbpl.mozilla.org/?tree=Try&rev=36dd893d6100

Not sure what is up with the "Gu" thing in Linux x64 Opt.

Comment 39

[Joey Armstrong [:joey]]

Comment on attachment 807239 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-from-hal.patch

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

The subdir moves look ok after a quick review but the patch may need a refresh to apply cleanly.

applying bug-875013-eliminate-vpath-mobile-message.patch
patching file dom/mobilemessage/src/Makefile.in
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file dom/mobilemessage/src/Makefile.in.rej
patching file dom/mobilemessage/src/MmsMessage.cpp
Hunk #1 FAILED at 2
1 out of 1 hunks FAILED -- saving rejects to file dom/mobilemessage/src/MmsMessage.cpp.rej
patching file dom/mobilemessage/src/MobileMessageManager.cpp
Hunk #1 FAILED at 5
1 out of 1 hunks FAILED -- saving rejects to file dom/mobilemessage/src/MobileMessageManager.cpp.rej
patching file dom/mobilemessage/src/MobileMessageThread.cpp
Hunk #1 FAILED at 3
1 out of 1 hunks FAILED -- saving rejects to file dom/mobilemessage/src/MobileMessageThread.cpp.rej

Comment 40

[Michael Shal [:mshal]]

(In reply to Joey Armstrong [:joey] from comment #39)
> Comment on attachment 807239 [details] [diff] [review]
> 0001-Bug-875013-Remove-VPATH-from-hal.patch
> 
> Review of attachment 807239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The subdir moves look ok after a quick review but the patch may need a
> refresh to apply cleanly.
> 
> applying bug-875013-eliminate-vpath-mobile-message.patch
> patching file dom/mobilemessage/src/Makefile.in
> Hunk #1 FAILED at 0
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/mobilemessage/src/Makefile.in.rej
> patching file dom/mobilemessage/src/MmsMessage.cpp
> Hunk #1 FAILED at 2
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/mobilemessage/src/MmsMessage.cpp.rej
> patching file dom/mobilemessage/src/MobileMessageManager.cpp
> Hunk #1 FAILED at 5
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/mobilemessage/src/MobileMessageManager.cpp.rej
> patching file dom/mobilemessage/src/MobileMessageThread.cpp
> Hunk #1 FAILED at 3
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/mobilemessage/src/MobileMessageThread.cpp.rej

What patch are you applying? The one under review only edits hal/Makefile.in and hal/moz.build - there are no changes under dom/, so I don't know why you would get those failures.

Comment 41

[Joey Armstrong [:joey]]

Comment on attachment 807239 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-from-hal.patch

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

Patch does apply cleanly on a freshly checked out sandbox so carry the feedback review forward.

Comment 42

[Michael Shal [:mshal]]

hal patch inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/28316508a37f

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/28316508a37f

Comment 44

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH-from-dom.patch

Ms2ger already did most of the work here, so most of the VPATHs can just disappear. This patch just covers the dom/ directory.

Try results:

https://tbpl.mozilla.org/?tree=Try&rev=db094187400c (b2g broken because of a missing LOCAL_INCLUDES in dom/bluetooth)

https://tbpl.mozilla.org/?tree=Try&rev=5810761fad9b (with fixed LOCAL_INCLUDES)

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Feel free to put new LOCAL_INCLUDES in moz.build ;)

Comment 46

[Joey Armstrong [:joey]]

Comment on attachment 819170 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-from-dom.patch

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

explicit path expansion looks good.

Comment 47

[Michael Shal [:mshal]]

(In reply to :Ms2ger from comment #45)
> Feel free to put new LOCAL_INCLUDES in moz.build ;)

That would've been smart :). But it seems like we don't have a strategy yet for includes that come from configure? The dom/bluetooth/Makefile.in adds MOZ_DBUS_CFLAGS to LOCAL_INCLUDES, so it can't go straight into moz.build.

Comment 48

[Trevor Saunders (:tbsaunde)]

(In reply to Michael Shal [:mshal] from comment #47)
> (In reply to :Ms2ger from comment #45)
> > Feel free to put new LOCAL_INCLUDES in moz.build ;)
> 
> That would've been smart :). But it seems like we don't have a strategy yet
> for includes that come from configure? The dom/bluetooth/Makefile.in adds
> MOZ_DBUS_CFLAGS to LOCAL_INCLUDES, so it can't go straight into moz.build.

We should just fix that to  add to CFLAGS / CXXFLAGS

Comment 49

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH.patch

Most of the hard work was already done in bug 912099 - this removes the now useless VPATH declarations.

try: https://tbpl.mozilla.org/?tree=Try&rev=05b5c1f2932d

Comment 50

[Mike Hommey [:glandium]]

Comment on attachment 8339694 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH.patch

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

\o/

Comment 51

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8339694 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH.patch

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

::: uriloader/exthandler/Makefile.in
@@ -24,5 @@
>  endif
>  endif
>  endif
>  
> -VPATH		:= $(srcdir) $(srcdir)/$(OSDIR)

I think that was the last use of OSDIR, and you can kill the if cascade above.

Comment 52

[Michael Shal [:mshal]]

(In reply to :Ms2ger from comment #51)
> I think that was the last use of OSDIR, and you can kill the if cascade
> above.

Good catch - I'll take that out too :)

Comment 53

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH.patch

Removed OSDIR & rebased. r+ carried forward

Comment 54

[Michael Shal [:mshal]]

latest try: https://tbpl.mozilla.org/?tree=Try&rev=b81d097f071b
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/91b5e3127b76

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/91b5e3127b76

Comment 56

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH-in-gfx.patch

This removes VPATH from gfx/. The use of these paths has already been fixed in other bugs. Try results: https://tbpl.mozilla.org/?tree=Try&rev=aeaa8a6eef9b

Comment 57

[Joey Armstrong [:joey]]

Comment on attachment 8356844 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-in-gfx.patch

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

vpath assignment, removal looks fine.

Comment 58

[Michael Shal [:mshal]]

gfx inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d2f31fdc4d4

Comment 59

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH-in-media-libvpx.patch

Comment 60

[Michael Shal [:mshal]]

Attachment: 0002-Bug-875013-Remove-VPATH-in-media-libopus.patch

This generates the sources.mozbuild fragment from update.sh and kills the rest of media/libvpx/Makefile.in

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d2f31fdc4d4

Comment 62

[Joey Armstrong [:joey]]

Comment on attachment 8358006 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-in-media-libvpx.patch

vpath removal looks good.

nit> if moz.build files are following python convention the "SOURCES += cpufeatures.c" will need to be split.

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
  >> moz.build are python and follow normal Python style
http://www.python.org/dev/peps/pep-0008/#maximum-line-length
  >> Maximum Line Length
  >> Limit all lines to a maximum of 79 characters.

Comment 63

[Gregory Szorc [:gps]]

Comment on attachment 8358009 [details] [diff] [review]
0002-Bug-875013-Remove-VPATH-in-media-libopus.patch

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

The check for sorted in moz.build processing uses:

    sorted(l, key=lambda x: x.lower())

If you rewrite sources.mozbuild generation inside Python, you can just sort via that before printing and you can avoid the SOURCES += sorted() mess. You could probably do it in shell as well. But just use Python, please.

::: media/libopus/celt_sources.mk
@@ -18,5 @@
> -celt/vq.c
> -
> -CELT_SOURCES_ARM = \
> -celt/arm/armcpu.c \
> -celt/arm/arm_celt_map.c

What happened to these files? I guess they're not used in any build config?

::: media/libopus/sources.mozbuild
@@ +1,1 @@
> +celt_sources = [

Please write a comment into the file that this file is automatically generated by XXX and it should not be modified directly.

::: media/libopus/update.mk
@@ +30,5 @@
> +	for var in $($(UPDATE_VARNAME)); do \
> +	echo "    '$$var',"; \
> +	done; \
> +	echo "]"; \
> +	echo ""

Why is this file a make file? Can't we just rewrite it as a shell function or preferably as a Python script?

::: media/libopus/update.sh
@@ +64,5 @@
>  sed -e "s/DEFINES\['OPUS_VERSION'\][ \t]*=[ \t]*'\".*\"'/DEFINES['OPUS_VERSION'] = '\"${version}-mozilla\"'/" \
>      ${TARGET}/moz.build > ${TARGET}/moz.build+ && \
>      mv ${TARGET}/moz.build+ ${TARGET}/moz.build
>  
> +make -f update.mk UPDATE_MAKEFILE="$1/celt_sources.mk" UPDATE_VARNAME="CELT_SOURCES" > ${TARGET}/sources.mozbuild+

'+' in a filename! Does that work everywhere? Why can't you simply use .tmp like every other script in the world? If you rewrote this as a Python script, you could just collect everything in memory :)

Comment 64

[Michael Shal [:mshal]]

inbound for media/libvpx: https://hg.mozilla.org/integration/mozilla-inbound/rev/92d510a09a8c

Comment 65

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #63)
> The check for sorted in moz.build processing uses:
> 
>     sorted(l, key=lambda x: x.lower())
> 
> If you rewrite sources.mozbuild generation inside Python, you can just sort
> via that before printing and you can avoid the SOURCES += sorted() mess. You
> could probably do it in shell as well. But just use Python, please.

Good point. The *_sources.mk files come from upstream libopus, and since make can parse them natively while Python cannot, I used the update.mk file for the conversion. But the *_sources.mk files are simple enough that it shouldn't be too hard to do in a python script.

I wasn't able to find out a way to get sort(1) to match moz.build's expected sorting. If you know the correct flags that match, please let me know. I've tried various combinations of using LC_ALL=C, and the -f flag, but it doesn't seem to match what moz.build is expecting. I still sometimes use sort from a vim macro in moz.build files, so it would be good to know even if it doesn't apply to this bug.

> 
> ::: media/libopus/celt_sources.mk
> @@ -18,5 @@
> > -celt/vq.c
> > -
> > -CELT_SOURCES_ARM = \
> > -celt/arm/armcpu.c \
> > -celt/arm/arm_celt_map.c
> 
> What happened to these files? I guess they're not used in any build config?

They are in upstream libopus, and used to be copied over in update.sh and included in our Makefile.in. Since this patch gets rid of the Makefile.in, we don't need to copy them over any more. They are just used from the upstream source to generate sources.mozbuild now - we don't need them after that.

> 
> ::: media/libopus/sources.mozbuild
> @@ +1,1 @@
> > +celt_sources = [
> 
> Please write a comment into the file that this file is automatically
> generated by XXX and it should not be modified directly.

Will do.

> 
> ::: media/libopus/update.mk
> @@ +30,5 @@
> > +	for var in $($(UPDATE_VARNAME)); do \
> > +	echo "    '$$var',"; \
> > +	done; \
> > +	echo "]"; \
> > +	echo ""
> 
> Why is this file a make file? Can't we just rewrite it as a shell function
> or preferably as a Python script?

As mentioned above, the source data is in make syntax, so it was the easiest way to grab it. I'll go with Python for the sorting, though.

> 
> ::: media/libopus/update.sh
> @@ +64,5 @@
> >  sed -e "s/DEFINES\['OPUS_VERSION'\][ \t]*=[ \t]*'\".*\"'/DEFINES['OPUS_VERSION'] = '\"${version}-mozilla\"'/" \
> >      ${TARGET}/moz.build > ${TARGET}/moz.build+ && \
> >      mv ${TARGET}/moz.build+ ${TARGET}/moz.build
> >  
> > +make -f update.mk UPDATE_MAKEFILE="$1/celt_sources.mk" UPDATE_VARNAME="CELT_SOURCES" > ${TARGET}/sources.mozbuild+
> 
> '+' in a filename! Does that work everywhere? Why can't you simply use .tmp
> like every other script in the world? If you rewrote this as a Python
> script, you could just collect everything in memory :)

I have no idea :). I just used '+' to match the rest of the script. giles, any preference?

Comment 66

[Michael Shal [:mshal]]

Comment on attachment 8358009 [details] [diff] [review]
0002-Bug-875013-Remove-VPATH-in-media-libopus.patch

New patch on the way.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92d510a09a8c

Comment 68

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-Remove-VPATH-in-media-libopus.patch

Here's the python approach - just requesting review from b:c for now.

Comment 69

[Gregory Szorc [:gps]]

Comment on attachment 8358706 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-in-media-libopus.patch

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

LGTM!

Comment 70

[Michael Shal [:mshal]]

Comment on attachment 8358706 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-in-media-libopus.patch

I'd like to get sign-off from :derf as well since this touches the update script.

Comment 71

[Timothy B. Terriberry (:derf)]

Comment on attachment 8358706 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-in-media-libopus.patch

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

r=me, but see the one question.

::: media/libopus/gen-sources.py
@@ +50,5 @@
> +                        # CELT_SOURCES = celt/bands.c
> +                        #
> +                        # So we treat the first group as the variable name and
> +                        # the second group as a potential value.
> +                        output.write('%s = [\n' % result.group(1).lower())

Any specific reason for the lower()? It'd be nice if these variables matched what was in the upstream Makefiles.

Comment 72

[Michael Shal [:mshal]]

(In reply to Timothy B. Terriberry (:derf) from comment #71)
> ::: media/libopus/gen-sources.py
> @@ +50,5 @@
> > +                        # CELT_SOURCES = celt/bands.c
> > +                        #
> > +                        # So we treat the first group as the variable name and
> > +                        # the second group as a potential value.
> > +                        output.write('%s = [\n' % result.group(1).lower())
> 
> Any specific reason for the lower()? It'd be nice if these variables matched
> what was in the upstream Makefiles.

Yeah, all-uppercase words are reserved for the "outputs" of the moz.build file. So things that are used by the backend like SOURCES and EXPORTS are all-uppercase, but local variables like celt_sources need to be lowercase.

Comment 73

[Timothy B. Terriberry (:derf)]

(In reply to Michael Shal [:mshal] from comment #72)
> Yeah, all-uppercase words are reserved for the "outputs" of the moz.build
> file. So things that are used by the backend like SOURCES and EXPORTS are
> all-uppercase, but local variables like celt_sources need to be lowercase.

Can you add a comment to that effect?

Comment 74

[Michael Shal [:mshal]]

(In reply to Timothy B. Terriberry (:derf) from comment #73)
> (In reply to Michael Shal [:mshal] from comment #72)
> > Yeah, all-uppercase words are reserved for the "outputs" of the moz.build
> > file. So things that are used by the backend like SOURCES and EXPORTS are
> > all-uppercase, but local variables like celt_sources need to be lowercase.
> 
> Can you add a comment to that effect?

Yep, done!

try: https://tbpl.mozilla.org/?tree=Try&rev=062a5163d0c8
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a98d3ec8f92

Comment 75

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a98d3ec8f92

Comment 76

[Michael Shal [:mshal]]

Attachment: 0001-Bug-875013-VPATH-removals-in-build-roboextender-cras.patch

I think this is the last of the VPATH changes. All the rest of the references seem to be in third-party code (nsprpub, libffi, etc) or standalone Makefiles (build/clang-plugin)

Comment 77

[(not currently active) Ted Mielczarek]

Comment on attachment 8363694 [details] [diff] [review]
0001-Bug-875013-VPATH-removals-in-build-roboextender-cras.patch

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

::: toolkit/crashreporter/breakpad-windows-libxul/moz.build
@@ +26,5 @@
> +
> +SOURCES += objs_common
> +SOURCES += objs_crash_generation
> +SOURCES += objs_handler
> +SOURCES += objs_sender

Presumably you could just write:
SOURCES += objs_common + objs_crash_generation ...

::: toolkit/crashreporter/breakpad-windows-standalone/Makefile.in
@@ +4,5 @@
>  
>  USE_STATIC_LIBS = 1
>  MOZ_GLUE_LDFLAGS =
>  
>  STL_FLAGS =

We should really move these things to moz.build.

::: toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/objs.mozbuild
@@ +12,5 @@
> +]
> +
> +subdir = 'toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation'
> +objs_crash_generation = [
> +    '%s/%s/%s' % (TOPSRCDIR, subdir, s) for s in lobjs_crash_generation

We should probably provide a helper in moz.build to do this. Can you file that as a followup?

::: toolkit/crashreporter/test/Makefile.in
@@ +6,3 @@
>  LOCAL_INCLUDES += \
>    -I$(srcdir)/../google-breakpad/src/ \
>    $(NULL)

You could have moved the LOCAL_INCLUDES while you were here, but no big deal.

Comment 78

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #77)
> Comment on attachment 8363694 [details] [diff] [review]
> 0001-Bug-875013-VPATH-removals-in-build-roboextender-cras.patch
> 
> Review of attachment 8363694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/crashreporter/breakpad-windows-libxul/moz.build
> @@ +26,5 @@
> > +
> > +SOURCES += objs_common
> > +SOURCES += objs_crash_generation
> > +SOURCES += objs_handler
> > +SOURCES += objs_sender
> 
> Presumably you could just write:
> SOURCES += objs_common + objs_crash_generation ...

It seems unlikely that objs_common + objs_crash_generation + ... is sorted.

Comment 79

[(not currently active) Ted Mielczarek]

Oh, good point. :-/

Comment 80

[Michael Shal [:mshal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #77)
> Comment on attachment 8363694 [details] [diff] [review]
> 0001-Bug-875013-VPATH-removals-in-build-roboextender-cras.patch
> 
> Review of attachment 8363694 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/crashreporter/breakpad-windows-libxul/moz.build
> @@ +26,5 @@
> > +
> > +SOURCES += objs_common
> > +SOURCES += objs_crash_generation
> > +SOURCES += objs_handler
> > +SOURCES += objs_sender
> 
> Presumably you could just write:
> SOURCES += objs_common + objs_crash_generation ...

Unfortunately that throws a sorting error as Ms2ger guessed.

> 
> ::: toolkit/crashreporter/breakpad-windows-standalone/Makefile.in
> @@ +4,5 @@
> >  
> >  USE_STATIC_LIBS = 1
> >  MOZ_GLUE_LDFLAGS =
> >  
> >  STL_FLAGS =
> 
> We should really move these things to moz.build.

I'm a bit surprised we didn't have bugs already - I've filed bug 964453 and bug 964869 for these.

> 
> :::
> toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/
> objs.mozbuild
> @@ +12,5 @@
> > +]
> > +
> > +subdir = 'toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation'
> > +objs_crash_generation = [
> > +    '%s/%s/%s' % (TOPSRCDIR, subdir, s) for s in lobjs_crash_generation
> 
> We should probably provide a helper in moz.build to do this. Can you file
> that as a followup?

I've filed bug 964302 for this - please chime in there if I misunderstood what you wanted the helper to do.

> 
> ::: toolkit/crashreporter/test/Makefile.in
> @@ +6,3 @@
> >  LOCAL_INCLUDES += \
> >    -I$(srcdir)/../google-breakpad/src/ \
> >    $(NULL)
> 
> You could have moved the LOCAL_INCLUDES while you were here, but no big deal.

Sorry, I'll try to catch it next time :)

Comment 81

[Michael Shal [:mshal]]

build/roboextender/crashreporter inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ccfb70c31b

Comment 82

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b3ccfb70c31b

Comment 83

[Michael Shal [:mshal]]

I think all instances of VPATH have been removed, so I'm closing this bug. The remaining VPATH usage is from third-party projects, like nspr, libffi, etc. In case I missed any, feel free to open another bug to get them resolved.

Comment 84

[Gregory Szorc [:gps]]

\o/