Add presubmit check to //mojo/{public, edk} for BUILD.gn file flexibility.

Add presubmit check to //mojo/{public, edk} for BUILD.gn file flexibility.

This script checks for changes to the build files that would break the ability
to drop the mojo SDK EDK into a client repo at a location other than
//mojo/{public, edk} and have GN work:

- References to "//mojo/public" in the SDK (these should be relative paths).
- References to other absolute paths in the SDK (these shouldn't be there at
  all).
- References to "//mojo/public" and "//mojo/edk" in the EDK (these should be
  relative paths).
- Source set targets that are not constructed via the appropriate wrapper
  (mojo_sdk_source_set or mojo_edk_source_set targets as appropriate).

R=jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/3723fae44e0c178a53e86c772ecc029df371c17a

[blundell, 2014-12-05 15:42:54]

blundell@chromium.org changed reviewers:
+ jamesr@chromium.org

[blundell, 2014-12-05 15:42:55]

I won't land this until the repo's ready for it, of course. Do you have a
suggestion for where to place the functions defined in
//mojo/public/PRESUBMIT.py so that I can share (slightly generalized versions
of) them with //mojo/edk/PRESUBMIT.py?

[jamesr, 2014-12-05 21:06:44]

jamesr@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[jamesr, 2014-12-05 21:06:45]

I'd guess //mojo/public/tools/.  Trung might know how to organize public python
utility stuff in the SDK

[blundell, 2014-12-08 14:33:26]

Please hold off on reviewing this CL. I started working on the addition of
checks for the Mojo EDK, and I'm restructuring the code a bunch.

[blundell, 2014-12-08 15:21:20]

Patchset #2 (id:20001) has been deleted

[blundell, 2014-12-08 15:22:50]

CL ready for review. Added the EDK checks into this CL, as adding those checks
ended up in enough code restructuring that the SDK-only CL would have been kind
of a waste to review.

Trung, let me know if you have objections to the addition of
mojo_presubmit_checks.py in //mojo/public/tools.

[viettrungluu, 2014-12-08 21:42:19]

On 2014/12/08 15:22:50, blundell wrote:
> CL ready for review. Added the EDK checks into this CL, as adding those checks
> ended up in enough code restructuring that the SDK-only CL would have been
kind
> of a waste to review.
> 
> Trung, let me know if you have objections to the addition of
> mojo_presubmit_checks.py in //mojo/public/tools.

This is a bit weird, for several reasons:
* tools/ contains public stuff, whereas a presubmit check decidedly isn't.
* tools/ actually contains tools that you'd execute, whereas
mojo_presubmit_checks.py isn't something anyone would execute

The ambitious reorg would be to:
* move tools/bindings/{*.py,pylib,generators} to tools/ (and delete the bindings
subdirectory, and maybe rename "generators" to "bindings_generators")
* put mojo_presubmit_checks.py in a place like tools/pylib/internal/ or
something like that

[blundell, 2014-12-08 21:44:32]

On 2014/12/08 21:42:19, viettrungluu wrote:
> On 2014/12/08 15:22:50, blundell wrote:
> > CL ready for review. Added the EDK checks into this CL, as adding those
checks
> > ended up in enough code restructuring that the SDK-only CL would have been
> kind
> > of a waste to review.
> > 
> > Trung, let me know if you have objections to the addition of
> > mojo_presubmit_checks.py in //mojo/public/tools.
> 
> This is a bit weird, for several reasons:
> * tools/ contains public stuff, whereas a presubmit check decidedly isn't.
> * tools/ actually contains tools that you'd execute, whereas
> mojo_presubmit_checks.py isn't something anyone would execute
> 
> The ambitious reorg would be to:
> * move tools/bindings/{*.py,pylib,generators} to tools/ (and delete the
bindings
> subdirectory, and maybe rename "generators" to "bindings_generators")
> * put mojo_presubmit_checks.py in a place like tools/pylib/internal/ or
> something like that

[blundell, 2014-12-08 21:44:32]

On 2014/12/08 21:42:19, viettrungluu wrote:
> On 2014/12/08 15:22:50, blundell wrote:
> > CL ready for review. Added the EDK checks into this CL, as adding those
checks
> > ended up in enough code restructuring that the SDK-only CL would have been
> kind
> > of a waste to review.
> > 
> > Trung, let me know if you have objections to the addition of
> > mojo_presubmit_checks.py in //mojo/public/tools.
> 
> This is a bit weird, for several reasons:
> * tools/ contains public stuff, whereas a presubmit check decidedly isn't.
> * tools/ actually contains tools that you'd execute, whereas
> mojo_presubmit_checks.py isn't something anyone would execute
> 
> The ambitious reorg would be to:
> * move tools/bindings/{*.py,pylib,generators} to tools/ (and delete the
bindings
> subdirectory, and maybe rename "generators" to "bindings_generators")
> * put mojo_presubmit_checks.py in a place like tools/pylib/internal/ or
> something like that

[blundell, 2014-12-08 21:45:54]

On 2014/12/08 21:42:19, viettrungluu wrote:
> On 2014/12/08 15:22:50, blundell wrote:
> > CL ready for review. Added the EDK checks into this CL, as adding those
checks
> > ended up in enough code restructuring that the SDK-only CL would have been
> kind
> > of a waste to review.
> > 
> > Trung, let me know if you have objections to the addition of
> > mojo_presubmit_checks.py in //mojo/public/tools.
> 
> This is a bit weird, for several reasons:
> * tools/ contains public stuff, whereas a presubmit check decidedly isn't.
> * tools/ actually contains tools that you'd execute, whereas
> mojo_presubmit_checks.py isn't something anyone would execute
> 
> The ambitious reorg would be to:
> * move tools/bindings/{*.py,pylib,generators} to tools/ (and delete the
bindings
> subdirectory, and maybe rename "generators" to "bindings_generators")
> * put mojo_presubmit_checks.py in a place like tools/pylib/internal/ or
> something like that

Ugh, codereview tool went berzerk on me.

How is the first item here related to the second item (and to this CL)? Because
we'd end up with a common tools/pylib dir?

[jamesr, 2014-12-08 21:46:35]

How about just moving the PRESUBMIT.pys one level up (i.e. to //mojo) ?

[blundell, 2014-12-08 21:48:19]

On 2014/12/08 21:46:35, jamesr wrote:
> How about just moving the PRESUBMIT.pys one level up (i.e. to //mojo) ?

The thing that's unfortunate about that is that that PRESUBMIT.py will then have
to manually filter for the files to perform the various checks on.

[jamesr, 2014-12-08 21:51:28]

On 2014/12/08 21:48:19, blundell wrote:
> On 2014/12/08 21:46:35, jamesr wrote:
> > How about just moving the PRESUBMIT.pys one level up (i.e. to //mojo) ?
> 
> The thing that's unfortunate about that is that that PRESUBMIT.py will then
have
> to manually filter for the files to perform the various checks on.

That doesn't sound so bad.

[blundell, 2014-12-08 21:54:13]

On 2014/12/08 21:51:28, jamesr wrote:
> On 2014/12/08 21:48:19, blundell wrote:
> > On 2014/12/08 21:46:35, jamesr wrote:
> > > How about just moving the PRESUBMIT.pys one level up (i.e. to //mojo) ?
> > 
> > The thing that's unfortunate about that is that that PRESUBMIT.py will then
> have
> > to manually filter for the files to perform the various checks on.
> 
> That doesn't sound so bad.

Yeah it's obviously doable, just a little bit against the distributed nature of
how the presubmit tooling works (imo). I'm fine doing that if the conclusion is
that we'd like to avoid introducing this code into the public dirs.

[blundell, 2014-12-09 15:46:03]

Patchset #3 (id:60001) has been deleted

[blundell, 2014-12-09 15:49:55]

Hi James,

Consolidated all the checks in //mojo/PRESUBMIT.py. PTAL.

[viettrungluu, 2014-12-10 00:02:02]

Drive-by suggestions. (The change basically looks fine to me, but it occurs to
me that I should defer to jamesr....)

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode14
mojo/PRESUBMIT.py:14: SDK_WHITELISTED_EXTERNAL_PATHS = [
Maybe this should be _SDK_... (leading underscore).

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode22
mojo/PRESUBMIT.py:22: "EDK")."""
nit: I don't believe docstrings should be indented to align, according to any
style guide....

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode23
mojo/PRESUBMIT.py:23: if package == "SDK":
Suggestion:

Define a global:

_PACKAGE_PATH_PREFIXES = {"SDK": "mojo/public",
                          "EDK": "mojo/edk"}

Then this is just:

  assert package in _PACKAGE_PATH_PREFIXES
  package_path_prefix = _PACKAGE_PATH_PREFIXES["package"]

(the assertion is optional, since it'll blow up in a pretty obvious way on
failure)

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode30
mojo/PRESUBMIT.py:30: if not f.LocalPath().startswith(package_path_prefix):
Arguably, you should include a trailing '/' in the prefixes above (otherwise
mojo/edk_not_really would match).

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode32
mojo/PRESUBMIT.py:32: if (not f.LocalPath().endswith("BUILD.gn") and
This isn't quite right. <shrug>

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode47
mojo/PRESUBMIT.py:47: if not _IsBuildFileWithinPackage(f, package):
Maybe the "correct" thing to do is to define a helper:

def _AffectedBuildFilesWithinPackage(input_api, package):
  return [f for f in input_api.AffectedFiles() if _IsBuildFileWithinPackage(f,
package)]

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode95
mojo/PRESUBMIT.py:95: results += [output_api.PresubmitError(
results.append() instead?

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcod...
mojo/PRESUBMIT.py:102: results += [output_api.PresubmitError(
" (etc.)

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcod...
mojo/PRESUBMIT.py:118: if package == "SDK":
Similar suggestion as above.

[blundell, 2014-12-10 16:11:45]

Patchset #4 (id:100001) has been deleted

[blundell, 2014-12-10 16:13:06]

Thanks.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode14
mojo/PRESUBMIT.py:14: SDK_WHITELISTED_EXTERNAL_PATHS = [
On 2014/12/10 00:02:02, viettrungluu wrote:
> Maybe this should be _SDK_... (leading underscore).

Done.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode22
mojo/PRESUBMIT.py:22: "EDK")."""
On 2014/12/10 00:02:02, viettrungluu wrote:
> nit: I don't believe docstrings should be indented to align, according to any
> style guide....

Done.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode23
mojo/PRESUBMIT.py:23: if package == "SDK":
On 2014/12/10 00:02:02, viettrungluu wrote:
> Suggestion:
> 
> Define a global:
> 
> _PACKAGE_PATH_PREFIXES = {"SDK": "mojo/public",
>                           "EDK": "mojo/edk"}
> 
> Then this is just:
> 
>   assert package in _PACKAGE_PATH_PREFIXES
>   package_path_prefix = _PACKAGE_PATH_PREFIXES["package"]
> 
> (the assertion is optional, since it'll blow up in a pretty obvious way on
> failure)

Done.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode30
mojo/PRESUBMIT.py:30: if not f.LocalPath().startswith(package_path_prefix):
On 2014/12/10 00:02:02, viettrungluu wrote:
> Arguably, you should include a trailing '/' in the prefixes above (otherwise
> mojo/edk_not_really would match).

Done.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode32
mojo/PRESUBMIT.py:32: if (not f.LocalPath().endswith("BUILD.gn") and
On 2014/12/10 00:02:02, viettrungluu wrote:
> This isn't quite right. <shrug>

I assume you mean because it will return True for e.g. "notaBUILD.gn"? Changed
to "/BUILD.gn", since all the BUILD.gn files we're concerned with are under some
prefix.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode47
mojo/PRESUBMIT.py:47: if not _IsBuildFileWithinPackage(f, package):
On 2014/12/10 00:02:02, viettrungluu wrote:
> Maybe the "correct" thing to do is to define a helper:
> 
> def _AffectedBuildFilesWithinPackage(input_api, package):
>   return [f for f in input_api.AffectedFiles() if _IsBuildFileWithinPackage(f,
> package)]

Done.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcode95
mojo/PRESUBMIT.py:95: results += [output_api.PresubmitError(
On 2014/12/10 00:02:02, viettrungluu wrote:
> results.append() instead?

results.extend() seems to be the canonical form here from looking at other
PRESUBMIT.py files. Changed throughout this file.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcod...
mojo/PRESUBMIT.py:102: results += [output_api.PresubmitError(
On 2014/12/10 00:02:02, viettrungluu wrote:
> " (etc.)

Done.

https://codereview.chromium.org/782743002/diff/80001/mojo/PRESUBMIT.py#newcod...
mojo/PRESUBMIT.py:118: if package == "SDK":
On 2014/12/10 00:02:02, viettrungluu wrote:
> Similar suggestion as above.

Done.

[jamesr, 2014-12-10 20:34:43]

Seems ok but for something this complicated I think we should have some sanity
tests - see //PRESUBMIT_test.py

lgtm as far as I can tell

[blundell, 2014-12-11 20:46:57]

Patchset #5 (id:140001) has been deleted

[blundell, 2014-12-11 20:56:11]

Thanks for the pointer. Tests added - PTAL. All the changes to PRESUBMIT.py are
just to expose things for testing except for one bugfix that I noted.

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT.py
File mojo/PRESUBMIT.py (right):

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT.py#newco...
mojo/PRESUBMIT.py:90: if referenced_path in _SDK_WHITELISTED_EXTERNAL_PATHS:
This is a bugfix that my manual testing had missed :).

[jamesr, 2014-12-11 23:42:13]

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT_test.py
File mojo/PRESUBMIT_test.py (right):

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:10: sys.path.insert(0, "..")
pretty sure this is wrong - Trung, what's the right thing to do here?

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:14: _SDK_BUILD_FILE = 'mojo/public/some/path/BUILD.gn'
this file has a mix of single and double quotes - why?  pick one and be
consistent

[blundell, 2014-12-12 06:31:32]

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT_test.py
File mojo/PRESUBMIT_test.py (right):

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:10: sys.path.insert(0, "..")
On 2014/12/11 23:42:13, jamesr wrote:
> pretty sure this is wrong - Trung, what's the right thing to do here?

Oops. Switched to the way that Chromium PRESUBMIT_test.py files insert the path
to find the mocks.

https://codereview.chromium.org/782743002/diff/160001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:14: _SDK_BUILD_FILE = 'mojo/public/some/path/BUILD.gn'
On 2014/12/11 23:42:13, jamesr wrote:
> this file has a mix of single and double quotes - why?  pick one and be
> consistent

Done.

[blundell, 2014-12-12 17:18:29]

qsr@ is going to take charge of this while I'm out.

[jamesr, 2014-12-13 01:48:07]

https://codereview.chromium.org/782743002/diff/180001/mojo/PRESUBMIT_test.py
File mojo/PRESUBMIT_test.py (right):

https://codereview.chromium.org/782743002/diff/180001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:12:
sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
this is still modifying sys.path, which i don't think is right

[qsr, 2014-12-15 09:02:28]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2014-12-15 09:02:28]

https://codereview.chromium.org/782743002/diff/180001/mojo/PRESUBMIT_test.py
File mojo/PRESUBMIT_test.py (right):

https://codereview.chromium.org/782743002/diff/180001/mojo/PRESUBMIT_test.py#...
mojo/PRESUBMIT_test.py:12:
sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
On 2014/12/13 01:48:06, jamesr wrote:
> this is still modifying sys.path, which i don't think is right

 That's the way it is done in the chromium repo:

chrome/browser/resources/PRESUBMIT_test.py

 If you have another suggestion, I'll be happy to change this, but if we want to
use this mock file, and not have to pass a PYTHONPATH environment variable when
running this test, I don't see any other way than modifying sys.path

[jamesr, 2014-12-15 19:22:20]

Hmm ok,

still lgtm them

[qsr, 2014-12-16 09:27:50]

Committed patchset #6 (id:180001) manually as
3723fae44e0c178a53e86c772ecc029df371c17a (presubmit successful).