it's a problem having to mirror changes to a copy of webrtc.org just to run a testm.

Bumping priority as we plan to look at this sometime in the first half of 2017.

Looking at this in Q1.

:glandium, I'm building these tests as a standalone program that links against the webrtc static library we already build. I have things working on linux, but on OS X, I keep getting link errors for these symbols: 12:38:14 INFO - "_calloc_impl", referenced from: 12:38:14 INFO - _moz_xcalloc in Unified_cpp_memory_mozalloc0.o 12:38:14 INFO - "_malloc_impl", referenced from: 12:38:14 INFO - _moz_xmalloc in Unified_cpp_memory_mozalloc0.o 12:38:14 INFO - "_posix_memalign_impl", referenced from: 12:38:14 INFO - _moz_xposix_memalign in Unified_cpp_memory_mozalloc0.o 12:38:14 INFO - _moz_posix_memalign in Unified_cpp_memory_mozalloc0.o 12:38:14 INFO - "_realloc_impl", referenced from: 12:38:14 INFO - _moz_xrealloc in Unified_cpp_memory_mozalloc0.o 12:38:14 INFO - "_valloc_impl", referenced from: 12:38:14 INFO - _moz_xvalloc in Unified_cpp_memory_mozalloc0.o On Linux I have to link against 'memory_mozalloc' or I'll get segfaults while running the tests. Here's an example of a recent try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4acebb0cbc36257041d51d62d907d8afe8560a5b which exhibits the problem. Sorry the try run is a bit of a mess, I've been working on the windows compile concurrently with trying to figure out the OS X link problem. I was wondering if you had any recommendations as for what definitions and/or libraries I need to get things working here. Thanks!

You want a dependency on mozglue, not memory_mozalloc. But really, what you want is to link your stuff in libxul-gtest. I don't know why you're doing that cherry-picking in media/webrtc/trunk/gtest/moz.build.

(In reply to Mike Hommey [:glandium] from comment #4) > You want a dependency on mozglue, not memory_mozalloc. But really, what you > want is to link your stuff in libxul-gtest. I don't know why you're doing > that cherry-picking in media/webrtc/trunk/gtest/moz.build. Thanks! Unfortunately the webrtc.org tests are on a different enough version of gtest from what we have in tree that they won't compile as written.

Latest try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d281a104d3508bfda944fcd38b7cf59286fe705c I've only actually been able to run the tests under Linux. I was hoping to use loaners to verify the tests, but of course the taskcluster OS X build is a cross-compile under Linux, and the Windows one-click loaner was disabled when I tried it.

Comment on attachment 8835468 [details] Bug 964133 - Build webrtc.org unit tests.mielczarik https://reviewboard.mozilla.org/r/110778/#review112436 We'll need a build peer for this as well. Note that a lot of the "needs external file" tests might be fine if we imported them in our import procedure; we should look at that for the next import. Note also that some are large, though, and only webrtc people (and automated tests) care.

Comment on attachment 8835462 [details] Bug 964133 - move nICEr and nrappkit to libxul; .mielczarek https://reviewboard.mozilla.org/r/110766/#review112438 Build-peer review

Comment on attachment 8835467 [details] Bug 964133 - Add stub implementation of OSXRunLoopSingleton.cpp; https://reviewboard.mozilla.org/r/110776/#review112440

Comment on attachment 8835466 [details] Bug 964133 - Fix clang static analysis errors; https://reviewboard.mozilla.org/r/110774/#review112444 I'll need to be convinced we don't have another solution for this that doesn't require mfbt includes in webrtc.org code ::: media/webrtc/trunk/third_party/gflags/src/gflags.cc:119 (Diff revision 1) > > #ifndef PATH_SEPARATOR > #define PATH_SEPARATOR '/' > #endif > > +#include <mfbt/Sprintf.h> Shouldn't that be quotes? Also, when we can I prefer to avoid including mozilla-specific files and functions in webrtc.org code; since that generally can't be merged upstream (or is ugly at best, and no one but us can know if it breaks)

Comment on attachment 8835464 [details] Bug 964133 - Fixup FakeIPC to build on windows; https://reviewboard.mozilla.org/r/110770/#review112448

Comment on attachment 8835465 [details] Bug 964133 - Don't build duplicate snprintf in port.cc; https://reviewboard.mozilla.org/r/110772/#review112450 Is there a better way which could be upstreamed? I presume this is because webrtc.org doesn't require MSVC 2013 (or is it 2015?), and so they may want to provide a safe snprintf.

Comment on attachment 8835461 [details] Bug 964133 - Update webrtc gtest and add gflags and gmock to match webrtc.org branch 49; https://reviewboard.mozilla.org/r/110764/#review112452 build-peer review Do we need to duplicate all of gmock another time in the tree here? are there any tricks that can (if need be) build two copies/versions of it?

Comment on attachment 8835463 [details] Bug 964133 - Move FakeIPC to webrtc.org gtest; https://reviewboard.mozilla.org/r/110768/#review112454

Comment on attachment 8835468 [details] Bug 964133 - Build webrtc.org unit tests.mielczarik https://reviewboard.mozilla.org/r/110778/#review112624 Sorry but no, this is unmaintainable. And multiplies the amount of stuff being built. ::: media/webrtc/moz.build:58 (Diff revision 1) > +if CONFIG['ENABLE_TESTS']: > + DIRS += [ Use TEST_DIRS

Comment on attachment 8835468 [details] Bug 964133 - Build webrtc.org unit tests.mielczarik https://reviewboard.mozilla.org/r/110778/#review112624 I don't see how this is any less maintainable than the rest of the webrtc.org code we currently import from upstream, unless you are objecting to my use of moz.build for this. I didn't see any point in hacking on gyp files to get this working when we know that gyp has already been removed from upstream. Once we have gn support in the build system, we can stop using moz.build here and start using gn. If you insist, I can use gyp in the meantime, but to be honest it is at least a big a problem to maintain at this point. I know what you mean about multiplying the amount of stuff being built. It didn't seem to have a huge impact on times for my try builds from what I could tell, but since the audience for these tests are limited and we don't intend to run them in automation perhaps we could add a config flag so people can opt-in to building them?

Or semi-equivalently, build the tests only when someone tries to run the mach command.

On IRC, glandium indicated "it seems to me this should all go in xul-gtest, and we should use the same gtest for everything", and that comment 5 appears to reference this, but "doesn't tell why we can't use the same version for everything"

Comment on attachment 8835462 [details] Bug 964133 - move nICEr and nrappkit to libxul; .mielczarek https://reviewboard.mozilla.org/r/110766/#review112982 I think glandium will make a better reviewer for this change.

Comment on attachment 8835469 [details] Bug 964133 - Add mach command for webrtc.org unit tests; https://reviewboard.mozilla.org/r/110780/#review112988 Skimming this it looks ok, but cancelling review for now as it looks like this patch may need to change based on the ongoing discussion.

(In reply to Randell Jesup [:jesup] from comment #27) > On IRC, glandium indicated "it seems to me this should all go in xul-gtest, > and we should use the same gtest for everything", and that comment 5 appears > to reference this, but "doesn't tell why we can't use the same version for > everything" The webrtc.org gtests use gtest 1.7.0. The current version in tree is 1.6.0. The webrtc.org gtests use features from 1.7.0 and so won't compile against the in tree version. If we want to use the same version for everything, we'd have to update the in tree version to 1.7.0. This is the latest version of gtest, so it would definitely make sense to do this at some point. I do have a few concerns about compiling everything into one gtest: - Everyone will have to run these tests, which are really only relevant to people making changes to code we've taken from webrtc.org. We would end up running them in automation by default. The current set of tests run in around 30 seconds on my system, so I don't think this is a big deal at the moment. - I've left out a chunk of tests that require external resources. These files take up about 1GB on my system. I think we will want to to run these tests eventually, which would definitely have an impact on both people running the tests locally and our automation. These tests also take longer to run (closer to 9 minutes on my system for a full run from upstream webrtc.org). - The version of gtest we use would be tied to using the version used by webrtc.org. If they move to a newer version, updating the in tree version of gtest would become a prerequisite for updating the webrtc.org code. If they stop updating their version, we would be stuck on whatever version they are using. I see two advantages to using the same version of gtest for everything: - saved repository space from not having two versions of gtest and gmock in tree - saved compile time from not have to build two versions of the static libraries Those are real advantages, but it's not clear to me that they outweigh the disadvantages I mentioned above.

glandium: comment 30

(In reply to Dan Minor [:dminor] from comment #30) > (In reply to Randell Jesup [:jesup] from comment #27) > > On IRC, glandium indicated "it seems to me this should all go in xul-gtest, > > and we should use the same gtest for everything", and that comment 5 appears > > to reference this, but "doesn't tell why we can't use the same version for > > everything" > > The webrtc.org gtests use gtest 1.7.0. The current version in tree is 1.6.0. > The webrtc.org gtests use features from 1.7.0 and so won't compile against > the in tree version. > > If we want to use the same version for everything, we'd have to update the > in tree version to 1.7.0. This is the latest version of gtest, so it would > definitely make sense to do this at some point. > > I do have a few concerns about compiling everything into one gtest: > - Everyone will have to run these tests, which are really only relevant to > people making changes to code we've taken from webrtc.org. We would end up > running them in automation by default. The current set of tests run in > around 30 seconds on my system, so I don't think this is a big deal at the > moment. Why is that a disadvantage? The same can be said of any other test. Plus, gtest allows to filter the tests you want to run. > - I've left out a chunk of tests that require external resources. These > files take up about 1GB on my system. I think we will want to to run these > tests eventually, which would definitely have an impact on both people > running the tests locally and our automation. These tests also take longer > to run (closer to 9 minutes on my system for a full run from upstream > webrtc.org). I'm sure we can find a way to have tests disabled by default, but runnable with a filter. > - The version of gtest we use would be tied to using the version used by > webrtc.org. If they move to a newer version, updating the in tree version of > gtest would become a prerequisite for updating the webrtc.org code. If they > stop updating their version, we would be stuck on whatever version they are > using. Are each new version of gtest so incompatible with the previous ones?

(In reply to Mike Hommey [:glandium] from comment #32) > (In reply to Dan Minor [:dminor] from comment #30) > > (In reply to Randell Jesup [:jesup] from comment #27) > > > On IRC, glandium indicated "it seems to me this should all go in xul-gtest, > > > and we should use the same gtest for everything", and that comment 5 appears > > > to reference this, but "doesn't tell why we can't use the same version for > > > everything" > > > > The webrtc.org gtests use gtest 1.7.0. The current version in tree is 1.6.0. > > The webrtc.org gtests use features from 1.7.0 and so won't compile against > > the in tree version. > > > > If we want to use the same version for everything, we'd have to update the > > in tree version to 1.7.0. This is the latest version of gtest, so it would > > definitely make sense to do this at some point. > > > > I do have a few concerns about compiling everything into one gtest: > > - Everyone will have to run these tests, which are really only relevant to > > people making changes to code we've taken from webrtc.org. We would end up > > running them in automation by default. The current set of tests run in > > around 30 seconds on my system, so I don't think this is a big deal at the > > moment. > > Why is that a disadvantage? The same can be said of any other test. Plus, > gtest allows to filter the tests you want to run. > I think the difference is that the only dependency these tests have on the rest of our code is on the memory allocator. For other tests, it's much more likely that changes elsewhere in tree could cause them to fail. > > - I've left out a chunk of tests that require external resources. These > > files take up about 1GB on my system. I think we will want to to run these > > tests eventually, which would definitely have an impact on both people > > running the tests locally and our automation. These tests also take longer > > to run (closer to 9 minutes on my system for a full run from upstream > > webrtc.org). > > I'm sure we can find a way to have tests disabled by default, but runnable > with a filter. > Sure, but at the cost of added complexity. > > - The version of gtest we use would be tied to using the version used by > > webrtc.org. If they move to a newer version, updating the in tree version of > > gtest would become a prerequisite for updating the webrtc.org code. If they > > stop updating their version, we would be stuck on whatever version they are > > using. > > Are each new version of gtest so incompatible with the previous ones? That is a risk we would be taking. I don't see the advantage of building everything into one library, and then having to add a bunch of special cases for the webrtc.org tests. This adds complexity and risk just to save the cost of building a second copy of gtest and gmock. Your original objection was that this would be "unmaintainable" and "multiplies the number of stuff being built". Please explain how having everything built in xul-gtest addresses these concerns.

I'm willing to do what glandium is suggesting if that is what it takes to get this moving again, but I still have reservations about compiling everything into one gtest. A second opinion would help :) Ted, are you willing to weigh in here?

Maybe an acceptable compromise would be to update the version of gtest in tree and have these tests link against that, but continue to build the tests as a separate executable? That would get rid of the duplicated gtest/gmock and keep it simple to run these tests separately from the main gtest job. If for some reason we need to support different versions of gtest/gmock in tree in the future, we could add those back in under media/webrtc/trunk and switch the webrtc.org gtest executable to link against them instead.

(In reply to Dan Minor [:dminor] from comment #34) > I'm willing to do what glandium is suggesting if that is what it takes to > get this moving again, but I still have reservations about compiling > everything into one gtest. A second opinion would help :) Ted, are you > willing to weigh in here? I'm sort of ambivalent here--I can understand glandium's argument, in that xul-gtest is an existing framework and running these tests there would make sense. However, they are standalone tests from an upstream source, so there's no real *need* to link them into xul-gtest, since they don't rely on anything else from gecko. If you can get them linking against mozalloc and working I think that's fine. Separate from that I think getting the other in-tree copy of gtest updated to the latest upstream and using a single copy of gtest everywhere would be a good and sensible thing to do. I don't think concerns about making people run extra tests are particularly warranted--we deal with this in all harnesses, it's a fact of life. I do think tests that require external resources need special care, and probably can't run by default.

I'm going to go ahead and update the version of gtest in tree so we can use that for these tests. I'm still planning to build these as a separate executable rather than linking into xul-gtest as I think this will be easier to maintain.

It would be easier to maintain if it used the data from gyp... which, in fact, creates different executables for different tests, not one big blob for them all...

(In reply to Mike Hommey [:glandium] from comment #38) > It would be easier to maintain if it used the data from gyp... which, in > fact, creates different executables for different tests, not one big blob > for them all... gyp support has already been removed from upstream so I don't think using it here has big advantages for maintainability. We would still be stuck going through and disabling tests for the parts of webrtc.org we don't currently build/use. My intention was to use moz.build as a temporary measure while we wait for gn support in the build system.

Updated try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e102a5b56c0f84154cfa10d7398002804cca96f1

Comment on attachment 8835469 [details] Bug 964133 - Add mach command for webrtc.org unit tests; https://reviewboard.mozilla.org/r/110780/#review123584 ::: python/mozbuild/mozbuild/mach_commands.py:1603 (Diff revision 2) > + cwd = os.path.join(self.topsrcdir, 'media', 'webrtc', 'trunk') > + > + if not os.path.isdir(cwd): > + os.makedirs(cwd) This doesn't seem right... we're going to create a srcdir path with a mach command if it doesn't exist? The other gtest mach command this is borrowing from is doing this wih an objdir path, which is also odd because some tests seem unlikely to pass if that path doesn't already exist. Can we just assume bail out if `media/webrts/trunk` ever goes away?

Comment on attachment 8835469 [details] Bug 964133 - Add mach command for webrtc.org unit tests; https://reviewboard.mozilla.org/r/110780/#review124134 ::: python/mozbuild/mozbuild/mach_commands.py:1606 (Diff revisions 2 - 3) > - os.makedirs(cwd) > + print('Unable to find working directory for tests: %s' % cwd) > + return False The convention in mach commands seems to be to return 1 or raise an exception.

Comment on attachment 8835462 [details] Bug 964133 - move nICEr and nrappkit to libxul; .mielczarek https://reviewboard.mozilla.org/r/110766/#review125950 nit: your commit message says 'webkit' the first time. :)

Comment on attachment 8835468 [details] Bug 964133 - Build webrtc.org unit tests.mielczarik https://reviewboard.mozilla.org/r/110778/#review125952 I think we already have a bug on supporting gn, if so can you mention it in a comment in the moz.build file? It would be great to get that fixed and not have to maintain this at some point. ::: media/webrtc/trunk/gtest/moz.build:117 (Diff revision 3) > + 'strmiids', > + "wmcodecdspuuid", > + ] > + > + SOURCES += [ > + # '../webrtc/base/win32regkey_unittest.cc', Are these lists hand-maintained or do you have a script or something? If they're hand-maintained I would say you should just leave out the commmented-out sources. ::: testing/gtest/moz.build:28 (Diff revision 3) > 'mozilla/MozGTestBench.h', > ] > + EXPORTS.gtest += gtest_exports > + > + # webrtc.org unit tests use this include path > + EXPORTS.testing.gtest.include.gtest += gtest_exports Hm, this kinda sucks. We're *so close* to being able to just do `LOCAL_INCLUDES += ['/']` in the webrtc moz.build and have it work, but our path is `testing/gtest/gtest/include/gtest` instead of `testing/gtest/include/gtest`. Is this something we can fix in the future, or are we stuck with it? If there isn't a fix in sight I could be convinced that the right call here is to `hg mv` `testing/gtest/gtest/*` up one level, and similarly move `testing/gtest/gmock` to `testing/gmock` so that topsrcdir-relative include paths would work.

Comment on attachment 8835468 [details] Bug 964133 - Build webrtc.org unit tests.mielczarik https://reviewboard.mozilla.org/r/110778/#review125952 > Hm, this kinda sucks. We're *so close* to being able to just do `LOCAL_INCLUDES += ['/']` in the webrtc moz.build and have it work, but our path is `testing/gtest/gtest/include/gtest` instead of `testing/gtest/include/gtest`. > > Is this something we can fix in the future, or are we stuck with it? If there isn't a fix in sight I could be convinced that the right call here is to `hg mv` `testing/gtest/gtest/*` up one level, and similarly move `testing/gtest/gmock` to `testing/gmock` so that topsrcdir-relative include paths would work. Their path has already changed upstream to "webrtc/test/gtest". I think the best thing would be to get upstream to use a include path that works for both of us, but that would be a bit of an undertaking.

Comment on attachment 8835468 [details] Bug 964133 - Build webrtc.org unit tests.mielczarik https://reviewboard.mozilla.org/r/110778/#review125952 > Are these lists hand-maintained or do you have a script or something? If they're hand-maintained I would say you should just leave out the commmented-out sources. I've dropped the stuff that we just don't build, kept the items that failed or required external resources as those are things I plan to follow up on short term.

Comment on attachment 8847088 [details] Bug 964133 - Remove webrtc.org copy of gtest; .mielczarek https://reviewboard.mozilla.org/r/120142/#review125954 ::: media/webrtc/moz.build:113 (Diff revision 2) > - GYP_DIRS['trunk/testing'].sandbox_vars['ALLOW_COMPILER_WARNINGS'] = True > - GYP_DIRS['trunk/testing'].non_unified_sources += webrtc_non_unified_sources > - > -if CONFIG['ENABLE_TESTS']: > + if CONFIG['ENABLE_TESTS']: > - DIRS += ['signaling/fuzztest'] > + TEST_DIRS += ['signaling/fuzztest'] > + TEST_DIRS += ['signaling/gtest'] Just merge this with the assignment above it.

Comment on attachment 8847089 [details] Bug 964133 - Import gflags from webrtc.org branch 49; .mielczarik https://reviewboard.mozilla.org/r/120144/#review126886

Comment on attachment 8847090 [details] Bug 964133 - Build gflags; .mielczarik https://reviewboard.mozilla.org/r/120146/#review126888 ::: media/webrtc/trunk/third_party/gflags/src/mutex.h:174 (Diff revision 2) > inline Mutex(); > // This constructor should be used for global, static Mutex objects. > // It inhibits work being done by the destructor, which makes it > // safer for code that tries to acqiure this mutex in their global > // destructor. > - inline Mutex(LinkerInitialized); > + explicit inline Mutex(LinkerInitialized); Are you upstreaming this change? ::: media/webrtc/trunk/third_party/gflags/src/windows/port.cc:53 (Diff revision 2) > if (size == 0) // not even room for a \0? > return -1; // not what C99 says to do, but what windows does > str[size-1] = '\0'; > return _vsnprintf(str, size-1, format, ap); > } > - > +/* Similarly, are you going to upstream some form of this change? Microsoft added snprintf in VC2015, which is our minimum required version, so this could be wrapped in: ``` #if _MSC_VER > 1400 // ... #endif ``` (assuming the Google folks even care about supporting older versions of MSVC, I know Chromium does not.)

Comment on attachment 8847090 [details] Bug 964133 - Build gflags; .mielczarik https://reviewboard.mozilla.org/r/120146/#review126888 > Similarly, are you going to upstream some form of this change? Microsoft added snprintf in VC2015, which is our minimum required version, so this could be wrapped in: > ``` > #if _MSC_VER > 1400 > // ... > #endif > ``` > > (assuming the Google folks even care about supporting older versions of MSVC, I know Chromium does not.) This is already fixed in gflags 2.2.0. I'll check the compiler version like you suggest.

Comment on attachment 8847090 [details] Bug 964133 - Build gflags; .mielczarik https://reviewboard.mozilla.org/r/120146/#review126888 > Are you upstreaming this change? Yes, in https://github.com/gflags/gflags/pull/207

https://hg.mozilla.org/mozilla-central/rev/038fcfb056af https://hg.mozilla.org/mozilla-central/rev/19823a25fecc https://hg.mozilla.org/mozilla-central/rev/a6ecc2e6bb1e https://hg.mozilla.org/mozilla-central/rev/db51509d9011 https://hg.mozilla.org/mozilla-central/rev/420f5091594c https://hg.mozilla.org/mozilla-central/rev/801cec6e49c8 https://hg.mozilla.org/mozilla-central/rev/9dee0d5ae0ea https://hg.mozilla.org/mozilla-central/rev/c539db176fe6 https://hg.mozilla.org/mozilla-central/rev/c4392f466eaa