Dependancy check for .proto files is added.

Dependancy check for .proto files is added.

proto_checker is added and checkdeps is modified so that import lines in proto files are checked for matching with rules stated in DEPS file.

BUG=684383
R=brettw@chromium.org

Committed: 4ca3556cb7098968142bb18f9eb0d1ea40c327f0

[Ramin Halavati, 2017-01-25 11:03:59]

rhalavati@chromium.org changed reviewers:
+ brettw@chromium.org

[Ramin Halavati, 2017-01-25 11:04:00]

Hi,

I have added .proto dependency check to scripts in src/buildtools. A separate CL
is created to update src/PRESUBMIT.py to use them:
https://codereview.chromium.org/2651553006/

Please review.

[brettw, 2017-01-25 21:12:18]

I like the idea of checking proto paths in general.

In looking at the imports in our proto files, I see none that use an absolute
path like we do in C++. Am I correct in reading your patch that these will then
just be ignored? So will this patch do anything?

If we want to move to consistently using full paths in protos, that sounds fine
to me. But then I think we should be more explicit about the goal here than
"possibly revisiting it in the future". In that case, we should probably have a
more formal proposal and have people more familiar with protos review it.

https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py
File checkdeps/proto_checker.py (right):

https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py#...
checkdeps/proto_checker.py:100: """Returns True iff the given path ends in one
of the extensions
I find "iff" harder to read and unnecessary (in programming you would never
write "if but not only if" in this type of context. CAn you just use "if"

[Ramin Halavati, 2017-01-26 06:52:01]

On 2017/01/25 21:12:18, brettw (ping after 24h) wrote:
> I like the idea of checking proto paths in general.
> 
> In looking at the imports in our proto files, I see none that use an absolute
> path like we do in C++. Am I correct in reading your patch that these will
then
> just be ignored? So will this patch do anything?
> 
> If we want to move to consistently using full paths in protos, that sounds
fine
> to me. But then I think we should be more explicit about the goal here than
> "possibly revisiting it in the future". In that case, we should probably have
a
> more formal proposal and have people more familiar with protos review it.
> 
> https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py
> File checkdeps/proto_checker.py (right):
> 
>
https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py#...
> checkdeps/proto_checker.py:100: """Returns True iff the given path ends in one
> of the extensions
> I find "iff" harder to read and unnecessary (in programming you would never
> write "if but not only if" in this type of context. CAn you just use "if"

I am sorry I did not include the background and my reason to add this CL.

Until now there was a limitation in proto_library.gni and protoc_wrapper.py that
one could not pass more than one import directory to protoc compiler using them.
Therefore, all of the required imports of each .proto file had to reside in its
own directory and sub-directories and it did not require dependency check.

But in this CL (https://codereview.chromium.org/2421333002/), I needed to import
a proto from another directory in my own .proto file and to do so, I needed to
create a CL that adds an option to add extra import directories for .proto files
(https://codereview.chromium.org/2646123002/). When this is added, protos can
start having absolute paths as in C++ and this CL is added to check them.

I have copied the code from C++ dependency check and added some import tests to
ensure that sub-directories of current directory are included by default and
other cases follow DEPS include rules.
Please advise if any other path is more suitable.

[Ramin Halavati, 2017-01-27 12:37:19]

On 2017/01/26 06:52:01, Ramin Halavati wrote:
> On 2017/01/25 21:12:18, brettw (ping after 24h) wrote:
> > I like the idea of checking proto paths in general.
> > 
> > In looking at the imports in our proto files, I see none that use an
absolute
> > path like we do in C++. Am I correct in reading your patch that these will
> then
> > just be ignored? So will this patch do anything?
> > 
> > If we want to move to consistently using full paths in protos, that sounds
> fine
> > to me. But then I think we should be more explicit about the goal here than
> > "possibly revisiting it in the future". In that case, we should probably
have
> a
> > more formal proposal and have people more familiar with protos review it.
> > 
> > https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py
> > File checkdeps/proto_checker.py (right):
> > 
> >
>
https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py#...
> > checkdeps/proto_checker.py:100: """Returns True iff the given path ends in
one
> > of the extensions
> > I find "iff" harder to read and unnecessary (in programming you would never
> > write "if but not only if" in this type of context. CAn you just use "if"
> 
> I am sorry I did not include the background and my reason to add this CL.
> 
> Until now there was a limitation in proto_library.gni and protoc_wrapper.py
that
> one could not pass more than one import directory to protoc compiler using
them.
> Therefore, all of the required imports of each .proto file had to reside in
its
> own directory and sub-directories and it did not require dependency check.
> 
> But in this CL (https://codereview.chromium.org/2421333002/), I needed to
import
> a proto from another directory in my own .proto file and to do so, I needed to
> create a CL that adds an option to add extra import directories for .proto
files
> (https://codereview.chromium.org/2646123002/). When this is added, protos can
> start having absolute paths as in C++ and this CL is added to check them.
> 
> I have copied the code from C++ dependency check and added some import tests
to
> ensure that sub-directories of current directory are included by default and
> other cases follow DEPS include rules.
> Please advise if any other path is more suitable.


Just pinging after 24h.
:)

[Ramin Halavati, 2017-01-30 17:09:37]

On 2017/01/27 12:37:19, Ramin Halavati wrote:
> On 2017/01/26 06:52:01, Ramin Halavati wrote:
> > On 2017/01/25 21:12:18, brettw (ping after 24h) wrote:
> > > I like the idea of checking proto paths in general.
> > > 
> > > In looking at the imports in our proto files, I see none that use an
> absolute
> > > path like we do in C++. Am I correct in reading your patch that these will
> > then
> > > just be ignored? So will this patch do anything?
> > > 
> > > If we want to move to consistently using full paths in protos, that sounds
> > fine
> > > to me. But then I think we should be more explicit about the goal here
than
> > > "possibly revisiting it in the future". In that case, we should probably
> have
> > a
> > > more formal proposal and have people more familiar with protos review it.
> > > 
> > >
https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py
> > > File checkdeps/proto_checker.py (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2653023004/diff/1/checkdeps/proto_checker.py#...
> > > checkdeps/proto_checker.py:100: """Returns True iff the given path ends in
> one
> > > of the extensions
> > > I find "iff" harder to read and unnecessary (in programming you would
never
> > > write "if but not only if" in this type of context. CAn you just use "if"
> > 
> > I am sorry I did not include the background and my reason to add this CL.
> > 
> > Until now there was a limitation in proto_library.gni and protoc_wrapper.py
> that
> > one could not pass more than one import directory to protoc compiler using
> them.
> > Therefore, all of the required imports of each .proto file had to reside in
> its
> > own directory and sub-directories and it did not require dependency check.
> > 
> > But in this CL (https://codereview.chromium.org/2421333002/), I needed to
> import
> > a proto from another directory in my own .proto file and to do so, I needed
to
> > create a CL that adds an option to add extra import directories for .proto
> files
> > (https://codereview.chromium.org/2646123002/). When this is added, protos
can
> > start having absolute paths as in C++ and this CL is added to check them.
> > 
> > I have copied the code from C++ dependency check and added some import tests
> to
> > ensure that sub-directories of current directory are included by default and
> > other cases follow DEPS include rules.
> > Please advise if any other path is more suitable.
> 
> 
> Just pinging after 24h.
> :)

Hi Brett,

Just a friendly ping on this.

[Ramin Halavati, 2017-02-02 06:00:54]

rhalavati@chromium.org changed reviewers:
+ dpranke@chromium.org

[Ramin Halavati, 2017-02-02 06:00:54]

dpranke@, could you please review this?

Until now there was a limitation in proto_library.gni and protoc_wrapper.py that
we could not pass more than one import directory to protoc compiler using them.
Therefore, all of the required imports of each proto file had to reside in its
own directory and sub-directories and it did not require dependency check.

But in this CL (https://codereview.chromium.org/2421333002/), I needed to import
a proto from another directory in my own proto file and to do so, I needed to
create a CL that adds an option to have extra import directories for proto files
(https://codereview.chromium.org/2646123002/). When this is added, protos can
start having absolute paths as in C++ and the current CL is added to check their
dependencies.

I have copied the code from C++ dependency check and added some import tests to
ensure that sub-directories of current directory are included by default and
other cases follow DEPS include rules.

There is also a second CL (https://codereview.chromium.org/2651553006/) to add
these changes to PRESUBMIT script.

[Dirk Pranke, 2017-02-07 02:50:15]

This looks mostly okay to me, but I defer to brettw@ here. He should be mostly
dug out by now, so, ping again :).

[brettw, 2017-02-07 05:07:06]

LGTM, sorry for the delay.

You'll want to roll buildtools deps into Chrome once this lands (don't forget or
somebody else can end up rolling your patch and sometimes there are surprising
bugs). You can TBR that patch.

[msramek, 2017-02-07 10:55:12]

Description was changed from

==========
Dependancy check for .proto files is added.

proto_checker is added and checkdeps is modified so that import lines in proto
files are checked for matching with rules stated in DEPS file.

BUG=684383
==========

to

==========
Dependancy check for .proto files is added.

proto_checker is added and checkdeps is modified so that import lines in proto
files are checked for matching with rules stated in DEPS file.

BUG=684383
R=brettw@chromium.org

Committed: 4ca3556cb7098968142bb18f9eb0d1ea40c327f0
==========

[msramek, 2017-02-07 10:55:14]

Committed patchset #2 (id:20001) manually as
4ca3556cb7098968142bb18f9eb0d1ea40c327f0 (presubmit successful).