Closure compiler build rules and externs should not be in third_party |
||||
Issue descriptionCurrently third_party/closure_compiler contains: * Externs used to properly compile Chrome code * Build rules to trigger closure compilation This is inconsistent with how third_party is used throughout the rest of the Chrome codebase. Normally third_party is only for code that's not part of our project - the upstream code. This also generates confusing warnings every time you try to edit third_party code. I know there are other efforts, like to convert the build rules from GYP to GN, and to auto-generate externs, but there's no reason to block moving some files on that. I can help do the move, we just need to agree on where that code should live. Ideas: build/ - because the Closure compiler is similar to a build tool tools/ - because it's not required for the build but it's a manually-triggered tool, and also run as kind of a trybot warning chrome/ - because the externs depend on most of the codebase, and chrome/ is the main directory for things that depend on the whole project We could also optionally split things, like putting the scripts in tools/ and the externs in chrome/
,
Feb 28 2017
this has come up a bunch. i can turn off the warning if that's what's bothering you. I talked with Nico about it when I first hit it (years ago), and we just decided to basically ignore it. build/ might be OK, but only Android uses the output tools/ is fine because desktop (at least) uses closure mainly as a static analysis tool chrome/ is aight but ui/file_manager uses closure a bunch i think externs for extension APIs that only exist in chrome/ would be fine. but weirdly enough, ui/file_manager has a few things that it uses on the chrome.* namespace. i'd be happy to review changes but the current location doesn't bother me. if we moved to the nodejs-based compiler and started doing things on presubmit, it might change my POV on being third_party (right now it requires installing a totally different runtime, i.e. Java) also, you know we develop blink in third_party/, right? ;)
,
Feb 28 2017
I'm thinking: chrome/common/extensions/externs for the extension-related externs and tools/closure for everything else, including any other externs that aren't obviously associated with one extension api Sound good?
,
Feb 28 2017
that generally sounds OK to me. I don't know if there are any extension or app APIs that work outside of chrome and have externs, but there's probably some i don't know about. so, just to double-check, what would we leave in third_party/closure_compiler?
,
Feb 28 2017
I would leave actual third_party code and code to build, update, and run closure in third_party/closure_compiler What I'd move out are the top-level scripts to run closure on Chrome code and anything else related to other Chrome code like externs I'll have to look more carefully to decide for sure. I'll update when I have a proposed patch. I'm not going to get to this today. Assigning back to me.
,
Oct 18
Unassigning because clearly I haven't prioritized this. I think we fixed some of the issues with warnings and owners for externs, so it's not bothering anyone. I still think it'd be better to move, but not enough that I want to refactor thousands of files. |
||||
►
Sign in to add a comment |
||||
Comment 1 by dmazz...@chromium.org
, Feb 28 2017