changing the way third_party/ headers are handled

[Paweł Hajdan, Jr.]

I'd like to get your feedback about the idea I have. Note that it may also help with reducing the number of directories on the include paths, which I hope is a Good Thing. :) I know you're looking for benefits other than just making the codebase more friendly to Linux distro packagers, and that's it.

My understanding is that one of the reasons we use #include "third_party/..." is to ensure that we don't #include the system-provided header in /usr/include accidentally, e.g. if it happens to be installed and some build target in chromium source does not add appropriate third_party subdir to include path. The problem with this is that it's still possible for the bad scenario to happen. Third-party headers include other third-party headers without "third_party/..." prefix. For an incomplete list of such cases (because some things are pulled in via svn) try e.g. git grep '#include "' -- 'third_party/*.h'

Now here is an approach I was thinking about. You can call it brave, crazy, or something else. We wouldn't add anything inside third_party to include path. Instead, a code generator would be added to assist with creating shim headers. The shim headers would either simply forward to corresponding system headers when using system library (#include <...>) or be a copy of the bundled header, further rewritten to ensure all #includes have an appropriate prefix in them.

For example, suppose we have libX target in third_party, with headers in third_party/libX/include/libX/header{A,B}.h

headerA.h:

#include "libX/headerB.h"

some chromium .cc file:

#include "third_party/libX/include/libX/headerA.h"

I'd like to rewrite the above to:

headerA.h:

#include "header_shims/libX/headerB.h"

chromium .cc file:

#include "header_shims/libX/headerA.h"

What do you think?

[Peter Kasting]

On Tue, Dec 4, 2012 at 4:55 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

For example, suppose we have libX target in third_party, with headers in third_party/libX/include/libX/header{A,B}.h

headerA.h:

#include "libX/headerB.h"

some chromium .cc file:

#include "third_party/libX/include/libX/headerA.h"

I'd like to rewrite the above to:

headerA.h:

#include "header_shims/libX/headerB.h"

chromium .cc file:

#include "header_shims/libX/headerA.h"

What do you think?

I thought the purpose of the Chromium files using the full third_party/ path was to be consistent with other headers and compliant with the style guide, where we specify full paths (from the base of the checkout) when possible.

If the goal of your change is to prevent third_party headers from #including the wrong other headers, then I don't see why the Chromium (non-third_party) #includes need rewriting at all -- they work as-is.

And if you're rewriting the third_party files' #includes anyway, why not just rewrite them to look like the full path the Chromium .cc files use?  This avoids the need for shim headers entirely.

PK

[Rachel Blum]

Now here is an approach I was thinking about. You can call it brave, crazy, or something else. We wouldn't add anything inside third_party to include path. Instead, a code generator would be added to assist with creating shim headers. The shim headers would either simply forward to corresponding system headers when using system library (#include <...>) or be a copy of the bundled header, further rewritten to ensure all #includes have an appropriate prefix in them.

Here's my concern with shim headers: I live on a operating system with file system performance that is, well, "sub-par" is a nice way to put it. Adding a bunch of additional headers that need to be read/timestamped is not going to help build times, at all. 

If we must rewrite, I second Peter's suggestion - just rewrite the third_party headers to use proper paths.

 - rachel

[David Turner]

This has already been discussed several times in the list I believe, and the conclusion is that the Chromium sources/build should not have to care about distro builds.

In your specific case, all you need is to have shim headers named "third_party/libX/include/libX/headerA.h" that just #include <libX/headerA.h>, and place them in a directory that is custom to your build, and added to the include search path with -I before the Chromium project path.

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

[Paweł Hajdan, Jr.]

On Wed, Dec 5, 2012 at 4:50 AM, David Turner <di...@chromium.org> wrote:

This has already been discussed several times in the list I believe, and the conclusion is that the Chromium sources/build should not have to care about distro builds.

I'm not sure how you came to that conclusion. Yes, the topic does come up frequently. This thread is my attempt to satisfy all perceived requirements, so that we can solve the shim headers once and for all (this is currently not the case).

We are using shim headers, we have options to build with various system libraries. How do you think we got them? They are not officially supported, but they are allowed to be there. Please avoid comments that seem to just mean "I'd rather ignore Linux distros".

In your specific case, all you need is to have shim headers named "third_party/libX/include/libX/headerA.h" that just #include <libX/headerA.h>, and place them in a directory that is custom to your build, and added to the include search path with -I before the Chromium project path.

If nobody is concerned about using system headers by mistake in that scheme (see my first e-mail for the scenario that makes it break), and there is strong opposition to my plan above (because I'm somewhat concerned about such header mix-up), I could do that.

On Tue, Dec 4, 2012 at 4:59 PM, Peter Kasting <pkas...@chromium.org> wrote:

I thought the purpose of the Chromium files using the full third_party/ path was to be consistent with other headers and compliant with the style guide, where we specify full paths (from the base of the checkout) when possible.

Do you refer to http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_and_Order_of_Includes#Names_and_Order_of_Includes ? There is possible interpretation quirk here, i.e. whether third party headers are considered Chromium project files.

Anyway, "should" allows justified exceptions I think.

 

If the goal of your change is to prevent third_party headers from #including the wrong other headers, then I don't see why the Chromium (non-third_party) #includes need rewriting at all -- they work as-is.

It may seem they happen to work. However, I have presented a scenario in which that assumption can be broken, and in fact, we do miss third party gyp dependencies from time to time (I've done about 5 CLs fixing these problems myself). This is not so much about fixing some known problems with that (there are no afaik), but to make the whole system mixup-proof, both ways.

It's quite surprising to me, that it seems there are more concerns about stylistic issues, i.e. how our #includes look like, than for correctness issue, i.e. possibility of building using headers that do not match code we link to.

 

And if you're rewriting the third_party files' #includes anyway, why not just rewrite them to look like the full path the Chromium .cc files use?  This avoids the need for shim headers entirely.

I think we still need shim headers to support using system headers as an option. Rewriting the checked-in headers would make updates in third_party more cumbersome, especially in cases when we pull directly (or mirror directly) from upstream repo.

On Tue, Dec 4, 2012 at 5:17 PM, Rachel Blum <gr...@chromium.org> wrote:

Here's my concern with shim headers: I live on a operating system with file system performance that is, well, "sub-par" is a nice way to put it. Adding a bunch of additional headers that need to be read/timestamped is not going to help build times, at all. 

Do you mean Windows or Mac OS X? :)

Note that for most cases we already use shim headers. This thread is just about generating them automatically at build time and some additional changes, but it's not like something completely new.

[Peter Kasting]

On Wed, Dec 5, 2012 at 4:53 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

We are using shim headers, we have options to build with various system libraries. How do you think we got them? They are not officially supported, but they are allowed to be there. Please avoid comments that seem to just mean "I'd rather ignore Linux distros".

Well, frankly, I would.  I am not just indifferent to other distros wanting to build with system libraries, I am actively hostile to them when there is any impact on Chromium at all, even seemingly-trivial impacts, like using shim headers in our non-third_party source files.

Frankly, I would like a public note from a project tech lead -- that is, Ben or Darin -- that we care enough about this case that we want to support it officially both now and in the future.  Because the "unofficial" support we have now produces a seemingly-endless stream of emails and CLs from you trying to fix all the problems that constantly come up.  I greatly respect your dedication but I do not take it as self-evident that we should allow this kind of support to exist in the first place.  And in a case like this you're not the only one impacted.

If nobody is concerned about using system headers by mistake in that scheme (see my first e-mail for the scenario that makes it break), and there is strong opposition to my plan above (because I'm somewhat concerned about such header mix-up), I could do that.

Without dispensation (see above), we should be spending zero energy trying to safeguard third_party libraries against accidentally referring to the wrong headers.  I can't tell if your "scenario" refers to that or to something where Chromium .cc files #include a third_party header without using a third_party/ path.  If they do, that's simply wrong and should be fixed.  We should be compiling such files without third_party subdirs in the #include path so that this immediately breaks.

Anyway, "should" allows justified exceptions I think.

Not any more than the rest of the style guide allows this.  There's no distinction between SHOULD and MUST in the style guide -- the plain-English meaning of "do this" is the intended interpretation.

We do break that in some places where we have to -- like with GRIT generated headers -- but I believe those are cases where there isn't an easy way around.

If the goal of your change is to prevent third_party headers from #including the wrong other headers, then I don't see why the Chromium (non-third_party) #includes need rewriting at all -- they work as-is.

It may seem they happen to work. However, I have presented a scenario in which that assumption can be broken, and in fact, we do miss third party gyp dependencies from time to time (I've done about 5 CLs fixing these problems myself).

Again, you're going to have to be more concrete about which scenario you're describing, because unfortunately I'm not really following where/what you're referring to.  Perhaps I'm just tired.

 

It's quite surprising to me, that it seems there are more concerns about stylistic issues, i.e. how our #includes look like, than for correctness issue, i.e. possibility of building using headers that do not match code we link to.

It seems like there is no correctness issue if we stop thinking about cases where we're not linking against libraries we build ourselves. Is that true?

And if you're rewriting the third_party files' #includes anyway, why not just rewrite them to look like the full path the Chromium .cc files use?  This avoids the need for shim headers entirely.

I think we still need shim headers to support using system headers as an option. Rewriting the checked-in headers would make updates in third_party more cumbersome, especially in cases when we pull directly (or mirror directly) from upstream repo.

Ray of hope for you: I'm OK with shim headers if only third_party files use them and if the alternative to their existence is that we do more work to mirror or update third_party libraries.  That seems like a valid use of this.

Note that for most cases we already use shim headers. This thread is just about generating them automatically at build time and some additional changes, but it's not like something completely new.

This sort of comment probably belonged earlier.  Please make it clear where pieces of your proposal already exist.  For example, if we don't add any new shims, and we're just making existing ones easier to maintain, that's a clear win.  If the idea of making non-third_party .cc files refer to these shims is a necessary component, things are cloudier.  If it's just an additional safeguard for cases of building in non-official configurations, then whether we should do it depends on whether we want to call that a scenario we should officially worry about.

PK

[David Turner]

On Thu, Dec 6, 2012 at 1:53 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Wed, Dec 5, 2012 at 4:50 AM, David Turner <di...@chromium.org> wrote:

This has already been discussed several times in the list I believe, and the conclusion is that the Chromium sources/build should not have to care about distro builds.

I'm not sure how you came to that conclusion. Yes, the topic does come up frequently. This thread is my attempt to satisfy all perceived requirements, so that we can solve the shim headers once and for all (this is currently not the case).

We are using shim headers, we have options to build with various system libraries. How do you think we got them? They are not officially supported, but they are allowed to be there. Please avoid comments that seem to just mean "I'd rather ignore Linux distros".

I stand by my statement that the regular Chromium build, Chromium developers and Chromium-specific sources should not have to care about the requirements of Linux distro builds.

I am in no way opposed to Linux distributions doing their own builds using the system versions of some third-party libraries. I'm against the fact that this forces anyone else to reconsider how he/she works on the project, and this includes the coding convention used in Chromium code (of course third_party/ code is third-party, so this does not apply).

Your proposal specifically mandates changing chromium sources to accommodate your needs. IMHO, this is a huge no-go, especially if, as I outlined, there is a completely equivalent solution that doesn't require it.

In your specific case, all you need is to have shim headers named "third_party/libX/include/libX/headerA.h" that just #include <libX/headerA.h>, and place them in a directory that is custom to your build, and added to the include search path with -I before the Chromium project path.

If nobody is concerned about using system headers by mistake in that scheme (see my first e-mail for the scenario that makes it break), and there is strong opposition to my plan above (because I'm somewhat concerned about such header mix-up), I could do that.

I believe the problem you're describing is entirely different, i.e. that a third-party source includes a system header by mistake because it doesn't use the third_party/... convention.

For these cases, the regular Chromium build should provide its own set of shim headers to fix this instead, i.e. provide <foo/foo.h> somewhere which just does a #include "third_party/foo/include/foo/foo.h" or something, and ensure the third-party build uses it.

I don't know if this is a huge problem in practice, but if it is, it should certainly be addressed. Do we have a list of system of Linux libraries/packages that Chromium is allowed to depend on? If so, I think there is a way to "easily" auto-detect these leakages.

Both set of headers could be auto-generated and placed in appropriate locations under out/, with required support in the Chromium .gyp/.gypi files, controlled by various variables.

Most importantly, all this can be done without invasive changes to the source code.

- Digit

[Scott Hess]

AFAICT, there are two points in this.

1) If we have anything in third_party/ which is not hermetic, we need
to fix it. Full stop, really no discussion necessary. If this is a
problem and you have a solution which is not terribly intrusive, I
think that could receive support.

2) Intentionally making the build not hermetic to use system
libraries. This is clearly a non-goal of the project, and I think
that if someone wants to support that use case, they need to try to
not make that support terribly intrusive.

WRT being intrusive, rewriting our header inclusions for arbitrary
reasons is pretty intrusive. I can pretty easily describe to someone
why something they want to import into Chromium needs to live in
third_party/, and from there it's pretty easy for them to figure out
how the gyp file needs to look and how to reference things. Having
some weirdo auto-generated include hierarchy is not easy to explain,
and I already dislike it in the places we have such things going on,
because it makes it hard to track down where stuff is coming from. I
think we should have less of that, not more of it.

-scott

[Paweł Hajdan, Jr.]

On Thu, Dec 6, 2012 at 10:54 AM, Scott Hess <sh...@chromium.org> wrote:

AFAICT, there are two points in this.

1) If we have anything in third_party/ which is not hermetic, we need
to fix it.  Full stop, really no discussion necessary.  If this is a
problem and you have a solution which is not terribly intrusive, I
think that could receive support.

I have a local POC that indicates possible problems. In fact, what I'm proposing in this thread is related to system libraries, but is mainly taking Google Chrome use case into account, i.e. using bundled copies. What we are currently doing is perfectly fine for Linux distros.

Now what also motivated me to start this thread is V8 case. Currently I can't solve one quirk with V8 headers without a unified solution for all shim headers. And when I started thinking about the unified solution, I anticipated possible criticism, and wanted to also address concerns that only apply to Google Chrome build. It is reasonable to complain about changes you see as intrusive (I don't, but I understand and respect your point). What I'm saying is that correctness issues justify making intrusive changes.

The whole idea is: I'm not just doing some craziness for some crazy guys working on crazy distros, even if things may seem like that to you. :) I've also spotted a possible problem that applies to our bundled libraries, and my proposal aims to address both of these cases.

 

2) Intentionally making the build not hermetic to use system
libraries.  This is clearly a non-goal of the project, and I think
that if someone wants to support that use case, they need to try to
not make that support terribly intrusive.

That's why I brought up the correctness concern. My solution aims to be as pretty, clean, simple and transparent as possible. Interestingly, I couldn't see much technical feedback in this thread.

By the way, wrt explaining where headers come from: currently when doing things like #include "unicode/..." or #include "v8.h", are you sure you can correctly explain where these are coming from looking just at the .cc file and not the include path? What if people forget to set the right include path? (and that happens! I've fixed at least 5 problems of omission in the past). Our current way of doing things is masking these problems.

I'm happy to adjust my proposal if needed. I'd like to see more constructive feedback (that may mean more questions about the scenario where things go wrong with our current way). I think the Scott's post is a good start, thank you!

[Ami Fischman]

I have a local POC that indicates possible problems.

Can you add a pointer to it (bugreport/not-for-submit-patch/whatever)  to this thread?

Currently I can't solve one quirk with V8 headers

What is that quirk?

I've also spotted a possible problem that applies to our bundled libraries

 

What is this "possible problem"?

 

Interestingly, I couldn't see much technical feedback in this thread.

That's because you've been very opaque about the problems being solved.  Please be as explicit as you can about both the problems you see and the fixes you propose.

 

(and that happens! I've fixed at least 5 problems of omission in the past)

Can you point to example bugs/patches to make concrete the problem you describe?

 

I'm happy to adjust my proposal if needed. I'd like to see more constructive feedback

I tried replying to your original email and deleted my draft when I realized I 

a) wasn't sure what problems you're solving; and

b) wasn't sure what solution you're proposing.

Your original email stated that the (presumably only) benefit to Chrome is reducing the number of 3-p include dirs on the include path, but libX still shows up in your header_shims proposal, so I doubt this will be a big benefit.  Can you quantify this benefit? 

You imply that the header_shims solution will make it less likely that third_party .cc files accidentally #include system-provided .h's instead of the third_party version fo the same .h's (because of missing -I's).  But unless you plan to translate all third_party .cc's to use your header_shim/ paths, I don't see how you in fact affect anything there.  

Besides the previous two paragraphs, are there other benefits to (official) Chrome?

Cheers,

-a 

[Paweł Hajdan, Jr.]

On Thu, Dec 6, 2012 at 12:53 PM, Ami Fischman <fisc...@chromium.org> wrote:

I have a local POC that indicates possible problems.

Can you add a pointer to it (bugreport/not-for-submit-patch/whatever)  to this thread?

Sure.

sudo apt-get install build-essential libpng-dev

mkdir -p include_test/third_party/libpng

cd include_test

echo '#include "libpng/pngconf.h"' > third_party/libpng/png.h

touch third_party/libpng/pngconf.h

echo '#include "third_party/libpng/png.h"' > test.cc

g++ -c test.cc -o test

This prints:

In file included from third_party/libpng/png.h:1:0,

                 from test.cc:1:

/usr/include/libpng/pngconf.h:1360:9: error: ‘charf’ does not name a type

/usr/include/libpng/pngconf.h:1361:9: error: ‘charf’ does not name a type

/usr/include/libpng/pngconf.h:1362:9: error: ‘z_stream’ does not name a type

This compiles fine:

g++ -Ithird_party -c test.cc -o test

This demonstrates everything can be fine as long as you always remember to set the right include path. When you forget, system headers may be used. Now the failure mode in real scenario could be even worse: instead of a failed compile, you could have a successful compile with different object files using different headers for the same thing, which is obviously bad and recipe for mysterious problems.

 

You can look at third_party/re2/re2/re2.h for a very similar situation. There can be more.

Currently I can't solve one quirk with V8 headers

What is that quirk?

See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/7l-hHqUgMHA/zZArWi8kDk0J . That includes some offline discussions, but the idea is that I think the best way to solve problem from the cited thread is to create a unified solution for shim headers.

 

I've also spotted a possible problem that applies to our bundled libraries

 

What is this "possible problem"?

See the POC.

 

Interestingly, I couldn't see much technical feedback in this thread.

That's because you've been very opaque about the problems being solved.  Please be as explicit as you can about both the problems you see and the fixes you propose.

Okay, that's good feedback. Please let me know if the issue here is still unclear.

 

(and that happens! I've fixed at least 5 problems of omission in the past)

Can you point to example bugs/patches to make concrete the problem you describe?

Sure, just a few that I could find easily:

https://codereview.chromium.org/7763005

https://codereview.chromium.org/6873148/

https://codereview.chromium.org/2830013

https://codereview.chromium.org/2729012

Note that even in reviews people are not fully aware of the consequences of forgetting to set the right include path for third party libraries that may also be installed on the system.

 

I'm happy to adjust my proposal if needed. I'd like to see more constructive feedback

I tried replying to your original email and deleted my draft when I realized I 

a) wasn't sure what problems you're solving; and

b) wasn't sure what solution you're proposing.

Your original email stated that the (presumably only) benefit to Chrome is reducing the number of 3-p include dirs on the include path, but libX still shows up in your header_shims proposal, so I doubt this will be a big benefit.  Can you quantify this benefit?

Instead of possibly adding multiple subdirectories of third_party to include path, there would be just one directory like header_shims, with subdirectories for each third party library, but everything would be referred to with the header_shims prefix, so that just the single directory containing header_shims would need to be added to include path.

This is not different like night and day, but is some improvement, and definitely does not increase number of directories on the include path (addressing another concern I've heard). It is also not neutral, justifying some change.

 

You imply that the header_shims solution will make it less likely that third_party .cc files accidentally #include system-provided .h's instead of the third_party version fo the same .h's (because of missing -I's).  But unless you plan to translate all third_party .cc's to use your header_shim/ paths, I don't see how you in fact affect anything there.  

I wouldn't rewrite third party .cc files. They would still need include path set for their own headers. It's unlikely to make mistakes in the third_party gyp file since it has to be done just once. The problem I'm addressing is caused by changes in Chrome code that add more dependencies on third party libraries. Then it's much easier to miss an added dependency that seems to just work.

 

Besides the previous two paragraphs, are there other benefits to (official) Chrome?

I think the biggest benefit is no doubt as to which headers get used, system vs. bundled. Again, Chrome build is affected by this, i.e. there is a chance we #include system headers on Linux by mistake even when using bundled libraries. 

[Scott Hess]

AFAICT, the problem you are describing is that we do not have hermetic
builds, and your solution does not _add_ hermetic builds, so it would
still be possible to have the problem. If people include the wrong
thing, nothing about your proposal will cause a break, they'll just
have an appropriate place to put the right thing. Unless I
misunderstand things.

I think the problem of people accidentally including system libraries
where they should have included third_party libraries would be better
solved at the buildbot level - if there simply is not any system
include for that thing in the buildbot's environment, then they will
have to resolve it before they can check in.

-scott

[Ami Fischman]

This demonstrates everything can be fine as long as you always remember to set the right include path. When you forget, system headers may be used. Now the failure mode in real scenario could be even worse: instead of a failed compile, you could have a successful compile with different object files using different headers for the same thing, which is obviously bad and recipe for mysterious problems.

The necessary condition here is a missing -I; in order for the problem to go unnoticed, a developer needs to add a new dep on a third_party lib, forget to right .gyp dep, get "lucky" (have the particular -dev package installed system-wide), get past CR, land the change, and have all tree-closing bots happen to also have the -dev package installed, too.  Is this a problem that really happens?

You can look at third_party/re2/re2/re2.h for a very similar situation. 

Not sure what you're demonstrating here.

 

Currently I can't solve one quirk with V8 headers

What is that quirk?

See https://groups.google.com/a/chromium.org/d/msg/chromium-dev/7l-hHqUgMHA/zZArWi8kDk0J . That includes some offline discussions, but the idea is that I think the best way to solve problem from the cited thread is to create a unified solution for shim headers.

Why not just change the way the v8 package is built for gentoo? 

 

(and that happens! I've fixed at least 5 problems of omission in the past)

Can you point to example bugs/patches to make concrete the problem you describe?

Sure, just a few that I could find easily:

https://codereview.chromium.org/7763005

https://codereview.chromium.org/6873148/

https://codereview.chromium.org/2830013

https://codereview.chromium.org/2729012

AFAICT all of these fix the opposite problem (use_system_<foo> is broken), not correctness problems for Chrome.

You imply that the header_shims solution will make it less likely that third_party .cc files accidentally #include system-provided .h's instead of the third_party version fo the same .h's (because of missing -I's).  But unless you plan to translate all third_party .cc's to use your header_shim/ paths, I don't see how you in fact affect anything there.  

I wouldn't rewrite third party .cc files. They would still need include path set for their own headers. It's unlikely to make mistakes in the third_party gyp file since it has to be done just once. The problem I'm addressing is caused by changes in Chrome code that add more dependencies on third party libraries. Then it's much easier to miss an added dependency that seems to just work.

IIUC this is a "fix" for the non-Chrome case.

Besides the previous two paragraphs, are there other benefits to (official) Chrome?

I think the biggest benefit is no doubt as to which headers get used, system vs. bundled. Again, Chrome build is affected by this, i.e. there is a chance we #include system headers on Linux by mistake even when using bundled libraries. 

Do you have examples of bug/CLs of this happening? 

-a

[Paweł Hajdan, Jr.]

Okay, another approach. Please take a look at https://codereview.chromium.org/11470020/ .

Some notes:

1. Changes in gyp are limited to use_system_foo=1 case. Nothing special is required if you are using the bundled library.

2. No changes in .cc files. Just do the normal #include "third_party/..." thing. In fact, with that change we could undo several #ifdefs in existing code (the example shows that).

3. Header file names are explicitly listed to allow packagers to remove all files from third party directories (this is one way to make sure they are not used). This is an implementation detail, but I think you might be wondering what are reasons for that.

4. The credit for the idea goes to Darin. I talked to him offline a few weeks ago, but forgot the critical part of the solution. I'm sorry if what I've implemented is not really what he had in mind. I'm not trying to imply the quality of the solution based solely on whom the idea came from. Please hold me fully responsible for the implementation. :)

What do you think? Thank you for the feedback so far. I'm listening and iterating based on it.

[Ami Fischman]

Thanks for iterating.  I like the resulting edits to the .cc files in that CL!

Some thoughts on the implementation:

1) why write shims instead of building a symlink farm?

2) (assuming you keep the idea of explicit shims) it seems unfortunate to me to include shim-building as part of the build, since the only time it needs to change is when the present headers change.  What do you think of checking in these shims, instead?  Seems like an equivalent amount of dev work (either edit a .gyp when a new .h shows up or re-run the shim generator for that .h) but with less compute work done at build-time.

3) IWBN to assert that shims are *not* being used for use_system_foo=0 builds, and, more importantly, for official builds.

Cheers,

-a

[Greg Thompson]

On Fri, Dec 7, 2012 at 4:43 PM, Ami Fischman <fisc...@chromium.org> wrote:

Thanks for iterating.  I like the resulting edits to the .cc files in that CL!

Some thoughts on the implementation:

1) why write shims instead of building a symlink farm?

2) (assuming you keep the idea of explicit shims) it seems unfortunate to me to include shim-building as part of the build, since the only time it needs to change is when the present headers change.  What do you think of checking in these shims, instead?

As a middle ground, is it possible to do the shim-building during the runhooks phase so that it still happens locally, but only when updating?

[Paweł Hajdan, Jr.]

On Fri, Dec 7, 2012 at 1:43 PM, Ami Fischman <fisc...@chromium.org> wrote:

1) why write shims instead of building a symlink farm?

It's not obvious where the symlinks should point to. For example on BSDs packages are installed under /usr/local instead of /usr. By writing headers which just #include <...> the header we can easily ensure right thing happens.

 

2) (assuming you keep the idea of explicit shims) it seems unfortunate to me to include shim-building as part of the build, since the only time it needs to change is when the present headers change.  What do you think of checking in these shims, instead?  Seems like an equivalent amount of dev work (either edit a .gyp when a new .h shows up or re-run the shim generator for that .h) but with less compute work done at build-time.

1. Checking in lots of simple files is actually higher maintenance than modifying a single .gyp file. With the critical bits extracted to a single .gypi file and the generator, it's easy to fix things in a single place. Doing that with checked-in headers would be harder.

2. It's easier for distros to patch a single file than to say add multiple files in a patch. Imagine distros need to change something in the way headers are generated. It's much easier to tweak the generator, than to change multiple already existing files.

3. Not checking in any additional files helps keeping the impact very low for other Chrome developers.

 

3) IWBN to assert that shims are *not* being used for use_system_foo=0 builds, and, more importantly, for official builds.

I've checked that. The shims are not used. They are not even generated in that case. Not even is the generator called at all to produce the list of inputs. 

On Fri, Dec 7, 2012 at 5:16 PM, Greg Thompson <g...@chromium.org> wrote:

As a middle ground, is it possible to do the shim-building during the runhooks phase so that it still happens locally, but only when updating?

I'm not sure why you call that middle ground. If you do not use shims (usual case), they will not be generated at all (see above). And when you do, it's much better to do that at build time. If you actually use them, you should understand why. ;) I can explain more of course. 

[Ami Fischman]

I think I got confused somewhere along the way because at the top of the thread "shim" referred to a thing that may refer to either bundled or system headers, but at this point in the thread, "shim" refers only to redirecting headers pointing to system headers.

As long as shim-generation is not triggered for the Chrome use-case, I'm fine with it.

3) IWBN to assert that shims are *not* being used for use_system_foo=0 builds, and, more importantly, for official builds.

I've checked that. The shims are not used. They are not even generated in that case. Not even is the generator called at all to produce the list of inputs. 

By "assert" I meant the code, not its author :).  E.g. add to the generated headers:

#ifdef OFFICIAL_BUILD

#error Shim headers must not be used in official builds!

#endif

 

On Fri, Dec 7, 2012 at 5:16 PM, Greg Thompson <g...@chromium.org> wrote:

As a middle ground, is it possible to do the shim-building during the runhooks phase so that it still happens locally, but only when updating?

I'm not sure why you call that middle ground

Middle between checking in generated headers and (potentially) generating them on each build, probably.

Per above, I think it's fine not to do this.

Cheers,

-a

[Paweł Hajdan, Jr.]

On Mon, Dec 10, 2012 at 12:47 PM, Ami Fischman <fisc...@chromium.org> wrote:

As long as shim-generation is not triggered for the Chrome use-case, I'm fine with it.

Excellent! :-D

 

3) IWBN to assert that shims are *not* being used for use_system_foo=0 builds, and, more importantly, for official builds.

I've checked that. The shims are not used. They are not even generated in that case. Not even is the generator called at all to produce the list of inputs. 

By "assert" I meant the code, not its author :).  E.g. add to the generated headers:

#ifdef OFFICIAL_BUILD

#error Shim headers must not be used in official builds!

#endif

Done (https://codereview.chromium.org/11470020/).