Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11415152:
build/java_cpp_template.gypi: new build rules. (Closed)
|
Created:
8 years ago by digit1 Modified:
8 years ago Reviewers:
Philippe, Ryan Sleevi, pasko-google - do not use, ppi, joth, Isaac (away), navabi1, Yaron, felipeg_google, bulach, willchan no longer on Chromium CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
Descriptionbuild/java_cpp_template.gypi: new build rules.
This patch adds a new .gypi file that allows one to generate Java source
files by parsing template files with the host C pre-processor. The main
use case is the ability to generate Java sources defining constants matching
their C/C++ counterparts.
This is actually a generalisation of the technique that was used in net/net.gyp
to generate a NetError.java source mirroring the definitions found in
net/base/net_error_list.h
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172041
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix indentation #Patch Set 3 : use <(DEPTH) #
Total comments: 6
Patch Set 4 : Move to build/android/ and remove un-necessary flags. #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Messages
Total messages: 21 (0 generated)
This feature was asked during the code review of https://chromiumcodereview.appspot.com/11266008/. The plan is to use it for certificate mime types, and probably a few other network-related constants in the future.
thanks digit! one suggestion below: https://chromiumcodereview.appspot.com/11415152/diff/1/build/java_cpp_templat... File build/java_cpp_template.gypi (right): https://chromiumcodereview.appspot.com/11415152/diff/1/build/java_cpp_templat... build/java_cpp_template.gypi:50: 'rules': [{ my understanding is that "rules" are used to map 1:1 a list of inputs to its output.. for instance, the jni_generator takes a list of .java, and then for each one, it generates a header file.. since this takes a pair of files (the template and the header), I think it's better suited as an "action" (see build/java.gypi versus build/jni_generator.gypi, and http://code.google.com/p/gyp/wiki/GypLanguageSpecification)
Thanks for this. LG from my side (but I'm not a gypi expert) https://chromiumcodereview.appspot.com/11415152/diff/1/net/android/java/NetEr... File net/android/java/NetError.template (right): https://chromiumcodereview.appspot.com/11415152/diff/1/net/android/java/NetEr... net/android/java/NetError.template:7: public class NetError { A follow up step maybe to have the new rule generate this file too? The only awkward/non-generic bit in here is the ERR_ prefixing. IMHO it's fine we check in distinct template files for now though, and see if clear pattern emerges.
https://chromiumcodereview.appspot.com/11415152/diff/1/build/java_cpp_templat... File build/java_cpp_template.gypi (right): https://chromiumcodereview.appspot.com/11415152/diff/1/build/java_cpp_templat... build/java_cpp_template.gypi:50: 'rules': [{ Hi Marcus, I think a 'rules' section is appropriate and more elegant here. Let me explain: Each rule inside a 'rules' directive only takes files from 'sources' filtered by their extension. This means that if you list both a header and a template, only the template (i.e. the file in sources that ends with '.template') will be considered for the generate_java_constants rule below. Also, 'rules' give you RULE_INPUT_ROOT, which makes the ClassName.template -> ClassName.java transformation trivial. You can't do it in an 'actions' block, so you end up having to hard-code both the template name and output file name in the caller (this is what the original net/net.gyp did). https://chromiumcodereview.appspot.com/11415152/diff/1/net/android/java/NetEr... File net/android/java/NetError.template (right): https://chromiumcodereview.appspot.com/11415152/diff/1/net/android/java/NetEr... net/android/java/NetError.template:7: public class NetError { I would say I'm wary about introducing yet-another-templating language/script to the build system. At least everybody understands the C pre-processor, and this patch is to unlock another one, so I'd prefer to keep changes to a minimum. On the other hand, I've written a C pre-processor in Python in the past, and we could probably reuse it to directly read native C headers and output just the right amount of information into Java sources, without having to use tricks like net_error_list.h. However, I'd prefer if this could be done in a separate patch.
lgtm, thanks for the explanation and sorry about the confusion! :) https://codereview.chromium.org/11415152/diff/1/build/java_cpp_template.gypi File build/java_cpp_template.gypi (right): https://codereview.chromium.org/11415152/diff/1/build/java_cpp_template.gypi#... build/java_cpp_template.gypi:50: 'rules': [{ On 2012/11/27 22:29:07, digit1 wrote: > Hi Marcus, I think a 'rules' section is appropriate and more elegant here. Let > me explain: > > Each rule inside a 'rules' directive only takes files from 'sources' filtered by > their extension. This means that if you list both a header and a template, only > the template (i.e. the file in sources that ends with '.template') will be > considered for the generate_java_constants rule below. > > Also, 'rules' give you RULE_INPUT_ROOT, which makes the ClassName.template -> > ClassName.java transformation trivial. You can't do it in an 'actions' block, so > you end up having to hard-code both the template name and output file name in > the caller (this is what the original net/net.gyp did). oh, you're right, thanks for the detailed explanation! I didn't know the "extension" was a filtering condition..
https://codereview.chromium.org/11415152/diff/1/build/java_cpp_template.gypi File build/java_cpp_template.gypi (right): https://codereview.chromium.org/11415152/diff/1/build/java_cpp_template.gypi#... build/java_cpp_template.gypi:69: '-I', '..', # Add project top-level to include path I wonder if we shouldn't use <(DEPTH) instead here. I'm not sure what this variable does, even after reading the Gyp documentation. Any opinion?
According to torne and joth, <(DEPTH) only needs to be used when the .gypi file can be included by more than one .gyp file. Currently, it is only included by net/net.gyp, and I plan a different patch to this .gyp file to generate another template, so this is not needed. I'll test <(DEPTH) though, just in case.
It turns out that using <(DEPTH) works very well, and is already heavily used in the Chromium .gyp/.gypi files, so I've added it. This should allow including from arbitrary directories easier, if the need arises to.
https://codereview.chromium.org/11415152/diff/6012/build/java_cpp_template.gypi File build/java_cpp_template.gypi (right): https://codereview.chromium.org/11415152/diff/6012/build/java_cpp_template.gy... build/java_cpp_template.gypi:7: # through the host C pre-processor. This should be in build/android, given that it depends on a particular compiler/toolchain (line 67-72) https://codereview.chromium.org/11415152/diff/6012/build/java_cpp_template.gy... build/java_cpp_template.gypi:75: 'process_outputs_as_sources': 1, Why 'process_outputs_as_sources', given that there is no .java rule here? Your canonical example in 13-24 indicates that this should be a whole target, but process_outputs_as_sources is only necessary if this .gypi will be included in non-"none" targets, right? https://codereview.chromium.org/11415152/diff/6012/build/java_cpp_template.gy... build/java_cpp_template.gypi:78: 'hard_dependency': 1, hard_dependency is only applicable to static_library, which I understand no java target will ever be, right? So why this directive?
https://chromiumcodereview.appspot.com/11415152/diff/6012/build/java_cpp_temp... File build/java_cpp_template.gypi (right): https://chromiumcodereview.appspot.com/11415152/diff/6012/build/java_cpp_temp... build/java_cpp_template.gypi:7: # through the host C pre-processor. On 2012/12/04 03:14:56, Ryan Sleevi wrote: > This should be in build/android, given that it depends on a particular > compiler/toolchain (line 67-72) I can move it there, but we already have plenty of other .gypi files under build/ that are in the same case, that's why I thought it was the natural location for this file. https://chromiumcodereview.appspot.com/11415152/diff/6012/build/java_cpp_temp... build/java_cpp_template.gypi:75: 'process_outputs_as_sources': 1, On 2012/12/04 03:14:56, Ryan Sleevi wrote: > Why 'process_outputs_as_sources', given that there is no .java rule here? > The NetError.template generates an output named NetError.java, I believe this is necessary to ensure that the latter is considered a proper source during the build. (Inspired from from build/jni_gnerator.gypi which does the reverse, i.e. generate C++ headers and sources from Java files). > Your canonical example in 13-24 indicates that this should be a whole target, > but process_outputs_as_sources is only necessary if this .gypi will be included > in non-"none" targets, right? I'm not too familiar with the terminology used here :) I essentially imitated what's in jni_generator.gypi. What would be your recommendation to fix this? https://chromiumcodereview.appspot.com/11415152/diff/6012/build/java_cpp_temp... build/java_cpp_template.gypi:78: 'hard_dependency': 1, On 2012/12/04 03:14:56, Ryan Sleevi wrote: > hard_dependency is only applicable to static_library, which I understand no java > target will ever be, right? > > So why this directive? It also came from jni_generator.gypi, I'll remove it.
Patch 4 moves the new file to build/android/ and also removes the hard_dependency and process_outputs_as_sources. I've checked that they are not necessary and that clean and incremental builds are fine.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/11415152/11001
Presubmit check for 11415152-11001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
net
Aarf, Ryan, does everything looks good to you here? Thanks in advance.
There's still a bug here, below; https://chromiumcodereview.appspot.com/11415152/diff/11001/build/android/java... File build/android/java_cpp_template.gypi (right): https://chromiumcodereview.appspot.com/11415152/diff/11001/build/android/java... build/android/java_cpp_template.gypi:62: }, This doesn't work - and if it does, it's a GYP bug, and you shouldn't depend on it. 'rules' don't have 'dependencies', so you cannot do this.
https://chromiumcodereview.appspot.com/11415152/diff/11001/build/android/java... File build/android/java_cpp_template.gypi (right): https://chromiumcodereview.appspot.com/11415152/diff/11001/build/android/java... build/android/java_cpp_template.gypi:62: }, I see. I've scanned all gyp files in the project and indeed this seems to be true. The 'additional_input_paths' was part of the original un-rules-using gyp fragment, but I've checked that it can be safely removed here (i.e. this doesn't affect clean or incremental builds). Fixed in the last patch. Thanks
Mostly looks good, but there's still an area of confusion for me https://codereview.chromium.org/11415152/diff/19001/net/net.gyp File net/net.gyp (left): https://codereview.chromium.org/11415152/diff/19001/net/net.gyp#oldcode2287 net/net.gyp:2287: 'additional_input_paths': ['<(SHARED_INTERMEDIATE_DIR)/net/template/NetError.java'], Do you no longer have to specify additional_input_paths to reference the specific generated .java files? Is that a bug in the _template.gypi?
https://codereview.chromium.org/11415152/diff/19001/net/net.gyp File net/net.gyp (left): https://codereview.chromium.org/11415152/diff/19001/net/net.gyp#oldcode2287 net/net.gyp:2287: 'additional_input_paths': ['<(SHARED_INTERMEDIATE_DIR)/net/template/NetError.java'], On 2012/12/07 03:20:10, Ryan Sleevi wrote: > Do you no longer have to specify additional_input_paths to reference the > specific generated .java files? > > Is that a bug in the _template.gypi? I believe this is handled by the 'generated_src_dirs' section in java_cpp_template.gypi. I've tested this manually by doing the following operations: - clean build - only touch net/base/net_error_list.h + rebuild - only touch NetError.template + rebuild - remove the generated NetError.java file + rebuild In all cases, the build/rebuild seems to complete fine, so I guess that's enough. However, if you think there is an edge case that wouldn't be covered, please let me know. If 'additional_input_paths' is required, it turns out a new variable will be needed in the caller to give the name of the output file. I don't think there is a way to perform file name translations (e.g. "NetError.template" -> "NetError.java") e.g. outside of a 'rules' section, and of course, one cannot add 'variables' inside a 'rules' section.
On 2012/12/07 08:46:07, digit1 wrote: > https://codereview.chromium.org/11415152/diff/19001/net/net.gyp > File net/net.gyp (left): > > https://codereview.chromium.org/11415152/diff/19001/net/net.gyp#oldcode2287 > net/net.gyp:2287: 'additional_input_paths': > ['<(SHARED_INTERMEDIATE_DIR)/net/template/NetError.java'], > On 2012/12/07 03:20:10, Ryan Sleevi wrote: > > Do you no longer have to specify additional_input_paths to reference the > > specific generated .java files? > > > > Is that a bug in the _template.gypi? > > I believe this is handled by the 'generated_src_dirs' section in > java_cpp_template.gypi. I've tested this manually by doing the following > operations: > > - clean build > - only touch net/base/net_error_list.h + rebuild > - only touch NetError.template + rebuild > - remove the generated NetError.java file + rebuild > > In all cases, the build/rebuild seems to complete fine, so I guess that's > enough. However, if you think there is an edge case that wouldn't be covered, > please let me know. > > If 'additional_input_paths' is required, it turns out a new variable will be > needed in the caller to give the name of the output file. I don't think there is > a way to perform file name translations (e.g. "NetError.template" -> > "NetError.java") e.g. outside of a 'rules' section, and of course, one cannot > add 'variables' inside a 'rules' section. digit: SGTM. Mostly I wanted to make sure that it was tested and working, since it was a difference. I'm glad to see that generated_src_dirs is "working as expected" LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/digit@chromium.org/11415152/19001
Message was sent while issue was closed.
Change committed as 172041 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
