Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10386188:
Fix ninja build for android. (Closed)
|
Created:
8 years, 7 months ago by Yaron Modified:
8 years, 6 months ago CC:
chromium-reviews, jochen+watch-content_chromium.org, erikwright (departed), jam, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix ninja build for android.
The primary issues is specifying the right path to PRODUCT_DIR (i.e.
out/Release). The gyp generator for make specifies the absolute path but
for ninja would use a relative path. Since the gyp targets don't line
up with where the ant build files are located this causes failures such
as base's java being generated in base/android/out/Release/...
See:
https://groups.google.com/forum/#!msg/gyp-developer/K2T_9obUya0/qq78_Ut-E-AJ
for details.
A couple of other minor fixes:
- content java files are placed in out/Release/java/content to be
consisent with other packages.
- shared-libraries are now referenced by correct variables for apk-based
tests
- removed unused media/base/android/java/java.gyp (target is in
media/media.gyp)
TBR=mark@chromium.org,ben@chromium.org,rsleevi@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139418
Patch Set 1 #
Total comments: 2
Patch Set 2 : added absolute_path_dir #
Total comments: 2
Patch Set 3 : switched to absolute_product_out #
Total comments: 1
Patch Set 4 : switched to ant_build_out, rebase on apk_test template #Patch Set 5 : fix shared lib path #Patch Set 6 : and finally apk_test.gypi #
Messages
Total messages: 29 (0 generated)
LGTM if you ABSOLUTE_PRODUCT_DIR (or something similar) in a common location. http://codereview.chromium.org/10386188/diff/1/base/android/java/base.xml File base/android/java/base.xml (right): http://codereview.chromium.org/10386188/diff/1/base/android/java/base.xml#new... base/android/java/base.xml:55: <mkdir dir="${out.dir}"/> Looks like general cleanup... thx http://codereview.chromium.org/10386188/diff/1/base/base.gyp File base/base.gyp (right): http://codereview.chromium.org/10386188/diff/1/base/base.gyp#newcode689 base/base.gyp:689: '-DPRODUCT_DIR=`realpath <(PRODUCT_DIR)`', OSX has no realpath cmd. (Eventually we want to support OSX I think.) How about $(cd <(PRODUCT_DIR) && pwd -P) ? In any case, how about a new var ABSOLUTE_PRODUCT_DIR set in common.gypi and use that instead? So we can realpath (or os.path.realpath, or whatever) in a single place.
Nico: Can you look at build/common.gypi. I switched to a single variable "absolute_product_dir". I kept it lower-case to be consistent with other variables. Note the leading "/" so it's not relativized as well. Finally, I switched to John's recommendation for setting the path. Alternatively, I could define a build/realpath.py so it works on Mac too but I don't know if that's necessary. And here's a try-job with make which appears to still work: http://build.chromium.org/p/tryserver.chromium/builders/android_test/builds/79
Reading that gyp-developer thread, it looks like make is running the action with a pwd of base/android/java while ninja runs it in base/. Why is that? http://codereview.chromium.org/10386188/diff/6001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815 build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && pwd -P`', Instead of the leading '/', you could name the variable something that doesn't end in _dir, _dirs, _file, _files, _path, _paths. I think no relativization is done for variables that don't end in those paths. pwd won't work on windows.
> pwd won't work on windows. The Android build will never be supported on Windows. (That doesn't mean we don't want this variable to work on other platforms, of course.)
On 2012/05/17 17:11:31, John Grabowski wrote: > > pwd won't work on windows. > > The Android build will never be supported on Windows. (That doesn't mean we > don't want this variable to work on other platforms, of course.) Right, but if the rhs of variables is evaluated at variable definition time, this patch will break `gclient runhooks` on windows.
http://codereview.chromium.org/10386188/diff/6001/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815 build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && pwd -P`', On 2012/05/17 17:04:45, Nico wrote: > Instead of the leading '/', you could name the variable something that doesn't > end in _dir, _dirs, _file, _files, _path, _paths. I think no relativization is > done for variables that don't end in those paths. > Done > pwd won't work on windows. Honestly nobody's tried developing Chrome for Android on windows. Mac used to work and probably isn't far off. I don't want to spend time on windows right now so I've moved the variable to an Android-only section.
> > pwd won't work on windows. > > Honestly nobody's tried developing Chrome for Android on windows. Mac used to > work and probably isn't far off. I don't want to spend time on windows right now > so I've moved the variable to an Android-only section. As mentioned above, I don't care about chrome for android on windows, I just don't want to break the normal chrome/win build. Moving this to an android-only section works for me. "Reading that gyp-developer thread, it looks like make is running the action with a pwd of base/android/java while ninja runs it in base/. Why is that?"
On 2012/05/17 17:04:44, Nico wrote: > Reading that gyp-developer thread, it looks like make is running the action with > a pwd of base/android/java while ninja runs it in base/. Why is that? Don't know. The rules don't specify to run in base/android/java. I'd expect that both run out of base. > > http://codereview.chromium.org/10386188/diff/6001/build/common.gypi > File build/common.gypi (right): > > http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815 > build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && pwd -P`', > Instead of the leading '/', you could name the variable something that doesn't > end in _dir, _dirs, _file, _files, _path, _paths. I think no relativization is > done for variables that don't end in those paths. > > pwd won't work on windows.
On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: > On 2012/05/17 17:04:44, Nico wrote: > >> Reading that gyp-developer thread, it looks like make is running the >> action >> > with > >> a pwd of base/android/java while ninja runs it in base/. Why is that? >> > > Don't know. The rules don't specify to run in base/android/java. I'd > expect that > both run out of base. The thread you linked to had cmd_base_java_ant_base = LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml which has a cd base/android/java. > > > http://codereview.chromium.**org/10386188/diff/6001/build/**common.gypi<http:... >> File build/common.gypi (right): >> > > http://codereview.chromium.**org/10386188/diff/6001/build/** >> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && pwd >> > -P`', > >> Instead of the leading '/', you could name the variable something that >> doesn't >> >> end in _dir, _dirs, _file, _files, _path, _paths. I think no >> relativization is >> done for variables that don't end in those paths. >> > > pwd won't work on windows. >> > > > > http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >
Right. It's not in the gyp rule, it's in the generator. I think it's an intentional difference that Evan wanted Ninja to use relative paths wherever possible. I think in make it's in: pylib/gyp/generator/make.py around L864 On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org> wrote: > On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: > >> On 2012/05/17 17:04:44, Nico wrote: >> >>> Reading that gyp-developer thread, it looks like make is running the >>> action >>> >> with >> >>> a pwd of base/android/java while ninja runs it in base/. Why is that? >>> >> >> Don't know. The rules don't specify to run in base/android/java. I'd >> expect that >> both run out of base. > > > The thread you linked to had > > cmd_base_java_ant_base = > LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; > export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant > "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml > > which has a cd base/android/java. > > > > >> >> >> http://codereview.chromium.**org/10386188/diff/6001/build/**common.gypi<http:... >>> File build/common.gypi (right): >>> >> >> http://codereview.chromium.**org/10386188/diff/6001/build/** >>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && >>> pwd >>> >> -P`', >> >>> Instead of the leading '/', you could name the variable something that >>> doesn't >>> >>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>> relativization is >>> done for variables that don't end in those paths. >>> >> >> pwd won't work on windows. >>> >> >> >> >> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >> > >
On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman <yfriedman@chromium.org>wrote: > Right. It's not in the gyp rule, it's in the generator. I think it's an > intentional difference that Evan wanted Ninja to use relative paths > wherever possible. > > I think in make it's in: pylib/gyp/generator/make.py around L864 > Both ninja and make try to cd into the directory of the gyp file every action is defined in. Make does this where you pointed at, ninja does this in WriteNewNinjaRule around L1094. I'm wondering why they cd into different directories. > > > On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org> wrote: > >> On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: >> >>> On 2012/05/17 17:04:44, Nico wrote: >>> >>>> Reading that gyp-developer thread, it looks like make is running the >>>> action >>>> >>> with >>> >>>> a pwd of base/android/java while ninja runs it in base/. Why is that? >>>> >>> >>> Don't know. The rules don't specify to run in base/android/java. I'd >>> expect that >>> both run out of base. >> >> >> The thread you linked to had >> >> cmd_base_java_ant_base = >> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >> export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant >> "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml >> >> which has a cd base/android/java. >> >> >> >> >>> >>> >>> http://codereview.chromium.**org/10386188/diff/6001/build/**common.gypi<http:... >>>> File build/common.gypi (right): >>>> >>> >>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>>> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && >>>> pwd >>>> >>> -P`', >>> >>>> Instead of the leading '/', you could name the variable something that >>>> doesn't >>>> >>>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>>> relativization is >>>> done for variables that don't end in those paths. >>>> >>> >>> pwd won't work on windows. >>>> >>> >>> >>> >>> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >>> >> >> >
Turns out my makefile snippet might've been stale from a work-in-progress attempt. Here's a freshly-generated makefiles with the current gyp: ### Rules for action "ant_base": quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ cmd_base_java_ant_base = LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd base; mkdir -p $(builddir)/lib.java; ant "-DPRODUCT_DIR=\`cd $(builddir) && pwd -P\`" "-DPACKAGE_NAME=base" -buildfile android/java/base.xml So in the case of make, \`cd $(builddir) && pwd -P\` is redundant but is necessary for make. On Thu, May 17, 2012 at 11:12 AM, Nico Weber <thakis@chromium.org> wrote: > On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman <yfriedman@chromium.org>wrote: > >> Right. It's not in the gyp rule, it's in the generator. I think it's an >> intentional difference that Evan wanted Ninja to use relative paths >> wherever possible. >> >> I think in make it's in: pylib/gyp/generator/make.py around L864 >> > > Both ninja and make try to cd into the directory of the gyp file every > action is defined in. Make does this where you pointed at, ninja does this > in WriteNewNinjaRule around L1094. I'm wondering why they cd into different > directories. > > >> >> >> On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: >>> >>>> On 2012/05/17 17:04:44, Nico wrote: >>>> >>>>> Reading that gyp-developer thread, it looks like make is running the >>>>> action >>>>> >>>> with >>>> >>>>> a pwd of base/android/java while ninja runs it in base/. Why is that? >>>>> >>>> >>>> Don't know. The rules don't specify to run in base/android/java. I'd >>>> expect that >>>> both run out of base. >>> >>> >>> The thread you linked to had >>> >>> cmd_base_java_ant_base = >>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>> export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant >>> "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml >>> >>> which has a cd base/android/java. >>> >>> >>> >>> >>>> >>>> >>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>> common.gypi<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi> >>>>> File build/common.gypi (right): >>>>> >>>> >>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>>>> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) && >>>>> pwd >>>>> >>>> -P`', >>>> >>>>> Instead of the leading '/', you could name the variable something that >>>>> doesn't >>>>> >>>>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>>>> relativization is >>>>> done for variables that don't end in those paths. >>>>> >>>> >>>> pwd won't work on windows. >>>>> >>>> >>>> >>>> >>>> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >>>> >>> >>> >> >
On Thu, May 17, 2012 at 11:21 AM, Yaron Friedman <yfriedman@chromium.org>wrote: > Turns out my makefile snippet might've been stale from a work-in-progress > attempt. Here's a freshly-generated makefiles with the current gyp: > > ### Rules for action "ant_base": > > quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ > > cmd_base_java_ant_base = > LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; > export LD_LIBRARY_PATH; cd base; mkdir -p > $(builddir)/lib.java; ant "-DPRODUCT_DIR=\`cd $(builddir) && pwd -P\`" > "-DPACKAGE_NAME=base" -buildfile android/java/base.xml > > So in the case of make, \`cd $(builddir) && pwd -P\` is redundant but is > necessary for make. > I guess you meant "but it is necessary for ninja" :-) I'd be interested in the run line for make without your patch; I'd like to understand why this works for make but not for ninja on trunk. > > On Thu, May 17, 2012 at 11:12 AM, Nico Weber <thakis@chromium.org> wrote: > >> On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman <yfriedman@chromium.org>wrote: >> >>> Right. It's not in the gyp rule, it's in the generator. I think it's an >>> intentional difference that Evan wanted Ninja to use relative paths >>> wherever possible. >>> >>> I think in make it's in: pylib/gyp/generator/make.py around L864 >>> >> >> Both ninja and make try to cd into the directory of the gyp file every >> action is defined in. Make does this where you pointed at, ninja does this >> in WriteNewNinjaRule around L1094. I'm wondering why they cd into different >> directories. >> >> >>> >>> >>> On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org>wrote: >>> >>>> On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: >>>> >>>>> On 2012/05/17 17:04:44, Nico wrote: >>>>> >>>>>> Reading that gyp-developer thread, it looks like make is running the >>>>>> action >>>>>> >>>>> with >>>>> >>>>>> a pwd of base/android/java while ninja runs it in base/. Why is that? >>>>>> >>>>> >>>>> Don't know. The rules don't specify to run in base/android/java. I'd >>>>> expect that >>>>> both run out of base. >>>> >>>> >>>> The thread you linked to had >>>> >>>> cmd_base_java_ant_base = >>>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>>> export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant >>>> "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml >>>> >>>> which has a cd base/android/java. >>>> >>>> >>>> >>>> >>>>> >>>>> >>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>> common.gypi<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi> >>>>>> File build/common.gypi (right): >>>>>> >>>>> >>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>>>>> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) >>>>>> && pwd >>>>>> >>>>> -P`', >>>>> >>>>>> Instead of the leading '/', you could name the variable something >>>>>> that doesn't >>>>>> >>>>>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>>>>> relativization is >>>>>> done for variables that don't end in those paths. >>>>>> >>>>> >>>>> pwd won't work on windows. >>>>>> >>>>> >>>>> >>>>> >>>>> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >>>>> >>>> >>>> >>> >> >
On Thu, May 17, 2012 at 11:23 AM, Nico Weber <thakis@chromium.org> wrote: > On Thu, May 17, 2012 at 11:21 AM, Yaron Friedman <yfriedman@chromium.org>wrote: > >> Turns out my makefile snippet might've been stale from a work-in-progress >> attempt. Here's a freshly-generated makefiles with the current gyp: >> >> ### Rules for action "ant_base": >> >> quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ >> >> cmd_base_java_ant_base = >> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >> export LD_LIBRARY_PATH; cd base; mkdir -p >> $(builddir)/lib.java; ant "-DPRODUCT_DIR=\`cd $(builddir) && pwd -P\`" >> "-DPACKAGE_NAME=base" -buildfile android/java/base.xml >> >> So in the case of make, \`cd $(builddir) && pwd -P\` is redundant but is >> necessary for make. >> > > I guess you meant "but it is necessary for ninja" :-) > > Doh. Yes. :) > I'd be interested in the run line for make without your patch; I'd like to > understand why this works for make but not for ninja on trunk. > > Sure: ### Rules for action "ant_base": quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ cmd_base_java_ant_base = LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd base; mkdir -p $(builddir)/lib.java; ant "-DPRODUCT_DIR=$(builddir)" "-DPACKAGE_NAME=base" -buildfile android/java/base.xml > >> On Thu, May 17, 2012 at 11:12 AM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman <yfriedman@chromium.org >>> > wrote: >>> >>>> Right. It's not in the gyp rule, it's in the generator. I think it's an >>>> intentional difference that Evan wanted Ninja to use relative paths >>>> wherever possible. >>>> >>>> I think in make it's in: pylib/gyp/generator/make.py around L864 >>>> >>> >>> Both ninja and make try to cd into the directory of the gyp file every >>> action is defined in. Make does this where you pointed at, ninja does this >>> in WriteNewNinjaRule around L1094. I'm wondering why they cd into different >>> directories. >>> >>> >>>> >>>> >>>> On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org>wrote: >>>> >>>>> On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: >>>>> >>>>>> On 2012/05/17 17:04:44, Nico wrote: >>>>>> >>>>>>> Reading that gyp-developer thread, it looks like make is running the >>>>>>> action >>>>>>> >>>>>> with >>>>>> >>>>>>> a pwd of base/android/java while ninja runs it in base/. Why is that? >>>>>>> >>>>>> >>>>>> Don't know. The rules don't specify to run in base/android/java. I'd >>>>>> expect that >>>>>> both run out of base. >>>>> >>>>> >>>>> The thread you linked to had >>>>> >>>>> cmd_base_java_ant_base = >>>>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>>>> export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant >>>>> "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml >>>>> >>>>> which has a cd base/android/java. >>>>> >>>>> >>>>> >>>>> >>>>>> >>>>>> >>>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>>> common.gypi<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi> >>>>>>> File build/common.gypi (right): >>>>>>> >>>>>> >>>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>>>>>> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) >>>>>>> && pwd >>>>>>> >>>>>> -P`', >>>>>> >>>>>>> Instead of the leading '/', you could name the variable something >>>>>>> that doesn't >>>>>>> >>>>>>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>>>>>> relativization is >>>>>>> done for variables that don't end in those paths. >>>>>>> >>>>>> >>>>>> pwd won't work on windows. >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >>>>>> >>>>> >>>>> >>>> >>> >> >
On Thu, May 17, 2012 at 11:27 AM, Yaron Friedman <yfriedman@google.com>wrote: > On Thu, May 17, 2012 at 11:23 AM, Nico Weber <thakis@chromium.org> wrote: > >> On Thu, May 17, 2012 at 11:21 AM, Yaron Friedman <yfriedman@chromium.org>wrote: >> >>> Turns out my makefile snippet might've been stale from a >>> work-in-progress attempt. Here's a freshly-generated makefiles with the >>> current gyp: >>> >>> ### Rules for action "ant_base": >>> >>> quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ >>> >>> cmd_base_java_ant_base = >>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>> export LD_LIBRARY_PATH; cd base; mkdir -p >>> $(builddir)/lib.java; ant "-DPRODUCT_DIR=\`cd $(builddir) && pwd -P\`" >>> "-DPACKAGE_NAME=base" -buildfile android/java/base.xml >>> >>> So in the case of make, \`cd $(builddir) && pwd -P\` is redundant but is >>> necessary for make. >>> >> >> I guess you meant "but it is necessary for ninja" :-) >> >> Doh. Yes. :) > > >> I'd be interested in the run line for make without your patch; I'd like >> to understand why this works for make but not for ninja on trunk. >> >> > Sure: > > ### Rules for action "ant_base": > > quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ > > cmd_base_java_ant_base = > LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; > export LD_LIBRARY_PATH; cd base; mkdir -p > $(builddir)/lib.java; ant "-DPRODUCT_DIR=$(builddir)" > "-DPACKAGE_NAME=base" -buildfile android/java/base.xml > Where is this rule defined? cs.chromium.org for "Building base java sources" finds nothing. I'm guessing that action's command says '-DPRODUCT_DIR=<(PRODUCT_DIR)'? > > > >> >>> On Thu, May 17, 2012 at 11:12 AM, Nico Weber <thakis@chromium.org>wrote: >>> >>>> On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman < >>>> yfriedman@chromium.org> wrote: >>>> >>>>> Right. It's not in the gyp rule, it's in the generator. I think it's >>>>> an intentional difference that Evan wanted Ninja to use relative paths >>>>> wherever possible. >>>>> >>>>> I think in make it's in: pylib/gyp/generator/make.py around L864 >>>>> >>>> >>>> Both ninja and make try to cd into the directory of the gyp file every >>>> action is defined in. Make does this where you pointed at, ninja does this >>>> in WriteNewNinjaRule around L1094. I'm wondering why they cd into different >>>> directories. >>>> >>>> >>>>> >>>>> >>>>> On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org>wrote: >>>>> >>>>>> On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: >>>>>> >>>>>>> On 2012/05/17 17:04:44, Nico wrote: >>>>>>> >>>>>>>> Reading that gyp-developer thread, it looks like make is running >>>>>>>> the action >>>>>>>> >>>>>>> with >>>>>>> >>>>>>>> a pwd of base/android/java while ninja runs it in base/. Why is >>>>>>>> that? >>>>>>>> >>>>>>> >>>>>>> Don't know. The rules don't specify to run in base/android/java. I'd >>>>>>> expect that >>>>>>> both run out of base. >>>>>> >>>>>> >>>>>> The thread you linked to had >>>>>> >>>>>> cmd_base_java_ant_base = >>>>>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>>>>> export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant >>>>>> "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml >>>>>> >>>>>> which has a cd base/android/java. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>>>> common.gypi<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi> >>>>>>>> File build/common.gypi (right): >>>>>>>> >>>>>>> >>>>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>>>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>>>>>>> build/common.gypi:815: 'absolute_product_dir': '/`cd <(PRODUCT_DIR) >>>>>>>> && pwd >>>>>>>> >>>>>>> -P`', >>>>>>> >>>>>>>> Instead of the leading '/', you could name the variable something >>>>>>>> that doesn't >>>>>>>> >>>>>>>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>>>>>>> relativization is >>>>>>>> done for variables that don't end in those paths. >>>>>>>> >>>>>>> >>>>>>> pwd won't work on windows. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >>>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >> >
On Thu, May 17, 2012 at 3:26 PM, Nico Weber <thakis@chromium.org> wrote: > On Thu, May 17, 2012 at 11:27 AM, Yaron Friedman <yfriedman@google.com>wrote: > >> On Thu, May 17, 2012 at 11:23 AM, Nico Weber <thakis@chromium.org> wrote: >> >>> On Thu, May 17, 2012 at 11:21 AM, Yaron Friedman <yfriedman@chromium.org >>> > wrote: >>> >>>> Turns out my makefile snippet might've been stale from a >>>> work-in-progress attempt. Here's a freshly-generated makefiles with the >>>> current gyp: >>>> >>>> ### Rules for action "ant_base": >>>> >>>> quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ >>>> >>>> cmd_base_java_ant_base = >>>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>>> export LD_LIBRARY_PATH; cd base; mkdir -p >>>> $(builddir)/lib.java; ant "-DPRODUCT_DIR=\`cd $(builddir) && pwd -P\`" >>>> "-DPACKAGE_NAME=base" -buildfile android/java/base.xml >>>> >>>> So in the case of make, \`cd $(builddir) && pwd -P\` is redundant but >>>> is necessary for make. >>>> >>> >>> I guess you meant "but it is necessary for ninja" :-) >>> >>> Doh. Yes. :) >> >> >>> I'd be interested in the run line for make without your patch; I'd like >>> to understand why this works for make but not for ninja on trunk. >>> >>> >> Sure: >> >> ### Rules for action "ant_base": >> >> quiet_cmd_base_java_ant_base = ACTION Building base java sources. $@ >> >> cmd_base_java_ant_base = >> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >> export LD_LIBRARY_PATH; cd base; mkdir -p >> $(builddir)/lib.java; ant "-DPRODUCT_DIR=$(builddir)" >> "-DPACKAGE_NAME=base" -buildfile android/java/base.xml >> > > Where is this rule defined? cs.chromium.org for "Building base java > sources" finds nothing. I'm guessing that action's command says > '-DPRODUCT_DIR=<(PRODUCT_DIR)'? > See base_java in base.gyp. It includes build/java.gypi which defines the actual action. > > >> >> >> >>> >>>> On Thu, May 17, 2012 at 11:12 AM, Nico Weber <thakis@chromium.org>wrote: >>>> >>>>> On Thu, May 17, 2012 at 11:00 AM, Yaron Friedman < >>>>> yfriedman@chromium.org> wrote: >>>>> >>>>>> Right. It's not in the gyp rule, it's in the generator. I think it's >>>>>> an intentional difference that Evan wanted Ninja to use relative paths >>>>>> wherever possible. >>>>>> >>>>>> I think in make it's in: pylib/gyp/generator/make.py around L864 >>>>>> >>>>> >>>>> Both ninja and make try to cd into the directory of the gyp file every >>>>> action is defined in. Make does this where you pointed at, ninja does this >>>>> in WriteNewNinjaRule around L1094. I'm wondering why they cd into different >>>>> directories. >>>>> >>>>> >>>>>> >>>>>> >>>>>> On Thu, May 17, 2012 at 10:29 AM, Nico Weber <thakis@chromium.org>wrote: >>>>>> >>>>>>> On Thu, May 17, 2012 at 10:26 AM, <yfriedman@chromium.org> wrote: >>>>>>> >>>>>>>> On 2012/05/17 17:04:44, Nico wrote: >>>>>>>> >>>>>>>>> Reading that gyp-developer thread, it looks like make is running >>>>>>>>> the action >>>>>>>>> >>>>>>>> with >>>>>>>> >>>>>>>>> a pwd of base/android/java while ninja runs it in base/. Why is >>>>>>>>> that? >>>>>>>>> >>>>>>>> >>>>>>>> Don't know. The rules don't specify to run in base/android/java. >>>>>>>> I'd expect that >>>>>>>> both run out of base. >>>>>>> >>>>>>> >>>>>>> The thread you linked to had >>>>>>> >>>>>>> cmd_base_java_ant_base = >>>>>>> LD_LIBRARY_PATH=$(builddir)/lib.host:$(builddir)/lib.target:$$LD_LIBRARY_PATH; >>>>>>> export LD_LIBRARY_PATH; cd base/android/java; mkdir -p $(builddir); ant >>>>>>> "-DPRODUCT_DIR=$(builddir)" -buildfile base/android/java/base.xml >>>>>>> >>>>>>> which has a cd base/android/java. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>>>>> common.gypi<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi> >>>>>>>>> File build/common.gypi (right): >>>>>>>>> >>>>>>>> >>>>>>>> http://codereview.chromium.**org/10386188/diff/6001/build/** >>>>>>>>> common.gypi#newcode815<http://codereview.chromium.org/10386188/diff/6001/build/common.gypi#newcode815> >>>>>>>>> build/common.gypi:815: 'absolute_product_dir': '/`cd >>>>>>>>> <(PRODUCT_DIR) && pwd >>>>>>>>> >>>>>>>> -P`', >>>>>>>> >>>>>>>>> Instead of the leading '/', you could name the variable something >>>>>>>>> that doesn't >>>>>>>>> >>>>>>>>> end in _dir, _dirs, _file, _files, _path, _paths. I think no >>>>>>>>> relativization is >>>>>>>>> done for variables that don't end in those paths. >>>>>>>>> >>>>>>>> >>>>>>>> pwd won't work on windows. >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
As far as I understand, the issue is 4. below. 1.) Actions are defined to be run from the directory containing the gyp file that defines that action 2.) All generators take care to re-resolve relative paths so that they are relative to the action's pwd when passing them as arguments 3.) The ant files use . as basedir 4.) The ant documentation says "A relative path is resolved relative to the directory containing the build file." So ant uses a different directory to resolve relative paths than make does. Does changing basedir in base/android/java/base.xml to ../.. (and making no other changes) fix things? (Unrelatedly, and off-topic for this CL, so just asking out of curiosity: Do you have any plans to not shell out to a secondary build system for parts of the build eventually?)
On Thu, May 17, 2012 at 5:04 PM, <thakis@chromium.org> wrote: > As far as I understand, the issue is 4. below. > > 1.) Actions are defined to be run from the directory containing the gyp > file > that defines that action > > 2.) All generators take care to re-resolve relative paths so that they are > relative to the action's pwd when passing them as arguments > > 3.) The ant files use . as basedir > > 4.) The ant documentation says "A relative path is resolved relative to the > directory containing the build file." > > So ant uses a different directory to resolve relative paths than make does. > > Does changing basedir in base/android/java/base.xml to ../.. (and making no > other changes) fix things? > I originally tried that. The issue was around the AndroidManifest.xml which has to be in the same location as the ant buildfile. Since you asked, I tried looking again and this time came up with: http://stackoverflow.com/questions/7419582/different-location-of-androidmanif.... I haven't tried it though and we may have to sever the import of ${sdk.dir}/tools/ant/build.xml. Alternatively the apk manifest could move to the top-level dir, but it's preferrable to keep with it's associated build.xml and java files. > > > (Unrelatedly, and off-topic for this CL, so just asking out of curiosity: > Do you > have any plans to not shell out to a secondary build system for parts of > the > build eventually?) > No plans currently. ant is commonly used for building Java/Android projects and allows for re-use (e.g. relying on ant rules provided with android sdk to build apks) > > http://codereview.chromium.**org/10386188/<http://codereview.chromium.org/103... >
On 2012/05/18 01:38:57, Yaron wrote: > On Thu, May 17, 2012 at 5:04 PM, <mailto:thakis@chromium.org> wrote: > > > As far as I understand, the issue is 4. below. > > > > 1.) Actions are defined to be run from the directory containing the gyp > > file > > that defines that action > > > > 2.) All generators take care to re-resolve relative paths so that they are > > relative to the action's pwd when passing them as arguments > > > > 3.) The ant files use . as basedir > > > > 4.) The ant documentation says "A relative path is resolved relative to the > > directory containing the build file." > > > > So ant uses a different directory to resolve relative paths than make does. > > > > Does changing basedir in base/android/java/base.xml to ../.. (and making no > > other changes) fix things? > > > > I originally tried that. The issue was around the AndroidManifest.xml which > has to be in the same location as the ant buildfile. Since you asked, I > tried looking again and this time came up with: > http://stackoverflow.com/questions/7419582/different-location-of-androidmanif.... > I haven't tried it though and we may have to sever the import of > ${sdk.dir}/tools/ant/build.xml. Alternatively the apk manifest could move > to the top-level dir, but it's preferrable to keep with it's associated > build.xml and java files. > > > > > > > (Unrelatedly, and off-topic for this CL, so just asking out of curiosity: > > Do you > > have any plans to not shell out to a secondary build system for parts of > > the > > build eventually?) > > > > No plans currently. ant is commonly used for building Java/Android projects > and allows for re-use (e.g. relying on ant rules provided with android sdk > to build apks) > > > > > > > http://codereview.chromium.**org/10386188/%3Chttp://codereview.chromium.org/1...> > > Any additional feedback, Nico?
My feedback is that I think this CL works around gyp's model of actions. Since this works for all generators that android cares about, I can live with it if you can find no other way, but I'm not terribly happy about it (because other people will see abs_build_dir and might try to use it in other places). http://codereview.chromium.org/10386188/diff/5002/build/java.gypi File build/java.gypi (left): http://codereview.chromium.org/10386188/diff/5002/build/java.gypi#oldcode47 build/java.gypi:47: '-DPRODUCT_DIR=<(PRODUCT_DIR)', Another possible approach would be to filter <(PRODUCT_DIR) through `t if '$' in t else os.path.join(os.path.relpath('.', <(java_in_dir)), <(PRODUCT_DIR))`. That would be a noop in make and re-relativize the path to java_in_dir for ninja.
On 2012/05/21 19:14:19, Nico wrote: > My feedback is that I think this CL works around gyp's model of actions. Since > this works for all generators that android cares about, I can live with it if > you can find no other way, but I'm not terribly happy about it (because other > people will see abs_build_dir and might try to use it in other places). > > http://codereview.chromium.org/10386188/diff/5002/build/java.gypi > File build/java.gypi (left): > > http://codereview.chromium.org/10386188/diff/5002/build/java.gypi#oldcode47 > build/java.gypi:47: '-DPRODUCT_DIR=<(PRODUCT_DIR)', > Another possible approach would be to filter <(PRODUCT_DIR) through `t if '$' in > t else os.path.join(os.path.relpath('.', <(java_in_dir)), <(PRODUCT_DIR))`. That > would be a noop in make and re-relativize the path to java_in_dir for ninja. So the original realpath (or cd && pwd) implementation (from patch 1) does effectively the same thing as what you're suggesting. I think John's original complaint was that I had to copy it to a bunch of places instead of hiding it nicely in one variable and re-using that. You don't like the one variable because it's prone to mis-use. Given that we plan to templatize the apk rules as well, I'm inclined to listen to you because we can hide the ugly command in two templates and not define an absolute_product_dir rule. I'll wait for John to comment
On 2012/05/22 17:23:31, Yaron wrote: > On 2012/05/21 19:14:19, Nico wrote: > > My feedback is that I think this CL works around gyp's model of actions. Since > > this works for all generators that android cares about, I can live with it if > > you can find no other way, but I'm not terribly happy about it (because other > > people will see abs_build_dir and might try to use it in other places). > > > > http://codereview.chromium.org/10386188/diff/5002/build/java.gypi > > File build/java.gypi (left): > > > > http://codereview.chromium.org/10386188/diff/5002/build/java.gypi#oldcode47 > > build/java.gypi:47: '-DPRODUCT_DIR=<(PRODUCT_DIR)', > > Another possible approach would be to filter <(PRODUCT_DIR) through `t if '$' > in > > t else os.path.join(os.path.relpath('.', <(java_in_dir)), <(PRODUCT_DIR))`. > That > > would be a noop in make and re-relativize the path to java_in_dir for ninja. > > So the original realpath (or cd && pwd) implementation (from patch 1) does > effectively the same thing as what you're suggesting. I think John's original > complaint was that I had to copy it to a bunch of places instead of hiding it > nicely in one variable and re-using that. You don't like the one variable > because it's prone to mis-use. Given that we plan to templatize the apk rules as > well, I'm inclined to listen to you because we can hide the ugly command in two > templates and not define an absolute_product_dir rule. I'll wait for John to > comment It does not make sense that cut-and-paste of a variable is a good idea. How about we name the global ant_build_base_dir to help avoid misuse?
Ok, rebased on Nilesh's http://codereview.chromium.org/10399126/ which makes it a bit cleaner. Also, stuck with John's suggestion and named it "ant_build_out". avi: can you rubberstamp content?
On 2012/05/25 20:52:09, Yaron wrote: > Ok, rebased on Nilesh's http://codereview.chromium.org/10399126/ which makes it > a bit cleaner. Also, stuck with John's suggestion and named it "ant_build_out". > > avi: can you rubberstamp content? rubberstamped lgtm; i haven't any idea how that works.
On 2012/05/25 21:05:22, Avi wrote: > On 2012/05/25 20:52:09, Yaron wrote: > > Ok, rebased on Nilesh's http://codereview.chromium.org/10399126/ which makes > it > > a bit cleaner. Also, stuck with John's suggestion and named it > "ant_build_out". > > > > avi: can you rubberstamp content? > > rubberstamped lgtm; i haven't any idea how that works. Actually, I was mistaken. This didn't get cleaner because something I thought would be in the template isn't. It seems like ant_build_out is the way to go and I've done that and propagated it everywhere. Confirmed that everythign builds with ninja: $ GYP_DEFINES="$GYP_DEFINES gtest_target_type=shared_library" android_gyp $ PATH=/usr/local/google/code/goma/android/:/usr/local/google/android-ndk-r7/toolchains/arm-linux-androideabi-4.4.3/prebuilt/linux-x86/bin/:$PATH ninja -j250 -C out/Release/ All $ build/android/run_tests.py -s out/Release/base_unittests_apk/base_unittests-debug.apk jrg: PTAL
LGTM!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/10386188/23014
Change committed as 139418 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
