Remove hand-rolled cases of protobufs generations/compilation

Remove hand-rolled cases of protobufs generations/compilation

using src/build/protoc.gypi to offer consistency between the other
compilation steps, as well as ensuring that important variables (such as
hard_dependency) are always properly set.

BUG=109955
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120063

[KushalP, 2012-01-13 00:14:10]

This is my first attempt at fixing the GYP file. Generation appears to work fine
for the following cases:

- GYP_GENERATORS=make GYP_DEFINES=clang=1 ./build/gyp_chromium
- GYP_GENERATOR=gypd ./build/gyp_chromium

Neither show any errors.

I need to head off now, but will compile locally to make sure it actually works
as expected.

[Ryan Sleevi, 2012-01-13 01:21:52]

http://codereview.chromium.org/9148084/diff/1/chrome/app/policy/cloud_policy_...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

http://codereview.chromium.org/9148084/diff/1/chrome/app/policy/cloud_policy_...
chrome/app/policy/cloud_policy_codegen.gyp:66: 'type': 'static_library',
We don't want this. It should still be a 'none' target (and that's fine).

This changes the semantics to actually produce a static lib, which should then
fail compiling due to ODR violations when 'policy' is built (also as a static
library)

http://codereview.chromium.org/9148084/diff/1/chrome/app/policy/cloud_policy_...
chrome/app/policy/cloud_policy_codegen.gyp:71: 'proto_in_dir': 'app/policy',
Doesn't this need to be '<(policy_out_dir)/policy' to match the
semantics/behaviours of the hand-rolled implementation?

The same for line 68.

These are generated files (by 'cloud_policy_code_generate'), so they don't exist
naked in the tree AFAICT.

http://codereview.chromium.org/9148084/diff/1/chrome/app/policy/cloud_policy_...
chrome/app/policy/cloud_policy_codegen.gyp:81: 'type': 'static_library',
Same here (re: none vs static_library)

[KushalP, 2012-01-13 12:47:48]

http://codereview.chromium.org/9148084/diff/1/chrome/app/policy/cloud_policy_...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

http://codereview.chromium.org/9148084/diff/1/chrome/app/policy/cloud_policy_...
chrome/app/policy/cloud_policy_codegen.gyp:66: 'type': 'static_library',
That makes sense. I got the impression that we wanted generated object files
because we'll only be including headers.

On 2012/01/13 01:21:52, Ryan Sleevi wrote:
> We don't want this. It should still be a 'none' target (and that's fine).
> 
> This changes the semantics to actually produce a static lib, which should then
> fail compiling due to ODR violations when 'policy' is built (also as a static
> library)

[KushalP, 2012-01-16 23:28:13]

Finally getting somewhere. It now generates "something" and compiling fails with
the following:


  CXX(target)
out/Debug/obj.target/browser/chrome/browser/policy/user_policy_cache.o
In file included from chrome/browser/policy/user_policy_cache.cc:5:
In file included from ./chrome/browser/policy/user_policy_cache.h:14:
./chrome/browser/policy/cloud_policy_cache_base.h:14:10: fatal error: 
      'chrome/browser/policy/proto/device_management_backend.pb.h' file not
found
#include "chrome/browser/policy/proto/device_management_backend.pb.h"
         ^
1 error generated.

Will take a better look at this tomorrow. Getting a little frustrated at trying
to figure out where these generated files are actually going on my hard drive.

[KushalP, 2012-01-20 01:00:03]

After some fiddling I've figured out how to reproduce the fails after I've done
a rebase! :D

The error that appears is: make: *** No rule to make target
`chrome/app/policy/cloud_policy.proto', needed by
`out/Debug/pyproto/chrome/browser/policy/proto/cloud_policy_pb2.py'.  Stop.

cloud_policy.proto is generated by the target which is a dependency of
'cloud_policy_proto_compile', called 'cloud_policy_code_generate'.

The generated cloud_policy.proto file lands in
./out/Debug/obj/gen/policy/policy/cloud_policy.proto for me.

Setting 'proto_in_dir' to '../../../out/Debug/obj/gen/policy/policy', doesn't
stop the error (same as posted in the top of this comment): make: *** No rule to
make target `chrome/app/policy/cloud_policy.proto', needed by
`out/Debug/pyproto/chrome/browser/policy/proto/cloud_policy_pb2.py'.  Stop.

proto_in_dir should be the relative directory location of the .proto file from
the dir that the GYP file is in. Any ideas why this isn't working as intended?

[Ryan Sleevi, 2012-01-22 01:04:03]

As I mentioned on the bug, care needs to be taken about the paths here.

I'd suggest taking a careful look at this GYP file and its rather extensive use
of variables, double checking that all the paths are expanding to their
equivalents. You also want to make sure that you're keeping the usage of
variables consistent - don't hardcode a path in one place, but then use a
variable in another, when they expand to the same.

See the note below about possibly needing to run GYPD for further diagnostics.

http://codereview.chromium.org/9148084/diff/10001/chrome/app/policy/cloud_pol...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

http://codereview.chromium.org/9148084/diff/10001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:72: 'proto_out_dir':
'chrome/browser/policy/proto',
You should be using <(policy_path_substr) here (because of lines 112-115)

http://codereview.chromium.org/9148084/diff/10001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:77: 'includes': [
'../../../build/protoc.gypi' ],
You may want to double check the GYP syntax rules, but it's possible that
because this 'includes' introduces a dependencies that they may be replacing the
ones from 74-76, causing out of order building.

Using GYPD or related should help you show the dependencies, as I suggested on
the bug.

http://codereview.chromium.org/9148084/diff/10001/chrome/app/policy/cloud_pol...
chrome/app/policy/cloud_policy_codegen.gyp:83:
'../../browser/policy/proto/chrome_device_policy.proto',
Why did you stop using <(proto_rel_path) here?

[KushalP, 2012-01-26 12:06:22]

I couldn't get GYPD to work. Instead I'm diffing the generated makefiles.

You can see the diff for cloud_policy_proto_compile.target.mk here:
https://gist.github.com/1682453

The paths are completely wrong. Especially for the the Python protobufs. I'm
starting to wonder whether this needs refactoring elsewhere so that the
protoc.gyp usage doesn't need as much work. WDYT?

[KushalP, 2012-01-26 12:18:06]

The biggest issue is the different file paths for the the Python files. They
reside in device_management_pb/ instead of the relative dir (like the headers
and body files).

[KushalP, 2012-01-31 13:29:34]

It appears to be building and finding the relevant files that it needs.

I'm not quite sure why all of those tests are failing at the moment as some of
them don't appear to have any output to open (URL not found).

I'll try to run those tests locally later today/tonight and see what happens.

[KushalP, 2012-01-31 13:59:58]

...and it's passed for mac_rel. That's interesting. Maybe the errors are
occurring due to clobber builds?

[KushalP, 2012-01-31 14:00:59]

On 2012/01/31 13:59:58, KushalP wrote:
> ...and it's passed for mac_rel. That's interesting. Maybe the errors are
> occurring due to clobber builds?

Nope! That was on a non-clobber build.

[Ryan Sleevi, 2012-01-31 19:55:40]

LGTM, mod nit below

https://chromiumcodereview.appspot.com/9148084/diff/24001/chrome/app/policy/c...
File chrome/app/policy/cloud_policy_codegen.gyp (right):

https://chromiumcodereview.appspot.com/9148084/diff/24001/chrome/app/policy/c...
chrome/app/policy/cloud_policy_codegen.gyp:86: 'proto_out_dir':
'chrome/browser/policy/proto',
<(proto_path_substr) here as well?

[KushalP, 2012-01-31 22:32:04]

Adding Joao as they're an OWNER for chrome/app/policy.

Summary of status: Patch Set #6 has a clean trybot run, Patch Set #7 is just for
a quick reviewer nit.

[Joao da Silva, 2012-02-01 10:09:05]

lgtm

http://codereview.chromium.org/9148084/diff/26003/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/9148084/diff/26003/net/test/test_server.cc#new...
net/test/test_server.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
Nit: update copyright year

[commit-bot: I haz the power, 2012-02-01 12:32:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kushi.p@gmail.com/9148084/32001

[commit-bot: I haz the power, 2012-02-01 14:09:53]

Change committed as 120063