Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9514018:
Add first java source code and gyp/ant changes to build java code. (Closed)
|
Created:
8 years, 9 months ago by navabi Modified:
8 years, 9 months ago CC:
chromium-reviews, brettw-cc_chromium.org, Ryan Sleevi, Steve Block Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd first java source code and gyp/ant changes to build java code.
Introduce the first java source for chrome on android to base/android/java.
Also add new target to build the java source in the directory. Currently
there is a dummy java file called DeleteStaging.java that should be replaced
when real base java source code is upstreamed. This will be the java files
from this patch: https://chromiumcodereview.appspot.com/9443018/. The
DeleteStaging.java file should be removed once real java code is checked in.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124134
Patch Set 1 #
Total comments: 13
Patch Set 2 : Add first java source code and gyp/ant changes to build java code. #
Total comments: 11
Messages
Total messages: 16 (0 generated)
This patch is a start for getting ready to upstream java code. Still need to add ant to the android install-deps
https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/base.xml File base/android/java/base.xml (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/base.... base/android/java/base.xml:29: <jar jarfile="${dist}/lib/Base.jar" basedir="${build}"/> chromium_base.jar https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.gyp File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:13: 'action_name': 'base_java', like this new name https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:15: '<(DEPTH)/base/android/java/base.xml', add TODO here about making this truly dependent on java files https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:18: '<(DEPTH)/base/android/java/dist/lib/Base.jar', chromium_base.jar https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:22: '-buildfile', looks like ant is NOT on the trybots. Let's talk about how to fix that. https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/org/c... File base/android/java/org/chromium/base/DeleteStaging.java (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/org/c... base/android/java/org/chromium/base/DeleteStaging.java:12: public class DeleteStaging I know this is to be deleted, but.... style guide says move the { on line 13 up to end of line 12 https://chromiumcodereview.appspot.com/9514018/diff/1/build/all_android.gyp File build/all_android.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1/build/all_android.gyp#n... build/all_android.gyp:53: '../base/android/java/java.gyp:base_java', Like This means we only have to add ant to the FYI bot first
went ahead and set up the inputs to be the list of java files in the directory. still need to figure out how to get ant on fyi-bots, try-bots. https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/base.xml File base/android/java/base.xml (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/base.... base/android/java/base.xml:29: <jar jarfile="${dist}/lib/Base.jar" basedir="${build}"/> On 2012/02/28 23:13:53, John Grabowski wrote: > chromium_base.jar Done. https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.gyp File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:15: '<(DEPTH)/base/android/java/base.xml', On 2012/02/28 23:13:53, John Grabowski wrote: > add TODO here about making this truly dependent on java files Done. https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:18: '<(DEPTH)/base/android/java/dist/lib/Base.jar', On 2012/02/28 23:13:53, John Grabowski wrote: > chromium_base.jar Done. https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/org/c... File base/android/java/org/chromium/base/DeleteStaging.java (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/org/c... base/android/java/org/chromium/base/DeleteStaging.java:12: public class DeleteStaging On 2012/02/28 23:13:53, John Grabowski wrote: > I know this is to be deleted, but.... style guide says move the { on line 13 up > to end of line 12 Done.
new patch adds *.java in directory as inputs in java.gyp (and other fixes).
LGTM You may want to wait for https://chromiumcodereview.appspot.com/9521014 to land first. Also please get feedback from at least one other person.
https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.gyp File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:15: '<(DEPTH)/base/android/java/base.xml', As discussed offline, we probably do need to express the dependency on the Java files themselves. I'm guessing if you specified them here though they would be input to the ant command which probably won't work. https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:16: '<!@(find . -name "*.java")' I wonder if we should be explicitly specifying java files. At least that's how cc files are handled in Chromium. https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:22: 'ant', Getting ant on the bots is also a pre-requisite for landing this, right?
https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.gyp File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1/base/android/java/java.... base/android/java/java.gyp:15: '<(DEPTH)/base/android/java/base.xml', On 2012/02/29 00:58:19, Yaron wrote: > As discussed offline, we probably do need to express the dependency on the Java > files themselves. I'm guessing if you specified them here though they would be > input to the ant command which probably won't work. oops. stale comment, please ignore.
https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:16: '<!@(find . -name "*.java")' On 2012/02/29 00:58:19, Yaron wrote: > I wonder if we should be explicitly specifying java files. At least that's how > cc files are handled in Chromium. Part of the reason for that in Chromium is so the files show up in generated projects (e.g. visual studio projects) for browsing. That doesn't apply to our use of Makefiles. In addition, the "don't list them all" model seems to honor concepts expressed in java in general (e.g. with ant). https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:22: 'ant', On 2012/02/29 00:58:19, Yaron wrote: > Getting ant on the bots is also a pre-requisite for landing this, right? Yes. I've updated the 'how to setup a bot' doc and installed ant explicitly on the FYI bot.
lgtm https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:16: '<!@(find . -name "*.java")' On 2012/02/29 01:01:29, John Grabowski wrote: > On 2012/02/29 00:58:19, Yaron wrote: > > I wonder if we should be explicitly specifying java files. At least that's how > > cc files are handled in Chromium. > > Part of the reason for that in Chromium is so the files show up in generated > projects (e.g. visual studio projects) for browsing. That doesn't apply to our > use of Makefiles. In addition, the "don't list them all" model seems to honor > concepts expressed in java in general (e.g. with ant). Ok. https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:22: 'ant', On 2012/02/29 01:01:29, John Grabowski wrote: > On 2012/02/29 00:58:19, Yaron wrote: > > Getting ant on the bots is also a pre-requisite for landing this, right? > > Yes. I've updated the 'how to setup a bot' doc and installed ant explicitly on > the FYI bot. Right, but what about developers? They'll need to manually install ant too or they won't be able to build. I'm ok with landing this now but we should do this in parallel (i.e. update install-build-deps-android.sh)
https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:16: '<!@(find . -name "*.java")' On 2012/02/29 00:58:19, Yaron wrote: > I wonder if we should be explicitly specifying java files. At least that's how > cc files are handled in Chromium. I'm not sure what the answer is here. Currently, the ant file compiles every java file in this directory tree. Thus, if we explicitly list the java files here, I think we would need to have the same list explicitly listed in the ant file. Otherwise, it could get confusing, especially for people that don't really know ant. They may not list a java file here, expecting it not to compile, and then not understand why the ant action is compiling that file (or something like that). I think for now we should leave it like this. If we want to explicitly specify, we should find a solution that is consistent with the ant build file (e..g the action here actually generates the ant file, which will list source files instead of compile everything in the directory). https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:22: 'ant', On 2012/02/29 00:58:19, Yaron wrote: > Getting ant on the bots is also a pre-requisite for landing this, right? Yes. That is correct.
Drive by comments https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:16: '<!@(find . -name "*.java")' As a drive by, one of the reasons this is done (listing all the files) is to keep GYP time low (similar to Ninja's goals) Using <!@() syntax vs explicitly listing them has no affect on the generators (such as MSVC) - if you globbed on .cc, they would still all individually show in Visual Studio, as they do today. My understanding of the history is that the transition from the early days of SCons/Hammer -> GYP, one of the concerns was file-globbing (the SCons-y) was becoming non-performant. See https://groups.google.com/d/topic/gyp-developer/-4sbLC5OfBk/discussion
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/navabi@google.com/9514018/1007
Change committed as 124134
+1 drive-by. https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... File base/android/java/java.gyp (right): https://chromiumcodereview.appspot.com/9514018/diff/1007/base/android/java/ja... base/android/java/java.gyp:16: '<!@(find . -name "*.java")' On 2012/02/29 01:05:06, navabi wrote: > > I'm not sure what the answer is here. It seems like the gyp-y answer would be to have gyp generate the ant file. I like it as it is to get a bare-bones .java build in place for the reasons we discussed, but before going too far down this path we may want to review if that would be possible.
http://codereview.chromium.org/9514018/diff/1007/base/android/java/java.gyp File base/android/java/java.gyp (right): http://codereview.chromium.org/9514018/diff/1007/base/android/java/java.gyp#n... base/android/java/java.gyp:19: '<(DEPTH)/base/android/java/dist/lib/chromium_base.jar', It seems a little ugly to write build output to the source tree. Is there a way to avoid this? If not, can we add this (and the .class files) to .gitignore?
I am in the process of cleaning this (and related issues) up. I will cc you on CLs that fix them jrg On Mon, Mar 26, 2012 at 5:02 AM, <steveblock@chromium.org> wrote: > > http://codereview.chromium.**org/9514018/diff/1007/base/** > android/java/java.gyp<http://codereview.chromium.org/9514018/diff/1007/base/android/java/java.gyp> > File base/android/java/java.gyp (right): > > http://codereview.chromium.**org/9514018/diff/1007/base/** > android/java/java.gyp#**newcode19<http://codereview.chromium.org/9514018/diff/1007/base/android/java/java.gyp#newcode19> > base/android/java/java.gyp:19: > '<(DEPTH)/base/android/java/**dist/lib/chromium_base.jar', > It seems a little ugly to write build output to the source tree. Is > there a way to avoid this? If not, can we add this (and the .class > files) to .gitignore? > > http://codereview.chromium.**org/9514018/<http://codereview.chromium.org/9514... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
