940708 - Build webrtc in unified mode

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[(no longer active)]

WebRTC is one of the most expensive dirs for us to build.  gps/glandium: can you guys help make the gyp makefile generator generate unified files please?  This stuff is probably way over my head!  :-)

Comment 1

[Mike Hommey [:glandium]]

This will be partially addressed by changing how gyp files are processed (I don't have the bug off-hand). That's something that should be coming in the next few weeks.

Comment 2

[Mike Hommey [:glandium]]

The best you can do in the meanwhile, is do it manually.

Comment 3

[Mike Hommey [:glandium]]

But I'm not sure how much change to the gyp files we're ready to take.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

I'm not sure what the question is.  I will note that the gyp files are imported from upstream, and major changes to them would be very painful, as they change substantially with each release.  Most of the changes we're carrying (and much of the merge pain) is in the gyp files currently, and I hope to reduce that by landing a number of our changes in upstream soon.

Comment 5

[(no longer active)]

Hmm, OK.  Let me think about this a bit more and see if I have some super awesome ideas I don't know about yet!  :-)

Comment 6

[Gregory Szorc [:gps]]

Better GYP integration is kinda/sorta tracked by bug 778236.

Comment 7

[(no longer active)]

OK, how does bug 778236 impact things here?  Can you please help me understand what we need to do for this bug?  Thanks!

Comment 8

[Mike Hommey [:glandium]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> OK, how does bug 778236 impact things here?  Can you please help me
> understand what we need to do for this bug?  Thanks!

try this:
diff --git a/python/mozbuild/mozbuild/frontend/gyp_reader.py b/python/mozbuild/mozbuild/frontend/gyp_reader.py
--- a/python/mozbuild/mozbuild/frontend/gyp_reader.py
+++ b/python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ -158,17 +158,17 @@ def read_from_gyp(config, path, output, 
             name = spec['target_name']
             if name.startswith('lib'):
                 name = name[3:]
             # The sandbox expects an unicode string.
             sandbox['LIBRARY_NAME'] = name.decode('utf-8')
             # The sandbox expects alphabetical order when adding sources
             sources = alphabetical_sorted(spec.get('sources', []))
             # gyp files contain headers in sources lists.
-            sandbox['SOURCES'] = \
+            sandbox['UNIFIED_SOURCES'] = \
                 [f for f in sources if mozpath.splitext(f)[-1] != '.h']
 
             for define in target_conf.get('defines', []):
                 if '=' in define:
                     name, value = define.split('=', 1)
                     sandbox['DEFINES'][name] = value
                 else:
                     sandbox['DEFINES'][define] = True


Now, this will use unified sources for *everything* gyp-controlled. This might not work without massive code changes. Or this might just work. In the former case, we'll need a way to express what not to unify.

Comment 9

[(no longer active)]

(In reply to comment #8)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)
> > OK, how does bug 778236 impact things here?  Can you please help me
> > understand what we need to do for this bug?  Thanks!
> 
> try this:
> diff --git a/python/mozbuild/mozbuild/frontend/gyp_reader.py
> b/python/mozbuild/mozbuild/frontend/gyp_reader.py
> --- a/python/mozbuild/mozbuild/frontend/gyp_reader.py
> +++ b/python/mozbuild/mozbuild/frontend/gyp_reader.py
> @@ -158,17 +158,17 @@ def read_from_gyp(config, path, output, 
>              name = spec['target_name']
>              if name.startswith('lib'):
>                  name = name[3:]
>              # The sandbox expects an unicode string.
>              sandbox['LIBRARY_NAME'] = name.decode('utf-8')
>              # The sandbox expects alphabetical order when adding sources
>              sources = alphabetical_sorted(spec.get('sources', []))
>              # gyp files contain headers in sources lists.
> -            sandbox['SOURCES'] = \
> +            sandbox['UNIFIED_SOURCES'] = \
>                  [f for f in sources if mozpath.splitext(f)[-1] != '.h']
> 
>              for define in target_conf.get('defines', []):
>                  if '=' in define:
>                      name, value = define.split('=', 1)
>                      sandbox['DEFINES'][name] = value
>                  else:
>                      sandbox['DEFINES'][define] = True
> 
> Now, this will use unified sources for *everything* gyp-controlled. This might
> not work without massive code changes. Or this might just work. In the former
> case, we'll need a way to express what not to unify.

This is for all gyp projects, right?  If that's true, then we probably don't want to specify the exlusion list here.  Where's a better place for that to live?

Comment 10

[Mike Hommey [:glandium]]

The ideal place would be gyp files themselves. I don't know how much local changes to those we'd allow, though. Otherwise, the moz.build that declares the corresponding GYP_DIRS...

Comment 11

[(no longer active)]

(In reply to comment #10)
> The ideal place would be gyp files themselves. I don't know how much local
> changes to those we'd allow, though.

Last I checked with jesup, he didn't want modifications to those files...

> Otherwise, the moz.build that declares the
> corresponding GYP_DIRS...

That would be media/webrtc/moz.build, right?

Comment 12

[Mike Hommey [:glandium]]

There's also media/mtransport/third_party/moz.build.

Comment 13

[Eric Rescorla (:ekr)]

(In reply to Mike Hommey [:glandium] from comment #12)
> There's also media/mtransport/third_party/moz.build.

The mtransport stuff is exclusively ours, so that should
be safe.

Comment 14

[Eric Rescorla (:ekr)]

(In reply to Eric Rescorla (:ekr) from comment #13)
> (In reply to Mike Hommey [:glandium] from comment #12)
> > There's also media/mtransport/third_party/moz.build.
> 
> The mtransport stuff is exclusively ours, so that should
> be safe.


Just to clarify, I mean the gypfiles. The code isn't.

Comment 15

[Mike Hommey [:glandium]]

(In reply to Eric Rescorla (:ekr) from comment #14)
> Just to clarify, I mean the gypfiles. The code isn't.

If they're ours, why did we write gyp files in the first place? Things that are exclusive to us should use moz.builds

Comment 16

[(no longer active)]

Attachment: Build webrtc in unified mode; r=glandium,jesup

Comment 17

[(no longer active)]

Comment on attachment 8347685 [details] [diff] [review]
Build webrtc in unified mode; r=glandium,jesup

I'll upstream the webrtc.org changes after this patch lands on mozilla-central.

Comment 18

[(no longer active)]

Attachment: Build webrtc in unified mode; r=glandium,jesup

Comment 19

[Eric Rescorla (:ekr)]

(In reply to Mike Hommey [:glandium] from comment #15)
> (In reply to Eric Rescorla (:ekr) from comment #14)
> > Just to clarify, I mean the gypfiles. The code isn't.
> 
> If they're ours, why did we write gyp files in the first place? Things that
> are exclusive to us should use moz.builds

Uh, they were written a while ago, back when we still had Makefile.in,
and gyp seemed convenient. Feel free to rewrite them using moz.build.

Comment 20

[(no longer active)]

Attachment: Build webrtc in unified mode; r=glandium,jesup

Comment 21

[(no longer active)]

Comment on attachment 8347751 [details] [diff] [review]
Build webrtc in unified mode; r=glandium,jesup

webrtc.org upstream patches:

https://webrtc-codereview.appspot.com/5879004
https://webrtc-codereview.appspot.com/5789005

Comment 22

[(no longer active)]

Attachment: Build webrtc in unified mode; r=glandium,jesup

Comment 23

[(no longer active)]

Note that I ended up limiting the unified prefix name in recursivemake.py to 50 characters because that's what turns our build green on try for Windows, but it's hardly worth uploading yet another version of the patch just for that change!

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8347766 [details] [diff] [review]
Build webrtc in unified mode; r=glandium,jesup

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

Please separate the source changes from the build system changes on next iteration.

::: media/webrtc/moz.build
@@ +8,5 @@
>  
>  GYP_DIRS += ['trunk']
>  
> +GYP_NON_UNIFIED_SOURCES = [
> +    './src/sipcc/core/gsm/fim.c',                           # Because of name clash in the logTag variable

It would be much better if those paths were paths relative to the directory where the moz.build is. Because like this, they have no meaning.

Also this variable should be under GYP_FILES, like input, variables and sandbox_vars are.

Comment 25

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #24)
> ::: media/webrtc/moz.build
> @@ +8,5 @@
> >  
> >  GYP_DIRS += ['trunk']
> >  
> > +GYP_NON_UNIFIED_SOURCES = [
> > +    './src/sipcc/core/gsm/fim.c',                           # Because of name clash in the logTag variable
> 
> It would be much better if those paths were paths relative to the directory
> where the moz.build is. Because like this, they have no meaning.

They surely have meaning, these strings are the exact strings that appear in the gyp file, see <http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/signaling.gyp#372> for example.  They are *not* relative path names, they are just copied directly from the gyp file so that we don't need to modify the gyp files.

If you have a suggestion on how to deal with path names instead of strings and make them relative to the moz.build file please tell me what your suggestion exactly is and how to implement it, because I'm not familiar with how path processing works in this code and also not sure how to tell gyp_reader where the moz.builf file is etc since that code seems not tied to a specific moz.build file, but I personally think that would be a change for worse since we would lose the nice property of being able to grep the gyp files for these exact strings which has been a goal of mine, so I'd like jesup to sign off on that as well.

> Also this variable should be under GYP_FILES, like input, variables and
> sandbox_vars are.

OK assuming you mean GYP_DIRS.

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

We can make any mods we like to the gyp files in media/webrtc/signaling (where most of those files are), and in mtransport (or change them to moz.build files, though it might be trickier for signaling perhaps).  It's the files in media/webrtc/trunk that are upstreamed and we need to be careful about changes to.

Needinfoing to ekr (mtransport file changes, as many of the cpp files with mods are upstreamed to a different source) and ehugg (signaling code)

Comment 27

[Ethan Hugg [:ehugg]]

Comment on attachment 8347766 [details] [diff] [review]
Build webrtc in unified mode; r=glandium,jesup

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

::: media/webrtc/moz.build
@@ +8,5 @@
>  
>  GYP_DIRS += ['trunk']
>  
> +GYP_NON_UNIFIED_SOURCES = [
> +    './src/sipcc/core/gsm/fim.c',                           # Because of name clash in the logTag variable

You could probably replace logTag with __FILE__ if that makes this easier.  Signaling has no upstream so feel free to make changes.  However I imagine that fim.c has the same problem as fsm.c below and would be in that group because it has NSPR logging.

Comment 28

[Ethan Hugg [:ehugg]]

I imagine more files in signaling might call to CSFLog.. functions in the future, does that mean that they would need to be added to this list with NSPR problems when they make their first logging call?

Comment 29

[(no longer active)]

(In reply to Ethan Hugg [:ehugg] from comment #27)
> ::: media/webrtc/moz.build
> @@ +8,5 @@
> >  
> >  GYP_DIRS += ['trunk']
> >  
> > +GYP_NON_UNIFIED_SOURCES = [
> > +    './src/sipcc/core/gsm/fim.c',                           # Because of name clash in the logTag variable
> 
> You could probably replace logTag with __FILE__ if that makes this easier. 
> Signaling has no upstream so feel free to make changes.  However I imagine
> that fim.c has the same problem as fsm.c below and would be in that group
> because it has NSPR logging.

I am not planning on making code changes to be able to unify more files beyond the trivial changes in my current patch does.  That work should go into follow-up bugs.  (But yes, I think that fsm.c forces NSPR logging anyways.)

(In reply to Ethan Hugg [:ehugg] from comment #28)
> I imagine more files in signaling might call to CSFLog.. functions in the
> future, does that mean that they would need to be added to this list with
> NSPR problems when they make their first logging call?

That would only be needed for stuff that defines FORCE_PR_LOGGING (to get NSPR logging both in debug and release builds).  If you start defining that macro in more files, you will get build errors that tell you that you need to move those files out of the unified sources.

Comment 30

[(no longer active)]

(In reply to Randell Jesup [:jesup] from comment #26)
> We can make any mods we like to the gyp files in media/webrtc/signaling
> (where most of those files are), and in mtransport (or change them to
> moz.build files, though it might be trickier for signaling perhaps).  It's
> the files in media/webrtc/trunk that are upstreamed and we need to be
> careful about changes to.

Hmm, that's not really what we're debating here.  If we need our solution to treat one gyp file as read-only then we might as well have it treat all gyp files as read-only.

In comment 24 glandium asked me to convert these lists to use actual path names relative to media/webrtc (which is where this moz.build file that contains this list lives.)  In comment 25 I disagreed since these strings are coming directly from the gyp file (down to including the "./" prefix for those path names that begin with ./ in the gyp file).  I think that what I have here now is better since you can just take the full string, grep for it in the gyp files without needing to worry about where the moz.build defining this variable lives.  Since changing what I have here will take a significant amount of time (it took me around a day or maybe more to get this patch to compile on all of our platforms) I don't want to do what glandium suggests if you think that the idea of using what appears in the gyp file verbatim makes sense, since I would expect that you're probably going to be the person caring most about these values.

Comment 31

[Mike Hommey [:glandium]]

Attachment: Build webrtc in unified mode

The commented portion of gyp_reader.py is something that requires the files to be registered in gyp, on top of existing in the source tree. It makes updating the moz.build much more cumbersome, since some files may or may not be included depending on the platform, options, etc. I think it's easier if we leave it out, but tell me if you'd rather have it in.

On the medium term, i think we should just rewrite non-upstream gyp files to moz.build. There's no reason to keep things using a gyp file if they aren't coming from a third party.

Ehsan, please test this patch with the code parts from yours. Note there was a webrtc_vac.c in your patch that doesn't exist, and I guessed it was meant to be webrtc_vad.c. However, if your patch built with webrtc_vac.c, it means webrtc_vad.c can be kept unified.

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #30)

> In comment 24 glandium asked me to convert these lists to use actual path
> names relative to media/webrtc (which is where this moz.build file that
> contains this list lives.)  In comment 25 I disagreed since these strings
> are coming directly from the gyp file (down to including the "./" prefix for
> those path names that begin with ./ in the gyp file).  I think that what I
> have here now is better since you can just take the full string, grep for it
> in the gyp files without needing to worry about where the moz.build defining
> this variable lives.  Since changing what I have here will take a
> significant amount of time (it took me around a day or maybe more to get
> this patch to compile on all of our platforms) I don't want to do what
> glandium suggests if you think that the idea of using what appears in the
> gyp file verbatim makes sense, since I would expect that you're probably
> going to be the person caring most about these values.

I generally don't care about verbatim paths, as the filenames are generally unique, and to find a use of foo.cc in the gyp files I'd simply use "gfind foo.cc", or in makesytem files "mfind foo.cc".  (gfind is roughly "find -name "*.gyp*" | xargs grep $*"; mfind is similar for makefiles/moz.build files)

Comment 33

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #31)
> Created attachment 8349207 [details] [diff] [review]
> Build webrtc in unified mode
> 
> The commented portion of gyp_reader.py is something that requires the files
> to be registered in gyp, on top of existing in the source tree. It makes
> updating the moz.build much more cumbersome, since some files may or may not
> be included depending on the platform, options, etc. I think it's easier if
> we leave it out, but tell me if you'd rather have it in.

I think that's fine.

> On the medium term, i think we should just rewrite non-upstream gyp files to
> moz.build. There's no reason to keep things using a gyp file if they aren't
> coming from a third party.

Sure.

> Ehsan, please test this patch with the code parts from yours. Note there was
> a webrtc_vac.c in your patch that doesn't exist, and I guessed it was meant
> to be webrtc_vad.c. However, if your patch built with webrtc_vac.c, it means
> webrtc_vad.c can be kept unified.

Hmm, yeah I probably misspelled it.

Comment 34

[(no longer active)]

Attachment: Code changes

Comment 35

[(no longer active)]

Attachment: Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

Comment 36

[(no longer active)]

Attachment: Adjust LOCAL_INCLUDES and handle asm sources for unified webrtc builds

Comment 37

[(no longer active)]

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

Comment 38

[Mike Hommey [:glandium]]

Comment on attachment 8349731 [details] [diff] [review]
Adjust LOCAL_INCLUDES and handle asm sources for unified webrtc builds

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

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +168,5 @@
>                  for f in spec.get('sources', [])
> +                if mozpath.splitext(f)[-1] not in ['.h', '.S'])
> +            asm_sources = set(mozpath.normpath(mozpath.join(sandbox['SRCDIR'], f))
> +                for f in spec.get('sources', [])
> +                if mozpath.splitext(f)[-1] == '.S')

It seems simpler to keep sources as it is and derive asm_sources from it:
  asm_sources = set(f for f in sources if mozpath.splitext(f)[-1] == '.S')

Then:
  unified_sources = sources - non_unified_sources - asm_sources
  sources -= unified_sources
  all_sources |= sources

@@ +177,4 @@
>              # The sandbox expects alphabetical order when adding sources
>              sandbox['SOURCES'] = alphabetical_sorted(sources)
>              sandbox['UNIFIED_SOURCES'] = alphabetical_sorted(unified_sources)
> +            sandbox['LOCAL_INCLUDES'] += ['.']

There's already -I. in INCLUDES in config.mk. There should be no need to add another.

Comment 39

[Mike Hommey [:glandium]]

Comment on attachment 8349722 [details] [diff] [review]
Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +609,5 @@
>                                                        files)):
>                  just_the_filenames = list(filter_out_dummy(unified_group))
> +                # On Windows, path names have a maximum length of 255 characters,
> +                # so avoid creating extremely long path names.
> +                yield '%s%d.%s' % (unified_prefix[0:50], i, unified_suffix), just_the_filenames

Arbitrary cut seems a bit harsh, especially considering it cuts the most relevant part. I'd rather see a cut on a "_", and keeping the end instead of the start.

That is, foo_bar_baz_something_very_long_so_long_that_it_breaks_windows would become very_long_so_long_that_it_breaks_windows. (longest substring starting after a "_")

Comment 40

[(no longer active)]

Attachment: Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8349207 [details] [diff] [review]
Build webrtc in unified mode

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

LGTM!

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +71,5 @@
>          return value.encode('utf-8')
>      return value
>  
>  
> +def read_from_gyp(config, path, output, vars, non_unified_sources):

Can we make non_unified_sources a named argument with a default value of empty set?

@@ +199,5 @@
>          time_start = time.time()
> +#    remainder = non_unified_sources - all_sources
> +#    if remainder:
> +#        raise SandboxValidationError('%s defined as non_unified_source, but is '
> +#            'not defined as a source' % list(remainder)[0])

There could be multiple values. So why don't you print:

', '.join(remainder)

::: python/mozbuild/mozbuild/frontend/reader.py
@@ +767,5 @@
>              # We could emit the parent sandbox before processing gyp
>              # configuration, but we need to add the gyp objdirs to that sandbox
>              # first.
>              from .gyp_reader import read_from_gyp
> +            non_unified_sources = []

I'd make it a set.

@@ +772,5 @@
> +            for s in gyp_dir.non_unified_sources:
> +                source = mozpath.normpath(mozpath.join(curdir, s))
> +                if not os.path.exists(source):
> +                    raise SandboxValidationError('Cannot find %s referenced '
> +                        'from %s' % (source, path))

Thank you for validating this!

Comment 42

[(no longer active)]

Attachment: Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 8349771 [details] [diff] [review]
Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +613,5 @@
> +            # so avoid creating extremely long path names.
> +            unified_prefix = unified_prefix[-50:]
> +            first_underscore = unified_prefix.find("_")
> +            if first_underscore >= 0:
> +                unified_prefix = unified_prefix[first_underscore + 1:]

That always removes up to the first underscore. In practice that means it removes "Unified_" for all the files.
On further thought, it would probably be better to move at the caller site, too (there's really only one caller where this matters), at which point you can do the munging on the path before the slashes are replaced with underscores.

Comment 44

[Mike Hommey [:glandium]]

Comment on attachment 8349775 [details] [diff] [review]
Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

Same comment.

Comment 45

[(no longer active)]

Attachment: Handle asm sources for unified webrtc builds

Comment 46

[(no longer active)]

Attachment: Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

Comment 47

[(no longer active)]

Attachment: Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

Comment 48

[(no longer active)]

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

Comment 49

[(no longer active)]

Attachment: Handle asm sources for unified webrtc builds

This fixes the .S issue on try causing ARM builds to fail.

Comment 50

[(no longer active)]

Attachment: Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

This fixes a cosmetic issue which dropped the first foo_ from the name of every unified file name even if it was not too long.

Comment 51

[(no longer active)]

These patches should be landable as far as I'm concerned at this point:

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

Comment 52

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8349721 [details] [diff] [review]
Code changes

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

r+ for the trunk/etc changes
r? to abr for the nicer/etc changes (since that's an upstreamed module)

Comment 53

[Eric Rescorla (:ekr)]

Comment on attachment 8349721 [details] [diff] [review]
Code changes

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

Please don't just make unconditional changes like this to upstream code.

If it's important to remove these, please wrap them in #ifdef NO_RCSSTRING
or the like. Note that this can't be a MOZ_ thingy because it's not Mozilla
code.

Comment 54

[(no longer active)]

(In reply to Eric Rescorla (:ekr) from comment #53)
> Comment on attachment 8349721 [details] [diff] [review]
> Code changes
> 
> Review of attachment 8349721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please don't just make unconditional changes like this to upstream code.
> 
> If it's important to remove these, please wrap them in #ifdef NO_RCSSTRING
> or the like. Note that this can't be a MOZ_ thingy because it's not Mozilla
> code.

I'll exclude them from the unified build as well.  Doing the #ifdef seems like a useless dance to me.

Comment 55

[(no longer active)]

Landed the code changes: https://hg.mozilla.org/integration/mozilla-inbound/rev/050d3c674e82

Comment 56

[(no longer active)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #21)
> Comment on attachment 8347751 [details] [diff] [review]
> Build webrtc in unified mode; r=glandium,jesup
> 
> webrtc.org upstream patches:
> 
> https://webrtc-codereview.appspot.com/5879004
> https://webrtc-codereview.appspot.com/5789005

Filed http://code.google.com/p/webrtc/issues/detail?id=2744

Comment 57

[(no longer active)]

Attachment: Exclude more files from unified builds because of name clashes on RCSSTRING

This works around the stuff that ekr r-minused.

Let's see if we can get this landed before the holidays!

Comment 58

[Gregory Szorc [:gps]]

Comment on attachment 8349809 [details] [diff] [review]
Handle asm sources for unified webrtc builds

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

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +167,4 @@
>              sources = set(mozpath.normpath(mozpath.join(sandbox['SRCDIR'], f))
>                  for f in spec.get('sources', [])
>                  if mozpath.splitext(f)[-1] != '.h')
> +            asm_sources = set(f for f in sources if mozpath.splitext(f)[-1] == '.S')

You could probably just do:

asm_sources = set(f for f in sources if f.endswith('.S'))

Comment 59

[Gregory Szorc [:gps]]

Comment on attachment 8349812 [details] [diff] [review]
Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

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

Glandium had comments about previous versions of this patch. I'm going to let him see this to completion.

Comment 60

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

https://hg.mozilla.org/mozilla-central/rev/050d3c674e82

Comment 61

[Mike Hommey [:glandium]]

Comment on attachment 8349812 [details] [diff] [review]
Limit the length of the unified file name prefix to 50 characters so that we don't blow up the Windows path name limit

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

::: python/mozbuild/mozbuild/backend/recursivemake.py
@@ +379,5 @@
> +                        if len(unified_prefix) > 39:
> +                            unified_prefix = unified_prefix[-39:]
> +                            first_underscore = unified_prefix.find("_")
> +                            if first_underscore >= 0:
> +                                unified_prefix = unified_prefix[first_underscore + 1:]

Note this would cut in the middle of a directory name if it contains an underscore. How about something like this:

unified_prefix = backend_file.relobjdir
if len(unified_prefix) > 39:
    unified_prefix = unified_prefix[-39:].split('/', 1)[-1]
unified_prefix = unified_prefix.replace('/', '_')

Comment 62

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d4aec392a165
https://hg.mozilla.org/integration/mozilla-inbound/rev/aedcac877ab0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1da29cee34
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b081bf3d16c

Note that I kept the checking code in gyp_reader commented.  We can uncomment it and fix up the moz.build to handle all of the moz.build mess that it will introduce in another bug if you want.

Comment 63

[(no longer active)]

Backed out because of ASAN bustage:

http://hg.mozilla.org/integration/mozilla-inbound/rev/2b7739306a76

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=8b081bf3d16c

Comment 64

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97516134f58b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca97d06ddfe8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d343497f7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4f915b9fb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/d02fac060b57

Comment 65

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

https://hg.mozilla.org/mozilla-central/rev/97516134f58b
https://hg.mozilla.org/mozilla-central/rev/ca97d06ddfe8
https://hg.mozilla.org/mozilla-central/rev/a9d343497f7d
https://hg.mozilla.org/mozilla-central/rev/4c4f915b9fb4
https://hg.mozilla.org/mozilla-central/rev/d02fac060b57

Comment 66

[Philip Chee]

https://tbpl.mozilla.org/php/getParsedLog.php?id=32327042&tree=Thunderbird-Trunk#error0

LINK : fatal error LNK1104: cannot open file '..\..\media\webrtc\trunk\webrtc\modules\remote_bitrate_estimator\remote_bitrate_estimator_components_rbe_components\Unified_cpp_ate_estimator_components_rbe_components0.obj'

Comment 67

[Mark Banner (:standard8)]

At a guess the Thunderbird issues are due to path length. Do we really need the duplicated text in those file locations?

Comment 68

[Mark Banner (:standard8)]

I've split out the issue in comment 66/67 to bug 955699