Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3011613002:
License generation script for build_aar.py. (Closed)
DescriptionLicense generation script for build_aar.py.
The script is forked from: tools_webrtc/ios/generate_licenses.py
BUG=webrtc:8182
Review-Url: https://codereview.webrtc.org/3011613002
Cr-Commit-Position: refs/heads/master@{#19679}
Committed: https://chromium.googlesource.com/external/webrtc/+/67e414ce709446a3d6538bd9a4d027b698afa031
Patch Set 1 : License generation script for build_aar.py #
Total comments: 4
Patch Set 2 : Library & json #Patch Set 3 : Remove ow2_asm #Patch Set 4 : Similarity 10 #Patch Set 5 : Skip deps without license #
Total comments: 4
Patch Set 6 : Add unittest #
Total comments: 8
Patch Set 7 : Address comments. #Patch Set 8 : Fix generation when using --use-gomae. #Patch Set 9 : Specify GN working directory. #
Messages
Total messages: 43 (25 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== License generation script for build_aar.py BUG= ========== to ========== License generation script for build_aar.py. The script is forked from: tools_webrtc/ios/generate_licenses.py BUG=webrtc:8182 ==========
sakal@webrtc.org changed reviewers: + kjellander@webrtc.org, magjed@webrtc.org
PTAL
lgtm
How about avoiding code duplication by extracting the common code into its own Python module? I think it would be fine for iOS to generate MarkDown instead of HTML as well (but maybe you should check that first).
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
sakal@webrtc.org changed reviewers: + denicija@webrtc.org
I combined the scripts. denicija, could you verify this still works for iOS?
This starting to look great. Thanks for taking the time to do this properly. I think we would quickly have ended up with missed dependencies etc. I'm still wondering if there's some way to avoid hardcoding the deps in the script. Since many deps have different paths to their "license" file, we'll need a dict in some form, but I still think it would be nice to not include the Android-specific stuff in the iOS version etc. How about this: 1. $ gn gen out/NoTests --args=rtc_include_tests=false 2. $ gn desc out/NoTests --format=json *:* 3. Take all the stout output from 2 and load it into a json dict 4. Declare a deps Python set 5. Iterate over all target and put everything you find in the 'deps' attribute list into the set. 6. Iterate over this set and pick the licenses for the current platform (looking up the file in a map containing both Android+iOS mappings) The above should work if also target_os is added to the 'gn gen' command, since this script will execute on the right platform that can build Android/iOS (i.e. Linux vs macOS). https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/android/bui... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/android/bui... tools_webrtc/android/build_aar.py:173: subprocess.check_call(cmd) Ideally you'd load the common code as a Python module instead of invoking another subprocess. Isnt' that easier? https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/generate_li... File tools_webrtc/generate_licenses.py (right): https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/generate_li... tools_webrtc/generate_licenses.py:37: 'ow2_asm': ['third_party/ow2_asm/LICENSE'], AFAIK this is only used by tests, so it shouldn't really be included since this is only building production code, right? See https://cs.chromium.org/search/?q=ow2_asm+f:%5C.gn&type=cs
On 2017/08/31 09:45:21, kjellander_webrtc wrote: > This starting to look great. Thanks for taking the time to do this properly. I > think we would quickly have ended up with missed dependencies etc. > > I'm still wondering if there's some way to avoid hardcoding the deps in the > script. Since many deps have different paths to their "license" file, we'll need > a dict in some form, but I still think it would be nice to not include the > Android-specific stuff in the iOS version etc. > > How about this: > 1. $ gn gen out/NoTests --args=rtc_include_tests=false > 2. $ gn desc out/NoTests --format=json *:* > 3. Take all the stout output from 2 and load it into a json dict > 4. Declare a deps Python set > 5. Iterate over all target and put everything you find in the 'deps' attribute > list into the set. > 6. Iterate over this set and pick the licenses for the current platform (looking > up the file in a map containing both Android+iOS mappings) > > The above should work if also target_os is added to the 'gn gen' command, since > this script will execute on the right platform that can build Android/iOS (i.e. > Linux vs macOS). > > https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/android/bui... > File tools_webrtc/android/build_aar.py (right): > > https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/android/bui... > tools_webrtc/android/build_aar.py:173: subprocess.check_call(cmd) > Ideally you'd load the common code as a Python module instead of invoking > another subprocess. Isnt' that easier? > > https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/generate_li... > File tools_webrtc/generate_licenses.py (right): > > https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/generate_li... > tools_webrtc/generate_licenses.py:37: 'ow2_asm': > ['third_party/ow2_asm/LICENSE'], > AFAIK this is only used by tests, so it shouldn't really be included since this > is only building production code, right? > > See https://cs.chromium.org/search/?q=ow2_asm+f:%5C.gn&type=cs Yeah, parsing JSON is much nicer. PTAL
https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/android/bui... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/android/bui... tools_webrtc/android/build_aar.py:173: subprocess.check_call(cmd) On 2017/08/31 09:45:21, kjellander_webrtc wrote: > Ideally you'd load the common code as a Python module instead of invoking > another subprocess. Isnt' that easier? Done. https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/generate_li... File tools_webrtc/generate_licenses.py (right): https://codereview.webrtc.org/3011613002/diff/100001/tools_webrtc/generate_li... tools_webrtc/generate_licenses.py:37: 'ow2_asm': ['third_party/ow2_asm/LICENSE'], On 2017/08/31 09:45:21, kjellander_webrtc wrote: > AFAIK this is only used by tests, so it shouldn't really be included since this > is only building production code, right? > > See https://cs.chromium.org/search/?q=ow2_asm+f:%5C.gn&type=cs I modified build_aar.py to not include test targets and now it is no longer required.
Works perfectly on iOS. LGTM
Nice work! But... I think this CL is now a great improvement, but it also grows the complexity a lot so I think it calls for a few unit tests for LicenseBuilder class. My idea was that only the third party deps showing up for Android/iOS would magically show up in the license file using gn desc. Is that the case here? Can you provide sample generated files for these platforms? https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/android/bui... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/android/bui... tools_webrtc/android/build_aar.py:46: GENERATE_LICENSES_SCRIPT = os.path.join( You can remove this now as it's unused. https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/libs/genera... File tools_webrtc/libs/generate_licenses.py (right): https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses.py:97: if missing_licenses: What does this mean? Doesn't it mean that it will fail on missing licenses since iOS is unlikely to have android_tools show up as a dependency, right?
On 2017/09/01 08:44:52, kjellander_webrtc wrote: > Nice work! But... > I think this CL is now a great improvement, but it also grows the complexity a > lot so I think it calls for a few unit tests for LicenseBuilder class. > > My idea was that only the third party deps showing up for Android/iOS would > magically show up in the license file using gn desc. Is that the case here? Can > you provide sample generated files for these platforms? > > https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/android/bui... > File tools_webrtc/android/build_aar.py (right): > > https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/android/bui... > tools_webrtc/android/build_aar.py:46: GENERATE_LICENSES_SCRIPT = os.path.join( > You can remove this now as it's unused. > > https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/libs/genera... > File tools_webrtc/libs/generate_licenses.py (right): > > https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/libs/genera... > tools_webrtc/libs/generate_licenses.py:97: if missing_licenses: > What does this mean? Doesn't it mean that it will fail on missing licenses since > iOS is unlikely to have android_tools show up as a dependency, right? I wrote an unittest. I have never written a python unittest before so please comment if there are problems with it. The thirdy_party deps are looked up using gn desc. The dictionary is just for mapping these dependencies to their corresponding license files. The license looks like this for Android: https://gist.github.com/Scintillo/3789557c7c90d28cfe6d6887b3c0a97d
https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/android/bui... File tools_webrtc/android/build_aar.py (right): https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/android/bui... tools_webrtc/android/build_aar.py:46: GENERATE_LICENSES_SCRIPT = os.path.join( On 2017/09/01 08:44:52, kjellander_webrtc wrote: > You can remove this now as it's unused. Done. https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/libs/genera... File tools_webrtc/libs/generate_licenses.py (right): https://codereview.webrtc.org/3011613002/diff/180001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses.py:97: if missing_licenses: On 2017/09/01 08:44:52, kjellander_webrtc wrote: > What does this mean? Doesn't it mean that it will fail on missing licenses since > iOS is unlikely to have android_tools show up as a dependency, right? This exception is thrown if there are deps found in gn desc that don't have an entry in LIB_TO_LICENSES_DICT. (We don't know where their license file is so we can't continue.)
lgtm with nits addressed and my question answered :) https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... File tools_webrtc/libs/generate_licenses.py (right): https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses.py:65: //a/b/third_party/libname:c(d) what does (d) mean here? GN target names can only contain alphanumeric characters and underscores AFAIK. https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... File tools_webrtc/libs/generate_licenses_test.py (right): https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses_test.py:16: os.path.dirname((__file__)), os.pardir, os.pardir)) indent align with above ( https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses_test.py:20: import mock nit: sort alphabetically https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses_test.py:23: nit: 2 blank lines between each top-level statements https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines (we really need to get these things into PyLint, I thought we had it).
Patchset #7 (id:220001) has been deleted
https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... File tools_webrtc/libs/generate_licenses.py (right): https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses.py:65: //a/b/third_party/libname:c(d) On 2017/09/01 12:42:35, kjellander_webrtc wrote: > what does (d) mean here? GN target names can only contain alphanumeric > characters and underscores AFAIK. Sometimes toolchain is contained in parentheses for example: //third_party/protobuf:protoc(//build/toolchain/linux:clang_x64) Modified the comment / the unittest to make it more clear. https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... File tools_webrtc/libs/generate_licenses_test.py (right): https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses_test.py:16: os.path.dirname((__file__)), os.pardir, os.pardir)) On 2017/09/01 12:42:35, kjellander_webrtc wrote: > indent align with above ( Done. https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses_test.py:20: import mock On 2017/09/01 12:42:35, kjellander_webrtc wrote: > nit: sort alphabetically Done. https://codereview.webrtc.org/3011613002/diff/200001/tools_webrtc/libs/genera... tools_webrtc/libs/generate_licenses_test.py:23: On 2017/09/01 12:42:35, kjellander_webrtc wrote: > nit: 2 blank lines between each top-level statements > https://google.github.io/styleguide/pyguide.html?showone=Blank_Lines#Blank_Lines > (we really need to get these things into PyLint, I thought we had it). Done.
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, denicija@webrtc.org, kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/3011613002/#ps260001 (title: "Fix generation when using --use-gomae.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...)
On 2017/09/04 11:12:31, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > ios_api_framework on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) Hmm, I think this is a valid failure related to how gn.py is looking for GN in the tree based on cwd. Does the script work if you run it standing above the checkout root? I think that's what the bots have. If you can make a way to make that work (i.e. figure out the root of the checkout and set that as cwd for the subprocess call, it would probably be solved). Otherwise, worst case, we'll have to alter the cwd for the buildbot step being executed, but preferably we can avoid that.
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by sakal@webrtc.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.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org, kjellander@webrtc.org, denicija@webrtc.org Link to the patchset: https://codereview.webrtc.org/3011613002/#ps300001 (title: "Specify GN working directory.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1504595602934500,
"parent_rev": "31377a2a103f040c3c4f0b1947d69657233c21a2", "commit_rev":
"67e414ce709446a3d6538bd9a4d027b698afa031"}
Message was sent while issue was closed.
Description was changed from ========== License generation script for build_aar.py. The script is forked from: tools_webrtc/ios/generate_licenses.py BUG=webrtc:8182 ========== to ========== License generation script for build_aar.py. The script is forked from: tools_webrtc/ios/generate_licenses.py BUG=webrtc:8182 Review-Url: https://codereview.webrtc.org/3011613002 Cr-Commit-Position: refs/heads/master@{#19679} Committed: https://chromium.googlesource.com/external/webrtc/+/67e414ce709446a3d6538bd9a... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:300001) as https://chromium.googlesource.com/external/webrtc/+/67e414ce709446a3d6538bd9a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
