Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347233002:
Add src_files to src_jar GN template (Closed)
|
Created:
4 years, 3 months ago by kapishnikov Modified:
4 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for "src_files" and "srcjar_deps" to "src_jar" GN template
Added a mandatory parameter to src_jar gn template that explicitly lists
all files that should be jarred. The script validates that one and only
one file with a given name exists in all search directories.
Added support for optional srcjar_deps attribute that can be used to
list dependencies that generate srcjar files and should be included
in the resulting jar file.
Added optional attribute 'source_deps' that allows packaging sources
of the dependent targets.
BUG=647293, 637887
Committed: https://crrev.com/624c32473d6d601e104bca2d91d68a0fa0266c0e
Cr-Commit-Position: refs/heads/master@{#420140}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Comment fixes #Patch Set 3 : Deduplicated Cronet API source file lists #Patch Set 4 : Build base_java, net_java, url_java from srcjar. Add support for srsjar_deps to jar_src template. #
Total comments: 4
Patch Set 5 : Removed reference to "url" sources #Patch Set 6 : Small fixes #
Total comments: 2
Patch Set 7 : Added source_deps that packages sources from the deps .sources files. #
Total comments: 4
Patch Set 8 : Added 'deps' for every 'source_deps' to make sure the .sources file exists #Patch Set 9 : Parse '.source' files in Python + set inputs var #
Total comments: 2
Patch Set 10 : Comment fixes + rebase #
Messages
Total messages: 42 (19 generated)
kapishnikov@chromium.org changed reviewers: + pauljensen@chromium.org, xunjieli@chromium.org
https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:309: "api/src/org/chromium/net/UserAgent.java", can we get rid of these lists and simply add the right srcjar to the srcjar_deps list below? I don't really like the idea of duplicating the massive file lists; seems like a recipe for divergence. https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... components/cronet/tools/jar_src.py:79: #Initialize the map space between "#" and "I" https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... components/cronet/tools/jar_src.py:105: 'removes from the search path' % src_search_dir) removes->removed search path->list of directories to search
https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:283: java_files = [ How about move put this list as a variable? so we can reuse it as well in jar_cronet_api_source. This will help us to not make the two jars diverge. https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:904: src_files = [ I am a bit concerned about listing non-cronet java files explicitly. How do we ensure that this stay in sync? Normal Android bots only compile cronet instrumentation target, not :cronet_package. https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... components/cronet/tools/jar_src.py:79: #Initialize the map nit: leave a space after #.
https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:283: java_files = [ On 2016/09/16 18:39:49, xunjieli wrote: > How about move put this list as a variable? so we can reuse it as well in > jar_cronet_api_source. This will help us to not make the two jars diverge. Implemented Paul's suggestion to build cronet_api library from the source jar. https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:309: "api/src/org/chromium/net/UserAgent.java", On 2016/09/16 18:38:39, pauljensen wrote: > can we get rid of these lists and simply add the right srcjar to the srcjar_deps > list below? I don't really like the idea of duplicating the massive file lists; > seems like a recipe for divergence. Done. https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:904: src_files = [ On 2016/09/16 18:39:49, xunjieli wrote: > I am a bit concerned about listing non-cronet java files explicitly. How do we > ensure that this stay in sync? Normal Android bots only compile cronet > instrumentation target, not :cronet_package. Good point. If we want to keep cronet-src.jar file, I think we should build individual srcjars in their corresponding packages, i.e. base, net & url. The cronet BUILD.gn should dependent on them. https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... File components/cronet/tools/jar_src.py (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... components/cronet/tools/jar_src.py:79: #Initialize the map On 2016/09/16 18:38:39, pauljensen wrote: > space between "#" and "I" Done. https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... components/cronet/tools/jar_src.py:79: #Initialize the map On 2016/09/16 18:39:49, xunjieli wrote: > nit: leave a space after #. Done. https://codereview.chromium.org/2347233002/diff/1/components/cronet/tools/jar... components/cronet/tools/jar_src.py:105: 'removes from the search path' % src_search_dir) On 2016/09/16 18:38:39, pauljensen wrote: > removes->removed > search path->list of directories to search Done.
One comment below. The rest l-g-t-m. https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... components/cronet/android/BUILD.gn:904: src_files = [ On 2016/09/16 20:28:27, kapishnikov wrote: > On 2016/09/16 18:39:49, xunjieli wrote: > > I am a bit concerned about listing non-cronet java files explicitly. How do we > > ensure that this stay in sync? Normal Android bots only compile cronet > > instrumentation target, not :cronet_package. > > Good point. If we want to keep cronet-src.jar file, I think we should build > individual srcjars in their corresponding packages, i.e. base, net & url. The > cronet BUILD.gn should dependent on them. If we can get rid of cronet-src.jar, then it will be great. I think developers are only interested in cronet_api-src.jar. Might be worth checking in with Misha whether we can get rid of this.
Description was changed from
==========
Add src_files to src_jar GN template
Added a mandatory parameter to src_jar gn template that explicitly lists
all files that should be jarred. The script validates that only one file
with a given name exists in all search directories.
Explicitly listed source files for jar_cronet_api_source,
jar_cronet_sample_source, jar_cronet_other_source targets.
BUG=647293
==========
to
==========
Add support for "src_files" and "srcjar_deps" to "src_jar" GN template
Added a mandatory parameter to src_jar gn template that explicitly lists
all files that should be jarred. The script validates that one and only
one file with a given name exists in all search directories.
Added support for optional srcjar_deps attribute that can be used to
list dependencies that generate srcjar files and should be included
in the resulting jar file.
To avoid duplicate lists of "base_java", "net_java" and "url_java"
source files, the CL creates srcjar files and uses them as the
dependencies ("srcjar_deps") in the corresponding targets and externally
from Cronet.
BUG=647293,637887
==========
The CQ bit was checked by kapishnikov@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...
On 2016/09/16 20:42:24, xunjieli wrote: > One comment below. The rest l-g-t-m. > > https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... > File components/cronet/android/BUILD.gn (right): > > https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... > components/cronet/android/BUILD.gn:904: src_files = [ > On 2016/09/16 20:28:27, kapishnikov wrote: > > On 2016/09/16 18:39:49, xunjieli wrote: > > > I am a bit concerned about listing non-cronet java files explicitly. How do > we > > > ensure that this stay in sync? Normal Android bots only compile cronet > > > instrumentation target, not :cronet_package. > > > > Good point. If we want to keep cronet-src.jar file, I think we should build > > individual srcjars in their corresponding packages, i.e. base, net & url. The > > cronet BUILD.gn should dependent on them. > > If we can get rid of cronet-src.jar, then it will be great. I think developers > are only interested in cronet_api-src.jar. Might be worth checking in with Misha > whether we can get rid of this. Talked to Misha. All libraries uploaded to google3 should be accompanied by sources. I have added support for "srcjar_deps" to the "jar_src" template, so that the base, net and url sources can be listed in one place and referenced by the corresponding "android_library" and Cronet.
Neat! LGTM
On 2016/09/19 19:33:01, kapishnikov wrote: > On 2016/09/16 20:42:24, xunjieli wrote: > > One comment below. The rest l-g-t-m. > > > > > https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... > > File components/cronet/android/BUILD.gn (right): > > > > > https://codereview.chromium.org/2347233002/diff/1/components/cronet/android/B... > > components/cronet/android/BUILD.gn:904: src_files = [ > > On 2016/09/16 20:28:27, kapishnikov wrote: > > > On 2016/09/16 18:39:49, xunjieli wrote: > > > > I am a bit concerned about listing non-cronet java files explicitly. How > do > > we > > > > ensure that this stay in sync? Normal Android bots only compile cronet > > > > instrumentation target, not :cronet_package. > > > > > > Good point. If we want to keep cronet-src.jar file, I think we should build > > > individual srcjars in their corresponding packages, i.e. base, net & url. > The > > > cronet BUILD.gn should dependent on them. > > > > If we can get rid of cronet-src.jar, then it will be great. I think developers > > are only interested in cronet_api-src.jar. Might be worth checking in with > Misha > > whether we can get rid of this. > > Talked to Misha. All libraries uploaded to google3 should be accompanied by > sources. > I have added support for "srcjar_deps" to the "jar_src" template, so that the > base, net and url > sources can be listed in one place and referenced by the corresponding > "android_library" and > Cronet. I think the better solution would be if the "android_library" rule had a parameter that indicated whether the srcjar file should be created as well along with the binary.
Forgot to send these out. I think we can consult Android and GN folks and see which approach is better. I am fine with the current approach. https://codereview.chromium.org/2347233002/diff/60001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/60001/components/cronet/andro... components/cronet/android/BUILD.gn:893: "//url/android/java/src", no longer needed? https://codereview.chromium.org/2347233002/diff/60001/components/cronet/andro... components/cronet/android/BUILD.gn:913: "org/chromium/url/IDNStringUtil.java", no longer needed?
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
https://codereview.chromium.org/2347233002/diff/60001/components/cronet/andro... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/60001/components/cronet/andro... components/cronet/android/BUILD.gn:893: "//url/android/java/src", On 2016/09/19 19:43:53, xunjieli wrote: > no longer needed? Good catch. Removed. https://codereview.chromium.org/2347233002/diff/60001/components/cronet/andro... components/cronet/android/BUILD.gn:913: "org/chromium/url/IDNStringUtil.java", On 2016/09/19 19:43:53, xunjieli wrote: > no longer needed? Removed.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kapishnikov@chromium.org changed reviewers: + abarth@chromium.org, rmcilroy@chromium.org
kapishnikov@chromium.org changed reviewers: + abarth@google.com, agrieve@chromium.org - abarth@chromium.org, rmcilroy@chromium.org
The CQ bit was checked by kapishnikov@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.
https://codereview.chromium.org/2347233002/diff/100001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/100001/base/BUILD.gn#newcode2256 base/BUILD.gn:2256: ":base_java_srcjar", Putting these files into a srcjar will make them uneditable in Android Studio (https://chromium.googlesource.com/chromium/src.git/+/master/docs/android_stud...) I think it's also the case that you don't really need these to be zipped, you just need the list of sources. If this is true, I'd recommend using the .sources file that's written as a part of gn gen here: https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?...
https://codereview.chromium.org/2347233002/diff/100001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/100001/base/BUILD.gn#newcode2256 base/BUILD.gn:2256: ":base_java_srcjar", On 2016/09/20 00:09:41, agrieve wrote: > Putting these files into a srcjar will make them uneditable in Android Studio > (https://chromium.googlesource.com/chromium/src.git/+/master/docs/android_stud...) > > I think it's also the case that you don't really need these to be zipped, you > just need the list of sources. If this is true, I'd recommend using the .sources > file that's written as a part of gn gen here: > https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?... > That is a very good point. Thanks for the idea of using ".sources" file. I will change it.
Description was changed from
==========
Add support for "src_files" and "srcjar_deps" to "src_jar" GN template
Added a mandatory parameter to src_jar gn template that explicitly lists
all files that should be jarred. The script validates that one and only
one file with a given name exists in all search directories.
Added support for optional srcjar_deps attribute that can be used to
list dependencies that generate srcjar files and should be included
in the resulting jar file.
To avoid duplicate lists of "base_java", "net_java" and "url_java"
source files, the CL creates srcjar files and uses them as the
dependencies ("srcjar_deps") in the corresponding targets and externally
from Cronet.
BUG=647293,637887
==========
to
==========
Add support for "src_files" and "srcjar_deps" to "src_jar" GN template
Added a mandatory parameter to src_jar gn template that explicitly lists
all files that should be jarred. The script validates that one and only
one file with a given name exists in all search directories.
Added support for optional srcjar_deps attribute that can be used to
list dependencies that generate srcjar files and should be included
in the resulting jar file.
Added optional attribute 'source_deps' that allows packaging sources
of the dependent targets.
BUG=647293,637887
==========
On 2016/09/20 14:59:31, kapishnikov wrote: > https://codereview.chromium.org/2347233002/diff/100001/base/BUILD.gn > File base/BUILD.gn (right): > > https://codereview.chromium.org/2347233002/diff/100001/base/BUILD.gn#newcode2256 > base/BUILD.gn:2256: ":base_java_srcjar", > On 2016/09/20 00:09:41, agrieve wrote: > > Putting these files into a srcjar will make them uneditable in Android Studio > > > (https://chromium.googlesource.com/chromium/src.git/+/master/docs/android_stud...) > > > > I think it's also the case that you don't really need these to be zipped, you > > just need the list of sources. If this is true, I'd recommend using the > .sources > > file that's written as a part of gn gen here: > > > https://cs.chromium.org/chromium/src/build/config/android/internal_rules.gni?... > > > > That is a very good point. Thanks for the idea of using ".sources" file. I will > change it. Implemented Andrew's suggestion to automatically retrieve sources from dependencies by reading the corresponding ".sources" files. No changes are needed in "base', "url" & "net".
kapishnikov@chromium.org changed reviewers: - abarth@google.com, agrieve@chromium.org
agrieve@chromium.org changed reviewers: + agrieve@chromium.org
https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... components/cronet/android/BUILD.gn:868: _source_file = "$_dep_gen_dir/$_dep_name.sources" You should also add these files to the action's inputs so that ninja will know to rebuild this target if they change. You should do this for _src_files and _src_jars https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... components/cronet/android/BUILD.gn:869: _sources = rebase_path(read_file(_source_file, "list lines"), The bot is failing because GN makes no ordering guarantees about parsing .gn files. To make this work, you'll need to read the .sources file at build time (via the python script)
On 2016/09/21 00:33:45, agrieve wrote: > https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... > File components/cronet/android/BUILD.gn (right): > > https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... > components/cronet/android/BUILD.gn:868: _source_file = > "$_dep_gen_dir/$_dep_name.sources" > You should also add these files to the action's inputs so that ninja will know > to rebuild this target if they change. You should do this for _src_files and > _src_jars > > https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... > components/cronet/android/BUILD.gn:869: _sources = > rebase_path(read_file(_source_file, "list lines"), > The bot is failing because GN makes no ordering guarantees about parsing .gn > files. To make this work, you'll need to read the .sources file at build time > (via the python script) Andrew, thanks for the explanation. It makes perfect sense now :)
https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... components/cronet/android/BUILD.gn:868: _source_file = "$_dep_gen_dir/$_dep_name.sources" On 2016/09/21 00:33:45, agrieve wrote: > You should also add these files to the action's inputs so that ninja will know > to rebuild this target if they change. You should do this for _src_files and > _src_jars Done. https://codereview.chromium.org/2347233002/diff/120001/components/cronet/andr... components/cronet/android/BUILD.gn:869: _sources = rebase_path(read_file(_source_file, "list lines"), On 2016/09/21 00:33:45, agrieve wrote: > The bot is failing because GN makes no ordering guarantees about parsing .gn > files. To make this work, you'll need to read the .sources file at build time > (via the python script) Done. Moved the ".sources" parsing to the python script.
lgtm :) https://codereview.chromium.org/2347233002/diff/160001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/160001/components/cronet/andr... components/cronet/android/BUILD.gn:843: # Add src jars that are implicitly listed through "srcjar_deps". nit: implicitly -> explicitly?
https://codereview.chromium.org/2347233002/diff/160001/components/cronet/andr... File components/cronet/android/BUILD.gn (right): https://codereview.chromium.org/2347233002/diff/160001/components/cronet/andr... components/cronet/android/BUILD.gn:843: # Add src jars that are implicitly listed through "srcjar_deps". On 2016/09/21 18:36:37, agrieve wrote: > nit: implicitly -> explicitly? Done. Changed to: # Add src-jar files that are generated by dependencies in "srcjar_deps".
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org, agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2347233002/#ps180001 (title: "Comment fixes + rebase")
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 ========== Add support for "src_files" and "srcjar_deps" to "src_jar" GN template Added a mandatory parameter to src_jar gn template that explicitly lists all files that should be jarred. The script validates that one and only one file with a given name exists in all search directories. Added support for optional srcjar_deps attribute that can be used to list dependencies that generate srcjar files and should be included in the resulting jar file. Added optional attribute 'source_deps' that allows packaging sources of the dependent targets. BUG=647293,637887 ========== to ========== Add support for "src_files" and "srcjar_deps" to "src_jar" GN template Added a mandatory parameter to src_jar gn template that explicitly lists all files that should be jarred. The script validates that one and only one file with a given name exists in all search directories. Added support for optional srcjar_deps attribute that can be used to list dependencies that generate srcjar files and should be included in the resulting jar file. Added optional attribute 'source_deps' that allows packaging sources of the dependent targets. BUG=647293,637887 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add support for "src_files" and "srcjar_deps" to "src_jar" GN template Added a mandatory parameter to src_jar gn template that explicitly lists all files that should be jarred. The script validates that one and only one file with a given name exists in all search directories. Added support for optional srcjar_deps attribute that can be used to list dependencies that generate srcjar files and should be included in the resulting jar file. Added optional attribute 'source_deps' that allows packaging sources of the dependent targets. BUG=647293,637887 ========== to ========== Add support for "src_files" and "srcjar_deps" to "src_jar" GN template Added a mandatory parameter to src_jar gn template that explicitly lists all files that should be jarred. The script validates that one and only one file with a given name exists in all search directories. Added support for optional srcjar_deps attribute that can be used to list dependencies that generate srcjar files and should be included in the resulting jar file. Added optional attribute 'source_deps' that allows packaging sources of the dependent targets. BUG=647293,637887 Committed: https://crrev.com/624c32473d6d601e104bca2d91d68a0fa0266c0e Cr-Commit-Position: refs/heads/master@{#420140} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/624c32473d6d601e104bca2d91d68a0fa0266c0e Cr-Commit-Position: refs/heads/master@{#420140} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
