RFC: the way we #include v8 headers

[Paweł Hajdan, Jr.]

This is somewhat nit-like, but I'd like to make our build a bit prettier for Linux distros, one step at a time. :-D

Currently we use #include "v8/include/v8.h" for v8 headers. However, when compiling with system v8 (again, just Chromium builds, never Google Chrome), the header is in /usr/include/v8.h, so the mentioned #include line doesn't "match it". An ugly hack around this is just symlinking v8/include inside the chromium tree to /usr/include, and it works. However, I wonder that maybe if we change the way v8 is included, that part could be made less hacky.

Here are different ways in which we handle similar situations:

1. Use a shim header, e.g. third_party/speex/speex.h:

// This is a shim header to include the right speex header.
// Use this instead of referencing the speex header directly.

#if defined(USE_SYSTEM_SPEEX)
#include <speex/speex.h>
#else
#include "third_party/speex/include/speex/speex.h"
#endif


2. Explicit #if at every #include, e.g. net/base/crl_set.cc (note this is .cc filed compared to .h file above):

#if defined(USE_SYSTEM_ZLIB)
#include <zlib.h>
#else
#include "third_party/zlib/zlib.h"
#endif


3. Using the prefix for our advantage:

#include "unicode/coll.h"
#include "unicode/locid.h"
#include "unicode/uchar.h"
#include "unicode/uscript.h"


Note that with above, we set the include path to contain our third_party dir, and rely on distros removing bundled headers when using system ICU, and in that case headers from /usr/include are used.


What solution would you prefer for v8? Note that there is usage in both WebKit and chromium tree, and in WebKit we seem to be using just #include "v8.h", see e.g. src/third_party/WebKit/Source/WebKit/chromium/src/WebKit.cpp. How about converting chromium code to the same style? I could do that.


Paweł

[Ryan Sleevi]

On Thu, Oct 18, 2012 at 4:02 PM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> This is somewhat nit-like, but I'd like to make our build a bit prettier for
> Linux distros, one step at a time. :-D

And what of the case where security updates are made to Chromium's
(pulled) v8 but that are not in the system v8? This happens regularly
with other libraries, such that for some critical, security-sensitive
libraries, we've retroactively removed support for using the system
version (ZLIB is a perfect example of where we no longer use or will
even compile with the system zlib)

I think there's a real trade-off for security, since changes such as
to Chromium's zlib, particularly if they're not yet upstreamed or not
accepted upstream, are not visible to the package/distro maintainers.

That said, and biased towards both Chromium style and Chromium
readability, I would prefer shim-headers such as #1 over the solutions
presented in #2 or #3, especially for widely-used headers such as V8.

[Adam Barth]

I don't think we should support using the system version of V8.  We depend on implementation details of V8 for the exact version we build with.

Adam

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

[David Turner]

As someone who regrettably had to deal with binary compatibility issues for many years, I'd like to warn against using any system version of a C++ library, unless it has an official ABI stability policy in place (and I don't think any of the Chromium dependencies does).

[Adam Barth]

Even in cases where the API is ABI compatible, we develop WebKit and
V8 in parallel assuming details of how each component works.
Otherwise, we wouldn't be able to make improvements like
<https://bugs.webkit.org/show_bug.cgi?id=98725>.

Obviously, Chromium is an open source project and you're free to try
to use the system version of V8, but it's going to break in difficult
to predict (or even realize) ways.

Adam

[Paweł Hajdan, Jr.]

On Gentoo Linux we use system v8 since October 2011 with no problems. We can handle dependencies properly (e.g. depend on a version of v8 that version of chrome was released with if needed), and watch ABI changes: http://upstream-tracker.org/versions/v8.html . Thanks to SONAME all reverse dependencies of v8 are properly rebuilt on update.

More specific comments inline.

On Fri, Oct 19, 2012 at 1:36 AM, Adam Barth <aba...@chromium.org> wrote:

Even in cases where the API is ABI compatible, we develop WebKit and
V8 in parallel assuming details of how each component works.
Otherwise, we wouldn't be able to make improvements like
<https://bugs.webkit.org/show_bug.cgi?id=98725>.

I see nothing in that change that would obviously break with system V8 setup that Gentoo uses. 

Obviously, Chromium is an open source project and you're free to try
to use the system version of V8, but it's going to break in difficult
to predict (or even realize) ways.

Using the same version of v8 that was used in given Chrome release should keep things reasonably safe. Note that this is not like situation from distros you might know, which ship outdated system libraries. On Gentoo we frequently hit the opposite case, i.e. system libraries (except v8) are more recent than Chrome's bundled copies.

 

On Fri, Oct 19, 2012 at 1:12 AM, David Turner <di...@chromium.org> wrote:
> As someone who regrettably had to deal with binary compatibility issues for
> many years, I'd like to warn against using any system version of a C++
> library, unless it has an official ABI stability policy in place (and I
> don't think any of the Chromium dependencies does).

I'm aware of possible C++ ABI issues, so in Gentoo we use a very conservative policy of handling SONAMEs. Basically each version of v8 has a different SONAME.

[Adam Barth]

On Fri, Oct 19, 2012 at 9:47 AM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> On Gentoo Linux we use system v8 since October 2011 with no problems. We can
> handle dependencies properly (e.g. depend on a version of v8 that version of
> chrome was released with if needed), and watch ABI changes:
> http://upstream-tracker.org/versions/v8.html . Thanks to SONAME all reverse
> dependencies of v8 are properly rebuilt on update.

The "if needed" part of this paragraph is what concerns me. How do
you know whether Chromium need a particular version? We only test
with precisely one version of V8. It's possible that Chromium won't
work properly with other versions. Do you run the full test suite in
every configuration to validate that you've got the right version?
What about the performance tests?

> More specific comments inline.
>
> On Fri, Oct 19, 2012 at 1:36 AM, Adam Barth <aba...@chromium.org> wrote:
>>
>> Even in cases where the API is ABI compatible, we develop WebKit and
>> V8 in parallel assuming details of how each component works.
>> Otherwise, we wouldn't be able to make improvements like
>> <https://bugs.webkit.org/show_bug.cgi?id=98725>.
>
> I see nothing in that change that would obviously break with system V8 setup
> that Gentoo uses.

Precisely. The dependency issue is very subtle, and I only know about
it because I'm engrossed in the details of that particular change.

>> Obviously, Chromium is an open source project and you're free to try
>> to use the system version of V8, but it's going to break in difficult
>> to predict (or even realize) ways.
>
> Using the same version of v8 that was used in given Chrome release should
> keep things reasonably safe. Note that this is not like situation from
> distros you might know, which ship outdated system libraries. On Gentoo we
> frequently hit the opposite case, i.e. system libraries (except v8) are more
> recent than Chrome's bundled copies.
>
>> On Fri, Oct 19, 2012 at 1:12 AM, David Turner <di...@chromium.org> wrote:
>> > As someone who regrettably had to deal with binary compatibility issues
>> > for
>> > many years, I'd like to warn against using any system version of a C++
>> > library, unless it has an official ABI stability policy in place (and I
>> > don't think any of the Chromium dependencies does).
>
> I'm aware of possible C++ ABI issues, so in Gentoo we use a very
> conservative policy of handling SONAMEs. Basically each version of v8 has a
> different SONAME.

I'm glad that works for you. I don't think the Chromium project
should expend any effort supporting this configuration.

Adam

[Paweł Hajdan, Jr.]

On Fri, Oct 19, 2012 at 10:00 AM, Adam Barth <aba...@chromium.org> wrote:

On Fri, Oct 19, 2012 at 9:47 AM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> On Gentoo Linux we use system v8 since October 2011 with no problems. We can
> handle dependencies properly (e.g. depend on a version of v8 that version of
> chrome was released with if needed), and watch ABI changes:
> http://upstream-tracker.org/versions/v8.html . Thanks to SONAME all reverse
> dependencies of v8 are properly rebuilt on update.

The "if needed" part of this paragraph is what concerns me.  How do
you know whether Chromium need a particular version?

In Gentoo, I keep the V8 version in sync with the bundled V8 version. I have included code to print out that info during build process:

	local v8_bundled="$(chromium_bundled_v8_version)"
	local v8_installed="$(chromium_installed_v8_version)"
	einfo "V8 version: bundled - ${v8_bundled}; installed - ${v8_installed}"

 

We only test with precisely one version of V8.

Given that there are no official tests with all the other system libraries and their version that Chromium packaged by various Linux distro uses, I don't think that's a big concern.

Remember, bug reports about Chromium packaged by distros should go to distros first. When you see a bug report about a distro compiled Chromium, feel totally free to direct the user to his distro bug tracker first.

 

It's possible that Chromium won't work properly with other versions.  Do you run the full test suite in
every configuration to validate that you've got the right version?

Only a subset of tests is run (and no performance tests). Note that this goes through community testing process, and seriously, I think this is really a concern of the distro, not the Chromium project.

Let's not repeat the discussion about system libraries, it has been agreed before that we do provide options to use system libraries only on a best-effort basis, i.e. no guarantees and they may be broken.

Paweł

[Adam Barth]

On Mon, Oct 22, 2012 at 9:49 AM, Paweł Hajdan, Jr.

I'm confused. I thought we had resolved this issue via chat.

Adam

[Paweł Hajdan, Jr.]

On Mon, Oct 22, 2012 at 10:20 AM, Adam Barth <aba...@chromium.org> wrote:

I'm confused.  I thought we had resolved this issue via chat.

Sorry about the confusion, I just hit reply here to answer your questions on the mailing list for reference. 

On Thu, Oct 18, 2012 at 4:10 PM, Ryan Sleevi <rsl...@chromium.org> wrote:

And what of the case where security updates are made to Chromium's

(pulled) v8 but that are not in the system v8?

Gentoo's system V8 is kept in sync with Chromium's V8, and in fact we even issue security advisories so that people who use just v8 with e.g. node.js or other packages can update too. See e.g. http://www.gentoo.org/security/en/glsa/glsa-201205-04.xml

 

This happens regularly with other libraries, such that for some critical, security-sensitive
libraries, we've retroactively removed support for using the system
version (ZLIB is a perfect example of where we no longer use or will
even compile with the system zlib)

I just fixed that, see https://codereview.chromium.org/11194068/

 

I think there's a real trade-off for security, since changes such as
to Chromium's zlib, particularly if they're not yet upstreamed or not
accepted upstream, are not visible to the package/distro maintainers.

The patches are public, and distros do use them. :) In fact, propagating them to the system versions of libraries makes every package secured, not just Chrome. This is important from a distro perspective, and it's also one of main reasons for using system versions of libraries, see https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries

 

That said, and biased towards both Chromium style and Chromium
readability, I would prefer shim-headers such as #1 over the solutions
presented in #2 or #3, especially for widely-used headers such as V8.

The problem with shim headers for v8 would be that the source directory of v8 would have to be moved one level down. I think a more pragmatic approach would be to just use #3 (or if there are any doubts about using the right header, #2). It seems we have only one copy of v8 in the source tree, so there shouldn't be much confusion that could happen with e.g. our many copies of logging.h.

Ah, and note that this #include talk would be best-effort, i.e. if someone forgets to do that, it's fine, it'll be detected on distros and can be fixed in Chromium tree afterwards.

[David Turner]

I don't think the Chromium authors should have to care about what distributions are doing. The patch you point to actually demonstrate something that I do not want to see in the Chromium code base, e.g.:

#if defined(USE_SYSTEM_ZLIB)

// System zlib is not expected to have workaround for http://crbug.com/139744,

// so disable compression in that case.

// TODO(phadjan.jr): Remove the special case when it's no longer necessary.

static const int kCompressorLevel = 0;

#else  // !defined(USE_SYSTEM_ZLIB)

static const int kCompressorLevel = 9;

#endif  // !defined(USE_SYSTEM_ZLIB)

The snippet above is only one of many in the corresponding patch, where USE_SYSTEM_ZLIB results in special-case code in several parts of the source.

The problem I see is that there is no way to know exactly when this special code will no longer be necessary. As such, it will have to be maintained indefinitely (or let's say several years) by the Chromium team members without any benefit to them. That doesn't look like a lot for now, but what if we start supporting system-v8, system-breakpad, system-whatever. The probability of cruft and maintenance burden is significant, imho.

That's not to say that we should prevent you to use the system zlib, (or system v8, or whatever), but I'd be much better if this support wouldn't result into invasive source code changes.

As such, I'd favor a solution that uses auto-generated shim headers that mirror the Chromium source hierarchy to point to the system ones, as in:

out/Debug/system_headers_shims/

  third_party/

    zlib/

      zlib.h     ---> #include <zlib.h>

  v8/

    include/

      v8.h       --> #include <v8.h>

      ...

  ...

And place out/Debug/system_header_shims at the start of the directory search path through the appropriate gyp rules.

This way, the Chromium sources don't need to care about distro-specific builds, and all the maintenance burden is isolated to the shim headers generation.

--

[Darin Fisher]

I agree, and I think this is a good point.  We shouldn't do anything that uglifies or adds tax to the Chromium code base here.  I'm OK with Linux distros doing extra work to support system libs, but it should be done transparently.

-Darin