Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1922363002:
Script to generate licenses for prebuilt WebRTC. (Closed)
|
Created:
4 years, 8 months ago by tkchin_webrtc Modified:
4 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc, kjellander_webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionScript for concatenating licenses.
BUG=webrtc:5737
Committed: https://crrev.com/0090b689bde6aa68aab2f7ea949b481675ed8963
Cr-Commit-Position: refs/heads/master@{#12551}
Patch Set 1 #
Total comments: 20
Patch Set 2 : CR comments #
Total comments: 2
Patch Set 3 : Refactor #
Messages
Total messages: 17 (8 generated)
Description was changed from ========== Script for concatenating licenses. BUG= ========== to ========== Script for concatenating licenses. BUG= ==========
tkchin@webrtc.org changed reviewers: + haysc@webrtc.org
kjellander@webrtc.org changed reviewers: + kjellander@webrtc.org
https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... File webrtc/build/ios/generate_licenses.py (right): https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:13: import sys sort this among the other standard library modules below. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:21: WEBRTC_LIBS = [ Ouch, you're signing up for painful maintenance here. I suggest you add a unit test that ensures these actually exists (which may be tricky to write). Otherwise this will break quickly when things are renamed/removed). Unfortunately that also won't protect against new targets being added. Maybe it's possible to do something with the output from ninja, like: ninja -C out/Default -t targets rule phony More info at https://ninja-build.org/manual.html It seems hard to filter properly though, since you don't want them all. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:86: path = os.path.dirname(os.path.realpath(sys.argv[0])) Define a constant like checkout_root instead (see https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/gyp_w...), and use that to concatenate paths from the root. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:127: style_tag = ''' <style> Use textwrap.dedent instead, like https://chromium.googlesource.com/external/webrtc/+/master/setup_links.py#321 https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:134: output_license_file.write('\n <title>Licenses</title>\n') Put this leading newline in the end of the style_tag string instead. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:138: output_license_file.write('<p>\n%s<br/></p>\n' % license_lib) newline in the beginning of the paragraph?
Description was changed from ========== Script for concatenating licenses. BUG= ========== to ========== Script for concatenating licenses. BUG=webrtc:5737 ==========
PTAL https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... File webrtc/build/ios/generate_licenses.py (right): https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:13: import sys On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > sort this among the other standard library modules below. linter told me to place this above https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:21: WEBRTC_LIBS = [ On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > Ouch, you're signing up for painful maintenance here. I suggest you add a unit > test that ensures these actually exists (which may be tricky to write). > Otherwise this will break quickly when things are renamed/removed). > Unfortunately that also won't protect against new targets being added. > > Maybe it's possible to do something with the output from ninja, like: > ninja -C out/Default -t targets rule phony > More info at https://ninja-build.org/manual.html > > It seems hard to filter properly though, since you don't want them all. > Yes, the goal was to cause breakage if license changes. But you are right that I don't really want to have to maintain this list. As a crude approximation, how about I grep webrtc/ for target_name: 'foo' and if the target exists in webrtc/ or talk/ we will assume it's a webrtc library covered under our license? gn actually fixes this for us I heard since it outputs dependencies. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:86: path = os.path.dirname(os.path.realpath(sys.argv[0])) On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > Define a constant like checkout_root instead (see > https://chromium.googlesource.com/external/webrtc/+/master/webrtc/build/gyp_w...), > and use that to concatenate paths from the root. Done. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:127: style_tag = ''' <style> On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > Use textwrap.dedent instead, like > https://chromium.googlesource.com/external/webrtc/+/master/setup_links.py#321 Done. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:134: output_license_file.write('\n <title>Licenses</title>\n') On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > Put this leading newline in the end of the style_tag string instead. Done. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:138: output_license_file.write('<p>\n%s<br/></p>\n' % license_lib) On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > newline in the beginning of the paragraph? Done.
lgtm but please consider addressing my comments. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... File webrtc/build/ios/generate_licenses.py (right): https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:13: import sys On 2016/04/27 21:43:28, tkchin_webrtc wrote: > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > sort this among the other standard library modules below. > > linter told me to place this above hmm, that never happened to me, odd. I guess we'll go with the linter then since I cannot explain it. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:21: WEBRTC_LIBS = [ On 2016/04/27 21:43:29, tkchin_webrtc wrote: > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > Ouch, you're signing up for painful maintenance here. I suggest you add a unit > > test that ensures these actually exists (which may be tricky to write). > > Otherwise this will break quickly when things are renamed/removed). > > Unfortunately that also won't protect against new targets being added. > > > > Maybe it's possible to do something with the output from ninja, like: > > ninja -C out/Default -t targets rule phony > > More info at https://ninja-build.org/manual.html > > > > It seems hard to filter properly though, since you don't want them all. > > > > Yes, the goal was to cause breakage if license changes. > But you are right that I don't really want to have to maintain this list. > As a crude approximation, how about I grep webrtc/ for target_name: 'foo' and if > the target exists in webrtc/ or talk/ we will assume it's a webrtc library > covered under our license? > > gn actually fixes this for us I heard since it outputs dependencies. That sounds great. I guess you'll get test and tools executables included in that list too, but that's handled when you check for "lib" and ".a" later on. So it should work as intended. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:138: output_license_file.write('<p>\n%s<br/></p>\n' % license_lib) On 2016/04/27 21:43:29, tkchin_webrtc wrote: > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > newline in the beginning of the paragraph? > > Done. I meant I was surprised to see the newline at the beginning of the contents of the paragraph. Does it even show up as anything? I'd expect just '<p>%s<br/></p>\n' to be better https://codereview.webrtc.org/1922363002/diff/20001/webrtc/build/ios/generate... File webrtc/build/ios/generate_licenses.py (right): https://codereview.webrtc.org/1922363002/diff/20001/webrtc/build/ios/generate... webrtc/build/ios/generate_licenses.py:36: SCRIPT_DIR = os.path.dirname(os.path.realpath(sys.argv[0])) I prefer __file__ instead of sys.argv[0] but I'm OK with this. https://codereview.webrtc.org/1922363002/diff/20001/webrtc/build/ios/generate... webrtc/build/ios/generate_licenses.py:42: webrtc_target_names = None I don't like the global variable here but I'll leave it up to you if you want to refactor this into some kind of LIcenseBuilder class instead, which has this as a class member and initializes it in the constructor.
Updated. Lmk if anything needs to change. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... File webrtc/build/ios/generate_licenses.py (right): https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:13: import sys On 2016/04/28 07:38:24, kjellander (webrtc) wrote: > On 2016/04/27 21:43:28, tkchin_webrtc wrote: > > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > > sort this among the other standard library modules below. > > > > linter told me to place this above > > hmm, that never happened to me, odd. I guess we'll go with the linter then since > I cannot explain it. using gpylint maybe that makes a difference https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:21: WEBRTC_LIBS = [ On 2016/04/28 07:38:24, kjellander (webrtc) wrote: > On 2016/04/27 21:43:29, tkchin_webrtc wrote: > > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > > Ouch, you're signing up for painful maintenance here. I suggest you add a > unit > > > test that ensures these actually exists (which may be tricky to write). > > > Otherwise this will break quickly when things are renamed/removed). > > > Unfortunately that also won't protect against new targets being added. > > > > > > Maybe it's possible to do something with the output from ninja, like: > > > ninja -C out/Default -t targets rule phony > > > More info at https://ninja-build.org/manual.html > > > > > > It seems hard to filter properly though, since you don't want them all. > > > > > > > Yes, the goal was to cause breakage if license changes. > > But you are right that I don't really want to have to maintain this list. > > As a crude approximation, how about I grep webrtc/ for target_name: 'foo' and > if > > the target exists in webrtc/ or talk/ we will assume it's a webrtc library > > covered under our license? > > > > gn actually fixes this for us I heard since it outputs dependencies. > > That sounds great. I guess you'll get test and tools executables included in > that list too, but that's handled when you check for "lib" and ".a" later on. So > it should work as intended. The tools won't actually be built since only dependencies for rtc_sdk_framework_objc will be built. https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:138: output_license_file.write('<p>\n%s<br/></p>\n' % license_lib) On 2016/04/28 07:38:24, kjellander (webrtc) wrote: > On 2016/04/27 21:43:29, tkchin_webrtc wrote: > > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > > newline in the beginning of the paragraph? > > > > Done. > > I meant I was surprised to see the newline at the beginning of the contents of > the paragraph. Does it even show up as anything? > I'd expect just > '<p>%s<br/></p>\n' > to be better Oh, nope, removed.
The CQ bit was checked by tkchin@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/1922363002/#ps40001 (title: "Refactor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922363002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922363002/40001
Message was sent while issue was closed.
Description was changed from ========== Script for concatenating licenses. BUG=webrtc:5737 ========== to ========== Script for concatenating licenses. BUG=webrtc:5737 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Nice, thanks for taking the extra time to fix the last comments :) https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... File webrtc/build/ios/generate_licenses.py (right): https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:13: import sys On 2016/04/28 22:38:10, tkchin_webrtc wrote: > On 2016/04/28 07:38:24, kjellander (webrtc) wrote: > > On 2016/04/27 21:43:28, tkchin_webrtc wrote: > > > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > > > sort this among the other standard library modules below. > > > > > > linter told me to place this above > > > > hmm, that never happened to me, odd. I guess we'll go with the linter then > since > > I cannot explain it. > > using gpylint maybe that makes a difference I dunno, it seems this passed git cl presubmit (what the presubmit bot ran, so it's fine). https://codereview.webrtc.org/1922363002/diff/1/webrtc/build/ios/generate_lic... webrtc/build/ios/generate_licenses.py:21: WEBRTC_LIBS = [ On 2016/04/28 22:38:10, tkchin_webrtc wrote: > On 2016/04/28 07:38:24, kjellander (webrtc) wrote: > > On 2016/04/27 21:43:29, tkchin_webrtc wrote: > > > On 2016/04/27 12:58:05, kjellander (webrtc) wrote: > > > > Ouch, you're signing up for painful maintenance here. I suggest you add a > > unit > > > > test that ensures these actually exists (which may be tricky to write). > > > > Otherwise this will break quickly when things are renamed/removed). > > > > Unfortunately that also won't protect against new targets being added. > > > > > > > > Maybe it's possible to do something with the output from ninja, like: > > > > ninja -C out/Default -t targets rule phony > > > > More info at https://ninja-build.org/manual.html > > > > > > > > It seems hard to filter properly though, since you don't want them all. > > > > > > > > > > Yes, the goal was to cause breakage if license changes. > > > But you are right that I don't really want to have to maintain this list. > > > As a crude approximation, how about I grep webrtc/ for target_name: 'foo' > and > > if > > > the target exists in webrtc/ or talk/ we will assume it's a webrtc library > > > covered under our license? > > > > > > gn actually fixes this for us I heard since it outputs dependencies. > > > > That sounds great. I guess you'll get test and tools executables included in > > that list too, but that's handled when you check for "lib" and ".a" later on. > So > > it should work as intended. > > The tools won't actually be built since only dependencies for > rtc_sdk_framework_objc will be built. Acknowledged.
Message was sent while issue was closed.
Description was changed from ========== Script for concatenating licenses. BUG=webrtc:5737 ========== to ========== Script for concatenating licenses. BUG=webrtc:5737 Committed: https://crrev.com/0090b689bde6aa68aab2f7ea949b481675ed8963 Cr-Commit-Position: refs/heads/master@{#12551} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/0090b689bde6aa68aab2f7ea949b481675ed8963 Cr-Commit-Position: refs/heads/master@{#12551} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
