Closed
Bug 875013
Opened 11 years ago
Closed 11 years ago
Eliminate VPATH usage
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla29
People
(Reporter: gps, Assigned: mshal)
References
Details
Attachments
(10 files, 3 obsolete files)
(deleted),
patch
|
gps
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joey
:
review+
joey
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
joey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gps
:
review+
derf
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
Ted, Joey, Mike, and I just decided we all don't like VPATH and should eliminate it from the build system.
Reporter | ||
Comment 1•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Attachment #767934 -
Flags: review?(gps)
Assignee | ||
Comment 4•11 years ago
|
||
Adding [leave open] since this will be done in parts - it's a lot to do all at once.
Whiteboard: [leave open]
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mshal
Reporter | ||
Comment 5•11 years ago
|
||
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.
Attachment #767934 -
Flags: review?(gps) → review-
Assignee | ||
Comment 6•11 years ago
|
||
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.
Attachment #768590 -
Flags: review?(joey)
Comment 7•11 years ago
|
||
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 ?
Attachment #768590 -
Flags: review?(joey) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Current try results are here:
https://tbpl.mozilla.org/?tree=Try&rev=c70693a25a32
I'll update the patch to use %s/%s.
Assignee | ||
Comment 9•11 years ago
|
||
Updated to use %s/%s. Carrying forward r=joey.
Latest try results: https://tbpl.mozilla.org/?tree=Try&rev=496a359945a8
Attachment #768590 -
Attachment is obsolete: true
Attachment #769128 -
Flags: review+
Assignee | ||
Comment 10•11 years ago
|
||
Comment 11•11 years ago
|
||
Comment 12•11 years ago
|
||
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
Updated•11 years ago
|
Comment 13•11 years ago
|
||
Completely remove VPATH usage in dom/mobilemessage.
Comment 14•11 years ago
|
||
Comment on attachment 802263 [details] [diff] [review]
Eliminate VPATH usage (mobilemessage)
https://tbpl.mozilla.org/?tree=Try&rev=acfd316e63cd
Attachment #802263 -
Attachment description: Eliminate VPATH usage (mobilemessage) : WIP → Eliminate VPATH usage (mobilemessage)
Attachment #802263 -
Flags: review?(joey)
Comment 15•11 years ago
|
||
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"
]
Attachment #802263 -
Flags: review?(joey) → review-
Comment 16•11 years ago
|
||
(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':
Reporter | ||
Comment 17•11 years ago
|
||
(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.
Assignee | ||
Comment 18•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
Note the mozilla/foo/bar type of include paths reflect C++ namespaces, not directories.
Comment 21•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
Sounds like there are other opinions about using LOCAL_INCLUDES as a replacement, ni for further work to do.
Flags: needinfo?(joey)
Comment 24•11 years ago
|
||
(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•11 years ago
|
||
(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.
Updated•11 years ago
|
Attachment #802263 -
Flags: review- → review?(mshal)
Comment 26•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
(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.
Flags: needinfo?(joey)
Comment 29•11 years ago
|
||
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.
Attachment #802263 -
Flags: review?(mshal) → review?(joey)
Comment 30•11 years ago
|
||
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
Attachment #802263 -
Flags: review?(joey) → review+
Comment 31•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
Comment 34•11 years ago
|
||
(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•11 years ago
|
||
(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.
Reporter | ||
Comment 36•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 38•11 years ago
|
||
Try results: https://tbpl.mozilla.org/?tree=Try&rev=36dd893d6100
Not sure what is up with the "Gu" thing in Linux x64 Opt.
Attachment #807239 -
Flags: review?(joey)
Updated•11 years ago
|
Attachment #807239 -
Flags: review?(joey) → feedback?(joey)
Comment 39•11 years ago
|
||
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
Attachment #807239 -
Flags: feedback?(joey) → feedback+
Assignee | ||
Comment 40•11 years ago
|
||
(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.
Flags: needinfo?(joey)
Updated•11 years ago
|
Attachment #807239 -
Flags: review?(joey)
Comment 41•11 years ago
|
||
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.
Attachment #807239 -
Flags: review?(joey) → review+
Updated•11 years ago
|
Flags: needinfo?(joey)
Assignee | ||
Comment 42•11 years ago
|
||
hal patch inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/28316508a37f
Assignee | ||
Comment 44•11 years ago
|
||
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)
Attachment #819170 -
Flags: review?(joey)
Comment 45•11 years ago
|
||
Feel free to put new LOCAL_INCLUDES in moz.build ;)
Comment 46•11 years ago
|
||
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.
Attachment #819170 -
Flags: review?(joey) → review+
Assignee | ||
Comment 47•11 years ago
|
||
(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•11 years ago
|
||
(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
Assignee | ||
Comment 49•11 years ago
|
||
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
Attachment #8339694 -
Flags: review?(mh+mozilla)
Comment 50•11 years ago
|
||
Comment on attachment 8339694 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH.patch
Review of attachment 8339694 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
Attachment #8339694 -
Flags: review?(mh+mozilla) → review+
Comment 51•11 years ago
|
||
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.
Assignee | ||
Comment 52•11 years ago
|
||
(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 :)
Assignee | ||
Comment 53•11 years ago
|
||
Removed OSDIR & rebased. r+ carried forward
Attachment #8339694 -
Attachment is obsolete: true
Attachment #8342425 -
Flags: review+
Assignee | ||
Comment 54•11 years ago
|
||
Comment 55•11 years ago
|
||
Assignee | ||
Comment 56•11 years ago
|
||
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
Attachment #8356844 -
Flags: review?(joey)
Comment 57•11 years ago
|
||
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.
Attachment #8356844 -
Flags: review?(joey) → review+
Assignee | ||
Comment 58•11 years ago
|
||
Assignee | ||
Comment 59•11 years ago
|
||
Attachment #8358006 -
Flags: review?(joey)
Assignee | ||
Comment 60•11 years ago
|
||
This generates the sources.mozbuild fragment from update.sh and kills the rest of media/libvpx/Makefile.in
Attachment #8358009 -
Flags: review?(tterribe)
Attachment #8358009 -
Flags: review?(gps)
Comment 62•11 years ago
|
||
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.
Attachment #8358006 -
Flags: review?(joey) → review+
Reporter | ||
Comment 63•11 years ago
|
||
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 :)
Attachment #8358009 -
Flags: review?(gps) → feedback+
Assignee | ||
Comment 64•11 years ago
|
||
inbound for media/libvpx: https://hg.mozilla.org/integration/mozilla-inbound/rev/92d510a09a8c
Assignee | ||
Comment 65•11 years ago
|
||
(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?
Assignee | ||
Comment 66•11 years ago
|
||
Comment on attachment 8358009 [details] [diff] [review]
0002-Bug-875013-Remove-VPATH-in-media-libopus.patch
New patch on the way.
Attachment #8358009 -
Flags: review?(tterribe)
Comment 67•11 years ago
|
||
Assignee | ||
Comment 68•11 years ago
|
||
Here's the python approach - just requesting review from b:c for now.
Attachment #8358009 -
Attachment is obsolete: true
Attachment #8358706 -
Flags: review?(gps)
Reporter | ||
Comment 69•11 years ago
|
||
Comment on attachment 8358706 [details] [diff] [review]
0001-Bug-875013-Remove-VPATH-in-media-libopus.patch
Review of attachment 8358706 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM!
Attachment #8358706 -
Flags: review?(gps) → review+
Assignee | ||
Comment 70•11 years ago
|
||
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.
Attachment #8358706 -
Flags: review?(tterribe)
Comment 71•11 years ago
|
||
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.
Attachment #8358706 -
Flags: review?(tterribe) → review+
Assignee | ||
Comment 72•11 years ago
|
||
(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•11 years ago
|
||
(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?
Assignee | ||
Comment 74•11 years ago
|
||
(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•11 years ago
|
||
Assignee | ||
Comment 76•11 years ago
|
||
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)
Attachment #8363694 -
Flags: review?(ted)
Comment 77•11 years ago
|
||
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.
Attachment #8363694 -
Flags: review?(ted) → review+
Comment 78•11 years ago
|
||
(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•11 years ago
|
||
Oh, good point. :-/
Assignee | ||
Comment 80•11 years ago
|
||
(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 :)
Assignee | ||
Comment 81•11 years ago
|
||
build/roboextender/crashreporter inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ccfb70c31b
Comment 82•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Assignee | ||
Comment 83•11 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Target Milestone: --- → mozilla29
Reporter | ||
Comment 84•11 years ago
|
||
\o/
Updated•7 years ago
|
Product: Core → Firefox Build System
Assignee | ||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•