Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670833004:
Use a depfile to know when to regenerate notice files (Closed)
DescriptionUse a depfile to know when to regenerate notice files
Cronet has a notice file generator as well, but I don't update it here
since it is very different from the main licenses.py.
BUG=687577
TBR=dpranke
Review-Url: https://codereview.chromium.org/2670833004
Cr-Commit-Position: refs/heads/master@{#450744}
Committed: https://chromium.googlesource.com/chromium/src/+/be96180ecd56757bf14524a75ac4ceaf469c030d
Patch Set 1 #
Total comments: 2
Messages
Total messages: 26 (9 generated)
agrieve@chromium.org changed reviewers: + blundell@chromium.org, dpranke@chromium.org, lambroslambrou@chromium.org, michaelbai@chromium.org
dpranke@chromium.org: Please review changes in tools/ blundell@chromium.org: Please review changes in components/resources/BUILD.gn michaelbai@chromium.org: Please review changes in android_webview lambroslambrou@chromium.org: Please review changes in remoting
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
remoting/ lgtm, but I have a slight concern about the "depfile" abbreviation. Style guide says don't abbreviate unless the meaning is unambiguous and clear. Is this term commonly used in Chromium? Am I correct that this is a "Dependency File", of the type that build systems will typically produce? If so, then variable naming should treat this as two words: "dep_file" or "depFile", and not "Depfile" or "depfile". Or write it in full: dependency_file, ...
On 2017/02/02 18:26:43, Lambros wrote: > remoting/ lgtm, but I have a slight concern about the "depfile" abbreviation. > Style guide says don't abbreviate unless the meaning is unambiguous and clear. > Is this term commonly used in Chromium? > > Am I correct that this is a "Dependency File", of the type that build systems > will typically produce? > > If so, then variable naming should treat this as two words: "dep_file" or > "depFile", and not "Depfile" or "depfile". > > Or write it in full: dependency_file, ... "depfile" is a gn/ninja concept. "gn help depfile" has information about it.
On 2017/02/02 18:29:36, agrieve wrote: > On 2017/02/02 18:26:43, Lambros wrote: > > remoting/ lgtm, but I have a slight concern about the "depfile" abbreviation. > > Style guide says don't abbreviate unless the meaning is unambiguous and clear. > > Is this term commonly used in Chromium? > > > > Am I correct that this is a "Dependency File", of the type that build systems > > will typically produce? > > > > If so, then variable naming should treat this as two words: "dep_file" or > > "depFile", and not "Depfile" or "depfile". > > > > Or write it in full: dependency_file, ... > > "depfile" is a gn/ninja concept. "gn help depfile" has information about it. Ah yes, I see that AddDepfileOption() refers to `gn help depfile`. So variable naming LGTM!
rubberstamp lgtm for //components, defer to Dirk for the technical details
webview LGTM
On 2017/02/07 01:40:42, michaelbai wrote: > webview LGTM ping dpranke
https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... File android_webview/tools/webview_licenses.py (right): https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... android_webview/tools/webview_licenses.py:291: I don't understand this block. Why do you want to re-run this whenever `gn gen` is run? What are you worried about becoming out of date?
https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... File android_webview/tools/webview_licenses.py (right): https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... android_webview/tools/webview_licenses.py:291: On 2017/02/10 00:15:59, Dirk Pranke wrote: > I don't understand this block. Why do you want to re-run this whenever `gn gen` > is run? What are you worried about becoming out of date? Before this change, it was scanning the filesystem during "gn gen", and that would cause it to tell ninja of any new LICENSE files that popped up. With this change, it's not scanning during gn gen, so any new third_party subdirectories that get pulled in will not be noticed by incremental builds. So, the assumption here is that a new third_party library being added will result in a "gn gen", which will in turn tell the license scraper to re-scan.
On 2017/02/10 00:32:35, agrieve (OOO Feb 10) wrote: > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > File android_webview/tools/webview_licenses.py (right): > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > android_webview/tools/webview_licenses.py:291: > On 2017/02/10 00:15:59, Dirk Pranke wrote: > > I don't understand this block. Why do you want to re-run this whenever `gn > gen` > > is run? What are you worried about becoming out of date? > > Before this change, it was scanning the filesystem during "gn gen", and that > would cause it to tell ninja of any new LICENSE files that popped up. > > With this change, it's not scanning during gn gen, so any new third_party > subdirectories that get pulled in will not be noticed by incremental builds. So, > the assumption here is that a new third_party library being added will result in > a "gn gen", which will in turn tell the license scraper to re-scan. Got it. What if we added a gclient hook to do the scan instead?
On 2017/02/10 00:46:40, Dirk Pranke wrote: > On 2017/02/10 00:32:35, agrieve (OOO Feb 10) wrote: > > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > > File android_webview/tools/webview_licenses.py (right): > > > > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > > android_webview/tools/webview_licenses.py:291: > > On 2017/02/10 00:15:59, Dirk Pranke wrote: > > > I don't understand this block. Why do you want to re-run this whenever `gn > > gen` > > > is run? What are you worried about becoming out of date? > > > > Before this change, it was scanning the filesystem during "gn gen", and that > > would cause it to tell ninja of any new LICENSE files that popped up. > > > > With this change, it's not scanning during gn gen, so any new third_party > > subdirectories that get pulled in will not be noticed by incremental builds. > So, > > the assumption here is that a new third_party library being added will result > in > > a "gn gen", which will in turn tell the license scraper to re-scan. > > Got it. > > What if we added a gclient hook to do the scan instead? gclient doesn't know where the output directories are though? We may also want to vary their creation based on GN args (e.g. cronet is disabled for is_component_build). If lambroslambrou is able to copy cronet's craftiness, where they only include LICENSE files reachable via gn deps, then that's another way in which gn args could vary it.
On 2017/02/10 01:12:12, agrieve (OOO Feb 10) wrote: > On 2017/02/10 00:46:40, Dirk Pranke wrote: > > On 2017/02/10 00:32:35, agrieve (OOO Feb 10) wrote: > > > > > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > > > File android_webview/tools/webview_licenses.py (right): > > > > > > > > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > > > android_webview/tools/webview_licenses.py:291: > > > On 2017/02/10 00:15:59, Dirk Pranke wrote: > > > > I don't understand this block. Why do you want to re-run this whenever `gn > > > gen` > > > > is run? What are you worried about becoming out of date? > > > > > > Before this change, it was scanning the filesystem during "gn gen", and that > > > would cause it to tell ninja of any new LICENSE files that popped up. > > > > > > With this change, it's not scanning during gn gen, so any new third_party > > > subdirectories that get pulled in will not be noticed by incremental builds. > > So, > > > the assumption here is that a new third_party library being added will > result > > in > > > a "gn gen", which will in turn tell the license scraper to re-scan. > > > > Got it. > > > > What if we added a gclient hook to do the scan instead? > > > gclient doesn't know where the output directories are though? We may also want > to vary their creation based on GN args (e.g. cronet is disabled for > is_component_build). If lambroslambrou is able to copy cronet's craftiness, > where they only include LICENSE files reachable via gn deps, then that's another > way in which gn args could vary it. Good point. Let me think about this more. There should be a way to make this be correct and reliable.
On 2017/02/10 01:16:07, Dirk Pranke wrote: > On 2017/02/10 01:12:12, agrieve (OOO Feb 10) wrote: > > On 2017/02/10 00:46:40, Dirk Pranke wrote: > > > On 2017/02/10 00:32:35, agrieve (OOO Feb 10) wrote: > > > > > > > > > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > > > > File android_webview/tools/webview_licenses.py (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2670833004/diff/1/android_webview/tools/webvi... > > > > android_webview/tools/webview_licenses.py:291: > > > > On 2017/02/10 00:15:59, Dirk Pranke wrote: > > > > > I don't understand this block. Why do you want to re-run this whenever > `gn > > > > gen` > > > > > is run? What are you worried about becoming out of date? > > > > > > > > Before this change, it was scanning the filesystem during "gn gen", and > that > > > > would cause it to tell ninja of any new LICENSE files that popped up. > > > > > > > > With this change, it's not scanning during gn gen, so any new third_party > > > > subdirectories that get pulled in will not be noticed by incremental > builds. > > > So, > > > > the assumption here is that a new third_party library being added will > > result > > > in > > > > a "gn gen", which will in turn tell the license scraper to re-scan. > > > > > > Got it. > > > > > > What if we added a gclient hook to do the scan instead? > > > > > > gclient doesn't know where the output directories are though? We may also want > > to vary their creation based on GN args (e.g. cronet is disabled for > > is_component_build). If lambroslambrou is able to copy cronet's craftiness, > > where they only include LICENSE files reachable via gn deps, then that's > another > > way in which gn args could vary it. > > Good point. Let me think about this more. There should be a way to make this be > correct > and reliable. Wonder if you've come up with anything yet? I'm hoping this change will eliminate the saw-tooth on the size graphs, so perhaps we could commit it while still thinking of a cleaner way?
On 2017/02/14 15:08:52, agrieve wrote: > Wonder if you've come up with anything yet? I'm hoping this change will > eliminate the saw-tooth on the size graphs, so perhaps we could commit it while > still thinking of a cleaner way? Sorry, I haven't had time to think of anything. Yes, feel free to commit as-is, but could you file a follow-up bug to see if we can make this more reliable?
On 2017/02/15 02:40:38, Dirk Pranke wrote: > On 2017/02/14 15:08:52, agrieve wrote: > > Wonder if you've come up with anything yet? I'm hoping this change will > > eliminate the saw-tooth on the size graphs, so perhaps we could commit it > while > > still thinking of a cleaner way? > > Sorry, I haven't had time to think of anything. Yes, feel free to commit as-is, > but could > you file a follow-up bug to see if we can make this more reliable? Filed https://bugs.chromium.org/p/chromium/issues/detail?id=692601
Description was changed from ========== Use a depfile to know when to regenerate notice files Cronet has a notice file generator as well, but I don't update it here since it is very different from the main licenses.py. BUG=687577 ========== to ========== Use a depfile to know when to regenerate notice files Cronet has a notice file generator as well, but I don't update it here since it is very different from the main licenses.py. BUG=687577 TBR=dpranke ==========
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1487177629819280, "parent_rev":
"4965734c3a73e6fc608071eba416e9af6b83555d", "commit_rev":
"be96180ecd56757bf14524a75ac4ceaf469c030d"}
Message was sent while issue was closed.
Description was changed from ========== Use a depfile to know when to regenerate notice files Cronet has a notice file generator as well, but I don't update it here since it is very different from the main licenses.py. BUG=687577 TBR=dpranke ========== to ========== Use a depfile to know when to regenerate notice files Cronet has a notice file generator as well, but I don't update it here since it is very different from the main licenses.py. BUG=687577 TBR=dpranke Review-Url: https://codereview.chromium.org/2670833004 Cr-Commit-Position: refs/heads/master@{#450744} Committed: https://chromium.googlesource.com/chromium/src/+/be96180ecd56757bf14524a75ac4... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/be96180ecd56757bf14524a75ac4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
