WebRTC base overrides

[Ricardo Vargas]

I'm working on a change to base and I'm hitting errors compiling WebRTC:

-------------------

../../third_party/webrtc/overrides/webrtc/base/logging.cc:22

../../base/numerics/safe_conversions.h:57:3: error: no member named 'ERROR' in namespace 'rtc'; did you mean 'LERROR'?

  NOTREACHED();

  ^~~~~~~~~~~~

../../base/logging.h:703:26: note: expanded from macro 'NOTREACHED'

#define NOTREACHED() LOG(ERROR) << "NOTREACHED() hit in " << \

                     ~~~~^~~~~~

../../third_party/webrtc/overrides/webrtc/base/logging.h:191:38: note: expanded from macro 'LOG'

#define LOG(sev) DIAGNOSTIC_LOG(rtc::sev, NONE, 0)

                                     ^

../../third_party/webrtc/overrides/webrtc/base/logging.h:183:27: note: expanded from macro 'DIAGNOSTIC_LOG'

      __FILE__, __LINE__, sev, VLOG_IS_ON(sev), \

                          ^

../../third_party/webrtc/overrides/webrtc/base/logging.h:89:24: note: 'LERROR' declared here

                       LERROR = LS_ERROR };

http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/18354/steps/compile/logs/stdio

----------------

third_party/webrtc/overrides/webrtc/base/logging.cc:

#include "base/atomicops.h"

#include "base/strings/string_util.h"

#include "base/threading/platform_thread.h"        <---------------------

#include "third_party/webrtc/base/ipaddress.h"

#include "third_party/webrtc/base/stream.h"

So, files from third_party/webrtc/overrides/webrtc/base are including files from src/base which looks wrong to me (given that webrtc is pulled in via DEPS)

Is that expected?

[Ryan Sleevi]

No, that's buggy behaviour that shouldn't be happening.

Historically, we've addressed this by keeping the Chromium-dependent directory checked into the Chromium tree, and then had a sub-directory within that import the 'upstream' source.

An example of this is src/third_party/libjingle

The libjingle/overrides is part of the Chromium tree, while libjingle/source is checked out via DEPS

It appears webrtc is checking out all of third_party/webrtc via DEPS, but also depending on //base, which is wrong.

[Tommi]

+kjellander

The overrides folder in WebRTC is actually checked into the WebRTC repo and not Chromium's, which is different from libjingle, but I suspect that it may have been done so for the WebRTC FYI bots (that build ToT WebRTC with ToT Chromium).  Henrik would know more.

Afaik though, the concept should be the same as for the override folder in libjingle.  I.e. that it should be considered to be a part of Chromium and not WebRTC.  It's glue if you will, and therefore has Chromium dependencies.

The NOTREACHED macro was added recently, so maybe there have been some oversights there with how all of the above works.

On Tue, Mar 17, 2015 at 5:48 AM Justin Uberti <jub...@google.com> wrote:

+a couple webrtc folks who may know the history here

[Ryan Sleevi]

On Tue, Mar 17, 2015 at 12:00 AM, Tommi <to...@chromium.org> wrote:

+kjellander

The overrides folder in WebRTC is actually checked into the WebRTC repo and not Chromium's, which is different from libjingle, but I suspect that it may have been done so for the WebRTC FYI bots (that build ToT WebRTC with ToT Chromium).  Henrik would know more.

Afaik though, the concept should be the same as for the override folder in libjingle.  I.e. that it should be considered to be a part of Chromium and not WebRTC.  It's glue if you will, and therefore has Chromium dependencies.

Right, but that should be checked into the Chromium tree, not the WebRTC tree.

For example, if Ricardo wanted to fix that include, he would need to:

1) Land a shim in Chromium for the new header that includes the old header

2) Roll Chromium into WebRTC (FYI)

3) Update WebRTC to use the new header

4) Roll WebRTC into Chromium

5) Remove the old header

6) Roll Chromium into WebRTC

That's an unacceptably high burden, and specifically why we don't do cross-repo dependencies like this. If a project wishes to depend on Chromium, it should

1) Declare a common set of base interfaces that "embedders" (e.g. Chromium) can implement

2) In the Chromium tree, provide concrete implementations of these interfaces to bridge things

This is what libjingle does, which is the correct way.

If WebRTC were using that, if Ricardo wanted to change/refactor //base, then he would just need to

1) Update the Chromium implementation of the WebRTC interface to use the new Chromium logic

No rolls needed.

To put it differently, no project whose source is outside Chromium (e.g. not part of the basic Git checkout of src) should be including files that are part of Chromium.

I realize it's a subtle distinction - they conceptually accomplish the same goal, but the implementation choice can mean a big difference in how changes are made. We want to make sure, as much as possible, that Chromium code can be hermetically updated and completely refactored simply with a //src checkout, in a single commit.*

*Yes, special cases exist for ChromeOS, in which the ChromeOS maintainers repackage bits of //base, //crypto and //net as part of Platform2. However, the responsibility of managing that is their's, not the Chromium contributors.

[Henrik Kjellander]

I haven't been involved in setting up these overrides (just the build infrastructure) but it sounds reasonable to keep them in Chromium instead to make refactorings like these easier.

It wouldn't make any different for the bots we use for our rolls since they, as tommi@ pointed out, build ToT of both Chromium and WebRTC.

[Tommi]

I agree on that and am not aware of any objections.  It's the same teams that did this for libjingle and webrtc fwiw.

Let's fix it.

[Nico Weber]

Is there a bug for fixing this?

(For context, there used to be a few out-of-repo dependencies on base/. Since that turned out to be a huge burden as Ryan explained, brettw cleaned all of that up – we shouldn't be moving back into that world. Or, as it sounds that we already are back in that world, not stay there for long at least :-) )

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Ricardo Vargas]

crbug.com/468375

[John Abd-El-Malek]

Just to chime in, the previous third_party dependencies on base or other parts of the chromium repo were all errors, and that's why they were fixed. We have a policy that DEPS'd code can't call into code in the repo for the reasons outlined below.

One reason that this keeps reoccurring is that check_deps skips third_party. We should undo that, with the caveats that it would make that step even slower. This isn't trivial (i.e. there's checked in code in third_party that can call base, while others have their own "base" directory. I've filed https://code.google.com/p/chromium/issues/detail?id=469692 to track this. I think this would also be obsoleted by switching to GN, which does these header checks (although not for all codebase yet afaik).

On Thu, Mar 19, 2015 at 5:14 PM, Nico Weber <tha...@chromium.org> wrote: