Intent to change testing/* #include paradigm (and roll gtest/gmock)

[Gabriel Charette]

Hello all,

This is probably insane but I'd like to roll gtest/gmock's DEPS (which haven't budged in year...).

The single thing you'll all care about is that I think we should change our include paradigm from

  #include "testing/gtest/include/gtest/gtest.h"

to

  #include "gtest/gtest.h"

(gtest.gyp/gmock.gyp in fact already have the "include_dirs" in place that allow this!)

This is required because the upstream repo has moved its files up a directory and we would need to update all includes to 

  #include "testing/gtest/googletest/include/gtest/gtest.h"  

anyways...

Does using the short include path sound good to everyone?

If so: I'll go ahead and add a PRESUBMIT prohibiting the full path so that I don't have to fight against incoming commits while fixing it all up.

If not: I pretty much can't roll DEPS because updating ~7000 files in lock step with rolling DEPS is pretty much impossible (well I guess I could close the tree, etc...).

Tracker bug: http://crbug.com/630705 (other details there -- like the fact that gmock is now in gtest and we could get rid of the SVN gmock repo :-)).

Cheers,

Gab

[Mark Mentovai]

I tried to do a gtest/gmock update last year and had a prototype in https://codereview.chromium.org/1466413002/ before deciding that I hated it.

I like your #include idea better. That’s how gtest wants to be #included.

Anyway, your point might be a little buried. The way I see this is: We have to change all of our gtest #includes in order to update gtest anyway, so why not change them to the form that gtest prefers?

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

[Brett Wilson]

checkdeps and "gn check" currently assume files are full paths, so this change will break our checks here. Ideally our tools will be smarter, but they currently aren't, and I think these tools are very important.

I also think it's confusing any time we have includes that aren't real paths. I get annoyed every time I try to find a generated header.

So I think we should not violate the style guide in this respect and keep using full paths.

Brett

[John Abd-El-Malek]

+1 to keeping full paths, I also find it confusing when I have to hunt around.

Note also that adding more include directories makes the build slower on Windows.

Given that 7000 files have to be changed either way, it seems we can add temporary redirects. i.e.

-add "src/testing" as a global include directory, and then put a forwarding header in src/testing/testing/gtest/include/gtest/gtest.h temporarily

-roll

-update the 7k files at your leisure

-remove testing/testing

[Gabriel Charette]

On Fri, Jul 22, 2016 at 3:34 PM John Abd-El-Malek <j...@chromium.org> wrote:

+1 to keeping full paths, I also find it confusing when I have to hunt around.

Note also that adding more include directories makes the build slower on Windows.

Note on speed though : these include paths are already added.

Given that 7000 files have to be changed either way, it seems we can add temporary redirects. i.e.

-add "src/testing" as a global include directory, and then put a forwarding header in src/testing/testing/gtest/include/gtest/gtest.h temporarily

-roll

-update the 7k files at your leisure

-remove testing/testing

Interesting idea, thought this couldn't be done per inability to modify anything in testing/gtest but hadn't thought of having testing/testing/gtest and redirecting it :-).

[Gabriel Charette]

On Fri, Jul 22, 2016 at 3:42 PM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Jul 22, 2016 at 3:34 PM John Abd-El-Malek <j...@chromium.org> wrote:

+1 to keeping full paths, I also find it confusing when I have to hunt around.

Note also that adding more include directories makes the build slower on Windows.

Note on speed though : these include paths are already added.

Given that 7000 files have to be changed either way, it seems we can add temporary redirects. i.e.

-add "src/testing" as a global include directory, and then put a forwarding header in src/testing/testing/gtest/include/gtest/gtest.h temporarily

-roll

-update the 7k files at your leisure

-remove testing/testing

Interesting idea, thought this couldn't be done per inability to modify anything in testing/gtest but hadn't thought of having testing/testing/gtest and redirecting it :-).

I'll give that a shot, thanks!

[Anand]

I have a CL that rolls gtest which I was about to submit: https://codereview.chromium.org/2169193002/

[Gabriel Charette]

On Fri, Jul 22, 2016 at 6:28 PM Anand <ami...@chromium.org> wrote:

I have a CL that rolls gtest which I was about to submit: https://codereview.chromium.org/2169193002/

Awesome, I'm sure happy to let someone else do this roll :-).

So it seems like what you're doing is:

1) Move mapping of DEPS to testing/third_party/googletest.

2) Add redirect headers in place of the old ones (under testing/gtest/include/* which is no longer controlled by DEPS).

Does that work as an incremental patch? (gclient knows to delete the old DEPS before syncing the new files in the same place?) I'm worried that people will end up with multiple copies of the same files (typically when DEPS are moved, the old path is at least removed from .gitignore so that if you're not using gclient sync -D you see cruft behind and can manually clean it. I feel like I've seen re-usage of an old DEPS path cause issues in the past...

Also, this feels pretty permanent (and even hard to undo if we decide we don't like it per having to unroll the steps I'm not sure will even roll forward properly above..), do you not intend to change the include paths over the codebase?

If not this feels like it semi goes against the discussion here of keeping full include paths (they're still full paths but to permanent redirect headers..)

Lastly: googletest now includes gmock, so we should probably also strip gmock.

In that light, I prefer the version proposed by John (keeping the DEPS in place, add temporary testing/testing/gtest/include/* redirect headers, do a massive search/replace for existing includes, remove redirect headers). And, bonus, this also allows to replace SVN gmock DEPS in one swoop.

[Gabriel Charette]

Catching up on emails I realize discussion is also happening on bug, let's keep it going there instead : http://crbug.com/630705