Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772213002:
Create doc on build arg best practices for Blimp. (Closed)
|
Created:
4 years, 9 months ago by Jess Modified:
4 years, 9 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate doc on build arg best practices for Blimp.
Currently this information is either not captured in docs or widely spread.
This document provides the key guidelines and best practices for creating and using a new Chrome build arg.
BUG=593073
Committed: https://crrev.com/4a8943a3f2bcb06115d9c82fd5158ccba07186a2
Cr-Commit-Position: refs/heads/master@{#380702}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address doc review recommendations pending larger structure discussion. #
Total comments: 12
Patch Set 3 : Restructure where new documentation lives. #Patch Set 4 : Separate arg creation from template value update. #
Total comments: 2
Patch Set 5 : Fix markdown syntax error. #
Messages
Total messages: 25 (11 generated)
Description was changed from ========== Add doc on how to add build args in Blimp. BUG= ========== to ========== Add doc on how to add build args in Blimp. BUG= ==========
jessicag@chromium.org changed reviewers: + dpranke@chromium.org, maniscalco@chromium.org, nyquist@chromium.org
jessicag@chromium.org changed required reviewers: + nyquist@chromium.org
jessicag@chromium.org changed reviewers: + marcinjb@chromium.org
Description was changed from ========== Add doc on how to add build args in Blimp. BUG= ========== to ========== Create doc on build arg best practices for Blimp. Currently this information is either not captured in docs or widely spread. This document provides the key guidelines and best practices for creating and using a new Chrome build arg. BUG=593073 ==========
https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md File blimp/docs/args.md (right): https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode1 blimp/docs/args.md:1: # Build Arguments Should this page be linked to from somewhere? https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode4 blimp/docs/args.md:4: can be passed via the command line or an args.gn file to control the build. Nit: could you do `args.gn`? https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode28 blimp/docs/args.md:28: String are typically used for filepaths. They are also used for enumerated Nit: Could you add ` around String to make it clear it's the data type String? Nit: "`String` is' or "`String`s are"? https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode72 blimp/docs/args.md:72: [build/args/blimp_client.gn](../../build/args/blimp_client.gn) and Could you make these paths surrounded by quotes? Like: [`build/args/...`](../../build/args/...) https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode73 blimp/docs/args.md:73: [build/args/blimp_engine.gn](../../build/args/blimp_engine.gn). Should we also describe here how to import them in args.gn, or maybe refer to build.md as a reference_
Most of this document is either repeating or clarifying or improving the generic GN docs in //tools/gn . Does it make sense to improve those docs instead and just limit this particular file to the blimp-specific stuff at the end?
On 2016/03/08 22:55:27, Dirk Pranke wrote: > Most of this document is either repeating or clarifying or improving the generic > GN docs in //tools/gn . Does it make sense to improve those docs instead and > just limit this particular file to the blimp-specific stuff at the end? This was definitely a thought that crossed my mind. My goal was collect this information around a specific use case I expect to repeatedly show up for Blimp. It pulls together ideas from several different docs to one place. I am not sure how useful that aspect is for a broader audience or how that organizing theme would work in the existing GN docs. The best practices section makes sense to expand in the GN docs, or I may have just missed where these guidelines lived there. I'd appreciate thoughts on how to best capture these and where they (should) live.
Resolving comments while discussing options on how this might be broken up with dpranke. https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md File blimp/docs/args.md (right): https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode1 blimp/docs/args.md:1: # Build Arguments On 2016/03/08 22:42:35, nyquist wrote: > Should this page be linked to from somewhere? Done. https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode4 blimp/docs/args.md:4: can be passed via the command line or an args.gn file to control the build. On 2016/03/08 22:42:35, nyquist wrote: > Nit: could you do `args.gn`? Done. https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode28 blimp/docs/args.md:28: String are typically used for filepaths. They are also used for enumerated On 2016/03/08 22:42:35, nyquist wrote: > Nit: Could you add ` around String to make it clear it's the data type String? > > Nit: "`String` is' or "`String`s are"? Done. https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode72 blimp/docs/args.md:72: [build/args/blimp_client.gn](../../build/args/blimp_client.gn) and On 2016/03/08 22:42:35, nyquist wrote: > Could you make these paths surrounded by quotes? > Like: [`build/args/...`](../../build/args/...) Done. https://codereview.chromium.org/1772213002/diff/1/blimp/docs/args.md#newcode73 blimp/docs/args.md:73: [build/args/blimp_engine.gn](../../build/args/blimp_engine.gn). On 2016/03/08 22:42:35, nyquist wrote: > Should we also describe here how to import them in args.gn, or maybe refer to > build.md as a reference_ Done.
https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md File blimp/docs/args.md (right): https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:6: This document addresses the creation of new build arguments for Blimp. See Honestly, I'm not sure that you should really have a document for creating new build arguments for Blimp specifically; people shouldn't really be doing this ... However, talking about when and how to define args is generically useful; you might want to move some of this text to either //tools/gn/docs/quick_start.md or .../style_guide.md , and link to those sections from here if possible. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:15: but the build team recommends this convention for all new arguments. I would probably drop lines 14 and 15. target_os isn't a good example of a historic feature, but I'm not sure what would be. I think most users (devs) don't need to worry about non-feature flags. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:29: options though integers and even sets of `boolean`s are often used instead. sets of booleans? https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:51: (e.g. `rtc_use_h264`, `v8_use_snapshot`) These three sections (Scope, Type, Naming conventions) might be good additions to the style guide (//tools/gn/docs/style_guide.md). https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:79: See [building](build.md) for using these in a developer GN args setup. If you were to get rid of this file, I would keep this last section and merge it into build.md. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/build.md File blimp/docs/build.md (right): https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/build.md#new... blimp/docs/build.md:6: [build arguments guide](args.md). I would try to distinguish between creating an entirely new flag (which we should be trying to avoid, but if we do need to do it, try to document it generically in the GN docs) and needing to set a particular flag to a particular value (which we do need), in which case you need to update the build template files in that case ...
jessicag@chromium.org changed reviewers: + brettw@chromium.org
jessicag@chromium.org changed required reviewers: + brettw@chromium.org
Agree with your restructuring proposal. PTAL at result. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md File blimp/docs/args.md (right): https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:6: This document addresses the creation of new build arguments for Blimp. See On 2016/03/09 01:28:45, Dirk Pranke wrote: > Honestly, I'm not sure that you should really have a document for creating new > build arguments for Blimp specifically; people shouldn't really be doing this > ... > > However, talking about when and how to define args is generically useful; you > might want to move some of this text to either //tools/gn/docs/quick_start.md or > .../style_guide.md , and link to those sections from here if possible. Done. Please let me know your thoughts on the restructing. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:15: but the build team recommends this convention for all new arguments. On 2016/03/09 01:28:45, Dirk Pranke wrote: > I would probably drop lines 14 and 15. target_os isn't a good example of a > historic feature, but I'm not sure what would be. I think most users (devs) > don't need to worry about non-feature flags. Done. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:29: options though integers and even sets of `boolean`s are often used instead. On 2016/03/09 01:28:45, Dirk Pranke wrote: > sets of booleans? Ugh. Bad writing on my part. There was a segment of code that used a string build argument as a switch statement for a series boolean defines. Is there a recommended build arg type for enumerations? That is actually what I'd like to capture. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:51: (e.g. `rtc_use_h264`, `v8_use_snapshot`) On 2016/03/09 01:28:45, Dirk Pranke wrote: > These three sections (Scope, Type, Naming conventions) might be good additions > to the style guide (//tools/gn/docs/style_guide.md). Done. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/args.md#newc... blimp/docs/args.md:79: See [building](build.md) for using these in a developer GN args setup. On 2016/03/09 01:28:45, Dirk Pranke wrote: > If you were to get rid of this file, I would keep this last section and merge it > into build.md. Done. https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/build.md File blimp/docs/build.md (right): https://codereview.chromium.org/1772213002/diff/20001/blimp/docs/build.md#new... blimp/docs/build.md:6: [build arguments guide](args.md). On 2016/03/09 01:28:45, Dirk Pranke wrote: > I would try to distinguish between creating an entirely new flag (which we > should be trying to avoid, but if we do need to do it, try to document it > generically in the GN docs) and needing to set a particular flag to a particular > value (which we do need), in which case you need to update the build template > files in that case ... Done.
Looks great, thanks for doing this! lgtm.
awesome! lgtm https://codereview.chromium.org/1772213002/diff/60001/blimp/docs/build.md File blimp/docs/build.md (right): https://codereview.chromium.org/1772213002/diff/60001/blimp/docs/build.md#new... blimp/docs/build.md:75: Build argument templates exist for the client and engine at Nit: missing empty line before this.
Thanks for the catch! https://codereview.chromium.org/1772213002/diff/60001/blimp/docs/build.md File blimp/docs/build.md (right): https://codereview.chromium.org/1772213002/diff/60001/blimp/docs/build.md#new... blimp/docs/build.md:75: Build argument templates exist for the client and engine at On 2016/03/10 00:32:47, nyquist wrote: > Nit: missing empty line before this. Done.
lgtm
The CQ bit was checked by jessicag@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1772213002/#ps80001 (title: "Fix markdown syntax error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772213002/80001
Message was sent while issue was closed.
Description was changed from ========== Create doc on build arg best practices for Blimp. Currently this information is either not captured in docs or widely spread. This document provides the key guidelines and best practices for creating and using a new Chrome build arg. BUG=593073 ========== to ========== Create doc on build arg best practices for Blimp. Currently this information is either not captured in docs or widely spread. This document provides the key guidelines and best practices for creating and using a new Chrome build arg. BUG=593073 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Create doc on build arg best practices for Blimp. Currently this information is either not captured in docs or widely spread. This document provides the key guidelines and best practices for creating and using a new Chrome build arg. BUG=593073 ========== to ========== Create doc on build arg best practices for Blimp. Currently this information is either not captured in docs or widely spread. This document provides the key guidelines and best practices for creating and using a new Chrome build arg. BUG=593073 Committed: https://crrev.com/4a8943a3f2bcb06115d9c82fd5158ccba07186a2 Cr-Commit-Position: refs/heads/master@{#380702} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4a8943a3f2bcb06115d9c82fd5158ccba07186a2 Cr-Commit-Position: refs/heads/master@{#380702} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
