Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2234113002:
Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc (Closed)
DescriptionInclude generated EffectiveConnectionType.java in cronet api jar and JavaDoc
This CL includes the EffectiveConnectionType.java in cronet
api jar and in JavaDoc, since that is a part of Cronet API.
BUG=634092
Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba
Committed: https://crrev.com/3d60df9ba5010441b73b5878ec3ff0fb9056a6a3
Cr-Original-Commit-Position: refs/heads/master@{#411977}
Cr-Commit-Position: refs/heads/master@{#412270}
Patch Set 1 : self review #
Total comments: 2
Patch Set 2 : address comments #
Total comments: 18
Patch Set 3 : address comments #Patch Set 4 : Address Andrei's comments #
Total comments: 2
Patch Set 5 : Address comments #Patch Set 6 : Ignore RequestFinishedListener.java #
Messages
Total messages: 36 (14 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== TEMP Source jar BUG= ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG= ==========
Description was changed from ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG= ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 ==========
Patchset #1 (id:20001) has been deleted
xunjieli@chromium.org changed reviewers: + mef@chromium.org
Hi Misha, This CL make cronet api source jar to include a generated Java file (which is introduced in https://codereview.chromium.org/2223773003/). Additionally, I changed the JavaDoc generation script to generate JavaDoc from Cronet api source jar (instead of from src/ directory). One benefit is that we can keep our API source jar and the JavaDoc in sync. PTAL. Thanks!
xunjieli@chromium.org changed reviewers: + kapishnikov@chromium.org
+ kapishnikov@ to the review as well. Andrei: Will this have a negative impact on GMSCore integration? The generated Java file is already in our .class jar, so I assume there shouldn't be any negative impact.
On 2016/08/11 17:30:28, xunjieli wrote: > + kapishnikov@ to the review as well. > Andrei: Will this have a negative impact on GMSCore integration? The generated > Java file is already in our .class jar, so I assume there shouldn't be any > negative impact. I don't think there will be any negative impact.
https://codereview.chromium.org/2234113002/diff/40001/components/cronet/tools... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/40001/components/cronet/tools... components/cronet/tools/jar_src.py:37: raise Exception It would be good to add the file name that does not exist to the exception message.
Thanks for the review. PTAL. https://codereview.chromium.org/2234113002/diff/40001/components/cronet/tools... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/40001/components/cronet/tools... components/cronet/tools/jar_src.py:37: raise Exception On 2016/08/11 17:59:22, kapishnikov wrote: > It would be good to add the file name that does not exist to the exception > message. Done.
Helen, the change looks great. See some small comments below. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/generate_javadoc.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:55: parser.add_option('--input-jar', help='Cronet api source jar') Should we rename the option to "--input-src-jar"? https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:63: build_utils.MakeDirectory(unzipped_jar_path) Should we make sure that 'unzipped_jar_path' directory does not exist before calling MakeDirectory since it throws an exception if the directory already exists? I see that the directory removed later in the script. Still, if the script fails in the middle, it will be necessary to remove the directory manually. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:36: jar_cmd = ['jar', 'xf', '../../' + jar] Can we use os.path.abspath here instead of "../../" to break the relative path dependency between "jar" & "unzipped_jar_path"? https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:58: build_utils.MakeDirectory(unzipped_jar_path) Similar to the previous comment, make sure that the directory does not exist. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:60: for gyp_list in options.src_jars: Rename "gyp_list" to "gn_list"? https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:61: jar_list.extend(build_utils.ParseGypList(gyp_list)) ParseGypList => ParseGnList?
https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/generate_javadoc.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:55: parser.add_option('--input-jar', help='Cronet api source jar') Are input-dir and input-jar mutuall exclusive? https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:62: unzipped_jar_path = os.path.abspath(os.path.join(options.output_dir, 'tmp')) maybe use tempfile.mkdtemp for temp directory?
Thanks for the review!PTAL https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/generate_javadoc.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:55: parser.add_option('--input-jar', help='Cronet api source jar') On 2016/08/12 15:33:39, kapishnikov wrote: > Should we rename the option to "--input-src-jar"? Done. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:55: parser.add_option('--input-jar', help='Cronet api source jar') On 2016/08/12 16:00:05, mef wrote: > Are input-dir and input-jar mutuall exclusive? Yes, they currently not the same. The input-dir is //components/cronet/src/, and input-src-jar is //out/Debug/cronet/cronet_api-src.jar https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:62: unzipped_jar_path = os.path.abspath(os.path.join(options.output_dir, 'tmp')) On 2016/08/12 16:00:05, mef wrote: > maybe use tempfile.mkdtemp for temp directory? Done. Great idea! https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/generate_javadoc.py:63: build_utils.MakeDirectory(unzipped_jar_path) On 2016/08/12 15:33:39, kapishnikov wrote: > Should we make sure that 'unzipped_jar_path' directory does not exist before > calling MakeDirectory since it throws an exception if the directory already > exists? I see that the directory removed later in the script. Still, if the > script fails in the middle, it will be necessary to remove the directory > manually. Acknowledged. I adopted Misha's suggestion to use tempfile.mkdtemp(). This problem will go away I think. Thanks for bringing it up. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:36: jar_cmd = ['jar', 'xf', '../../' + jar] On 2016/08/12 15:33:39, kapishnikov wrote: > Can we use os.path.abspath here instead of "../../" to break the relative path > dependency between "jar" & "unzipped_jar_path"? We need to execute the jar command inside the temporary directory, so that all unzipped jars' java files can be in one place. I am not sure if there is a way to do it differently. out/Debug/cronet/1.jar out/Debug/cronet/2.jar cd out/Debug/cronet/tmpdir/; jar xf ../1.jar; jar xf ../2.jar All sources will now be found in out/Debug/cronet/tmpdir/ https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:58: build_utils.MakeDirectory(unzipped_jar_path) On 2016/08/12 15:33:39, kapishnikov wrote: > Similar to the previous comment, make sure that the directory does not exist. Done. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:60: for gyp_list in options.src_jars: On 2016/08/12 15:33:39, kapishnikov wrote: > Rename "gyp_list" to "gn_list"? Done. https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:61: jar_list.extend(build_utils.ParseGypList(gyp_list)) On 2016/08/12 15:33:39, kapishnikov wrote: > ParseGypList => ParseGnList? Done.
https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:36: jar_cmd = ['jar', 'xf', '../../' + jar] On 2016/08/12 17:52:58, xunjieli wrote: > On 2016/08/12 15:33:39, kapishnikov wrote: > > Can we use os.path.abspath here instead of "../../" to break the relative path > > dependency between "jar" & "unzipped_jar_path"? > > We need to execute the jar command inside the temporary directory, so that all > unzipped jars' java files can be in one place. > I am not sure if there is a way to do it differently. > > out/Debug/cronet/1.jar > out/Debug/cronet/2.jar > cd out/Debug/cronet/tmpdir/; jar xf ../1.jar; jar xf ../2.jar > All sources will now be found in out/Debug/cronet/tmpdir/ I was thinking of something like: jar_cmd = ['jar', 'xf', os.path.abspath(jar)] build_utils.CheckOutput(jar_cmd, cwd=unzipped_jar_path)
https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/60001/components/cronet/tools... components/cronet/tools/jar_src.py:36: jar_cmd = ['jar', 'xf', '../../' + jar] On 2016/08/12 19:18:51, kapishnikov wrote: > On 2016/08/12 17:52:58, xunjieli wrote: > > On 2016/08/12 15:33:39, kapishnikov wrote: > > > Can we use os.path.abspath here instead of "../../" to break the relative > path > > > dependency between "jar" & "unzipped_jar_path"? > > > > We need to execute the jar command inside the temporary directory, so that all > > unzipped jars' java files can be in one place. > > I am not sure if there is a way to do it differently. > > > > out/Debug/cronet/1.jar > > out/Debug/cronet/2.jar > > cd out/Debug/cronet/tmpdir/; jar xf ../1.jar; jar xf ../2.jar > > All sources will now be found in out/Debug/cronet/tmpdir/ > > I was thinking of something like: > jar_cmd = ['jar', 'xf', os.path.abspath(jar)] > build_utils.CheckOutput(jar_cmd, cwd=unzipped_jar_path) Ah, that's very smart. Why I didn't think of that. Thanks for the suggestion! Done.
LGTM https://codereview.chromium.org/2234113002/diff/100001/components/cronet/tool... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/100001/components/cronet/tool... components/cronet/tools/jar_src.py:35: jar_cmd = ['jar', 'xf', jar] Nit: Suggestion: In order to allow the caller to pass the relative jar path to the method, we should call os.path.abspath(jar) inside the method rather than outside. Thus, both relative and absolute paths will work.
Thanks! https://codereview.chromium.org/2234113002/diff/100001/components/cronet/tool... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2234113002/diff/100001/components/cronet/tool... components/cronet/tools/jar_src.py:35: jar_cmd = ['jar', 'xf', jar] On 2016/08/12 20:11:48, kapishnikov wrote: > Nit: Suggestion: In order to allow the caller to pass the relative jar path to > the method, we should call os.path.abspath(jar) inside the method rather than > outside. Thus, both relative and absolute paths will work. Done. Great idea!
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kapishnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2234113002/#ps120001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:120001) has been created in https://codereview.chromium.org/2242243002/ by bsep@chromium.org. The reason for reverting is: compile failed on Android Arm64: FAILED: gen/components/cronet/android/generate_javadoc.d python ../../components/cronet/tools/generate_javadoc.py --output-dir cronet --input-dir ../../components/cronet --overview-file cronet/README.md.html --readme-file ../../components/cronet/README.md --depfile gen/components/cronet/android/generate_javadoc.d --lib-java-dir lib.java/components/cronet/android --input-src-jar cronet/cronet_api-src.jar Traceback (most recent call last): File "../../components/cronet/tools/generate_javadoc.py", line 85, in <module> sys.exit(main()) File "../../components/cronet/tools/generate_javadoc.py", line 73, in main GenerateJavadoc(options, os.path.abspath(unzipped_jar_path)) File "../../components/cronet/tools/generate_javadoc.py", line 48, in GenerateJavadoc raise build_utils.CalledProcessError(working_dir, javadoc_cmd, stdout).
Message was sent while issue was closed.
Andrei, could you take a look at PS #6? The problem with the failed bot is that somehow its checkout is stale. It still has org/chromium/net/RequestFinishedListener.java in its source tree. That file was removed from the codebase recently. I reproduced the error seen on that bot by creating the same file and compile cronet_package. I think the simplest solution now is to ignore this file when generating Javadoc. What do you think?
Message was sent while issue was closed.
Description was changed from ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977} ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977} ==========
On 2016/08/16 14:36:13, xunjieli wrote: > Andrei, could you take a look at PS #6? > > The problem with the failed bot is that somehow its checkout is stale. It still > has org/chromium/net/RequestFinishedListener.java in its source tree. That file > was removed from the codebase recently. I reproduced the error seen on that bot > by creating the same file and compile cronet_package. > > I think the simplest solution now is to ignore this file when generating > Javadoc. What do you think? Is it possible to list all files that should be included rather than excluded? This solution is okay but let's keep the bug open until we can remove that file from the exclude list. LGTM.
On 2016/08/16 16:12:20, kapishnikov wrote: > On 2016/08/16 14:36:13, xunjieli wrote: > > Andrei, could you take a look at PS #6? > > > > The problem with the failed bot is that somehow its checkout is stale. It > still > > has org/chromium/net/RequestFinishedListener.java in its source tree. That > file > > was removed from the codebase recently. I reproduced the error seen on that > bot > > by creating the same file and compile cronet_package. > > > > I think the simplest solution now is to ignore this file when generating > > Javadoc. What do you think? > > Is it possible to list all files that should be included rather than excluded? Yes, we can do it in build.xml or modify the template in BUILD.gn to specify files to be included. > This solution is okay but let's keep the bug open until we can remove that file > from the exclude list. LGTM. SGTM. We can try and see if this solves the problem. We can do the include approach rather than exclude approach as a follow-up. Thanks.
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977} ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Cr-Commit-Position: refs/heads/master@{#411977} ========== to ========== Include generated EffectiveConnectionType.java in cronet api jar and JavaDoc This CL includes the EffectiveConnectionType.java in cronet api jar and in JavaDoc, since that is a part of Cronet API. BUG=634092 Committed: https://crrev.com/cfb2180def7b0e17bd4dce2bc8300fb4ea02e4ba Committed: https://crrev.com/3d60df9ba5010441b73b5878ec3ff0fb9056a6a3 Cr-Original-Commit-Position: refs/heads/master@{#411977} Cr-Commit-Position: refs/heads/master@{#412270} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3d60df9ba5010441b73b5878ec3ff0fb9056a6a3 Cr-Commit-Position: refs/heads/master@{#412270} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
