Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10918045:
Fix Clang build on Android. (Closed)
|
Created:
8 years, 3 months ago by Philippe Modified:
8 years, 3 months ago CC:
chromium-reviews, peter+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy+watch_chromium.org, Isaac (away) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix Clang build on Android.
This is part of Chrome for Android upstreaming.
In case we are doing a Clang build, we have to unset CC_target and CXX_target
(if they are gcc-related).
Otherwise GYP ends up generating a gcc build (although we set 'clang' to 1 in
$GYP_DEFINES).
This behavior was introduced by 54d2f6fe6d8a7b9d9786bd1f8540df6b4f46b83f in
GYP.
BUG=143889
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155185
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address Hans' comment #Patch Set 3 : Handle case where clang= appears multiple times #Patch Set 4 : Address David's (offline) comment #
Total comments: 2
Patch Set 5 : Address Sam's comment #Messages
Total messages: 18 (0 generated)
Sam, feel free to suggest a better approach if there is one. This was a quick and dirty hack we did downstream in a hurry.
+hans and bulach
http://codereview.chromium.org/10918045/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/10918045/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:114: if echo "$GYP_DEFINES" | grep -q clang; then What if i have clang=0 in my GYP_DEFINES? This looks pretty rigid..
http://codereview.chromium.org/10918045/diff/1/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/10918045/diff/1/build/android/envsetup.sh#newc... build/android/envsetup.sh:114: if echo "$GYP_DEFINES" | grep -q clang; then On 2012/09/03 13:53:19, hans wrote: > What if i have clang=0 in my GYP_DEFINES? > This looks pretty rigid.. Indeed. I guess you would be the first one to have that but you're right this should have been more robust :)
On 2012/09/03 14:02:29, Philippe wrote: > http://codereview.chromium.org/10918045/diff/1/build/android/envsetup.sh > File build/android/envsetup.sh (right): > > http://codereview.chromium.org/10918045/diff/1/build/android/envsetup.sh#newc... > build/android/envsetup.sh:114: if echo "$GYP_DEFINES" | grep -q clang; then > On 2012/09/03 13:53:19, hans wrote: > > What if i have clang=0 in my GYP_DEFINES? > > This looks pretty rigid.. > > Indeed. I guess you would be the first one to have that but you're right this > should have been more robust :) Good thing I don't have clang=2 in my GYP_DEFINES ;) I'm not familiar with the gyp issue behind this, but I guess the change itself lgtm.
Thanks Hans :) Let's see what Sam thinks. He's the one behind 54d2f6fe6d8a7b9d9786bd1f8540df6b4f46b83f in GYP. He might have a nicer fix.
On 2012/09/03 14:10:02, Philippe wrote: > Thanks Hans :) Let's see what Sam thinks. He's the one behind > 54d2f6fe6d8a7b9d9786bd1f8540df6b4f46b83f in GYP. He might have a nicer fix. FYI, I uploaded a new patch set handling the case where clang=\d appears multiple times in $GYP_DEFINES. David suggested that offline.
http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... build/android/envsetup.sh:117: unset AR_target Note that I'm actually planning to remove the *_target declarations from envsetup at some point in the near future. At least for make/ninja they're not necessary anymore except for one place in v8. If we can clean that up then we don't need to set or unset these as and can rely on gyp for everything. As this is temporary, it seems reasonable for now. (also if you want to fix the underlying issue let me know and I can point you at it :)
On 2012/09/04 17:16:18, Yaron wrote: > http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh > File build/android/envsetup.sh (right): > > http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... > build/android/envsetup.sh:117: unset AR_target > Note that I'm actually planning to remove the *_target declarations from > envsetup at some point in the near future. At least for make/ninja they're not > necessary anymore except for one place in v8. If we can clean that up then we > don't need to set or unset these as and can rely on gyp for everything. As this > is temporary, it seems reasonable for now. (also if you want to fix the > underlying issue let me know and I can point you at it :) Sorry for the late reply Philippe, I've been on holiday for the last week. I'm new to chrome and completely new to the android build so I might not be the best person to review this fix. Having said that it seems to me that having *_target set to gcc at the same time as clang=1 doesn't make much sense. Would it be cleaner to conditionally export these variables rather than conditionally unset them after export?
Good point Sam, I followed your suggestion. http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh File build/android/envsetup.sh (right): http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... build/android/envsetup.sh:117: unset AR_target On 2012/09/04 17:16:19, Yaron wrote: > Note that I'm actually planning to remove the *_target declarations from > envsetup at some point in the near future. At least for make/ninja they're not > necessary anymore except for one place in v8. If we can clean that up then we > don't need to set or unset these as and can rely on gyp for everything. As this > is temporary, it seems reasonable for now. (also if you want to fix the > underlying issue let me know and I can point you at it :) Yes that sounds good, please assign the underlying bug to me :). For now let's keep this (with the approach suggested by Sam).
On 2012/09/05 15:23:24, Philippe wrote: > Good point Sam, I followed your suggestion. > > http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh > File build/android/envsetup.sh (right): > > http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... > build/android/envsetup.sh:117: unset AR_target > On 2012/09/04 17:16:19, Yaron wrote: > > Note that I'm actually planning to remove the *_target declarations from > > envsetup at some point in the near future. At least for make/ninja they're not > > necessary anymore except for one place in v8. If we can clean that up then we > > don't need to set or unset these as and can rely on gyp for everything. As > this > > is temporary, it seems reasonable for now. (also if you want to fix the > > underlying issue let me know and I can point you at it :) > > Yes that sounds good, please assign the underlying bug to me :). For now let's > keep this (with the approach suggested by Sam). crbug.com/146561 Thanks!
Semi-related: we have a VM allocated for an upstream clang builder to reduce unexpected clang issues on the merge. Would any of you like to take on writing a buildbot_fyi_clang.sh script and making the necessary changes to buildbot_functions.sh? Assume we can pass in new variables to factory properties, just like we're doing with the buildtype variable. On Wed, Sep 5, 2012 at 10:20 AM, <yfriedman@chromium.org> wrote: > On 2012/09/05 15:23:24, Philippe wrote: >> >> Good point Sam, I followed your suggestion. > > >> >> http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh >> File build/android/envsetup.sh (right): > > > > http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... >> >> build/android/envsetup.sh:117: unset AR_target >> On 2012/09/04 17:16:19, Yaron wrote: >> > Note that I'm actually planning to remove the *_target declarations from >> > envsetup at some point in the near future. At least for make/ninja >> > they're > > not >> >> > necessary anymore except for one place in v8. If we can clean that up >> > then > > we >> >> > don't need to set or unset these as and can rely on gyp for everything. >> > As >> this >> > is temporary, it seems reasonable for now. (also if you want to fix the >> > underlying issue let me know and I can point you at it :) > > >> Yes that sounds good, please assign the underlying bug to me :). For now >> let's >> keep this (with the approach suggested by Sam). > > > crbug.com/146561 > > Thanks! > > http://codereview.chromium.org/10918045/
Related bug is crbug.com/143931 On Wed, Sep 5, 2012 at 10:30 AM, Isaac Levy <ilevy@chromium.org> wrote: > Semi-related: we have a VM allocated for an upstream clang builder to > reduce unexpected clang issues on the merge. Would any of you like to > take on writing a buildbot_fyi_clang.sh script and making the > necessary changes to buildbot_functions.sh? Assume we can pass in new > variables to factory properties, just like we're doing with the > buildtype variable. > > On Wed, Sep 5, 2012 at 10:20 AM, <yfriedman@chromium.org> wrote: >> On 2012/09/05 15:23:24, Philippe wrote: >>> >>> Good point Sam, I followed your suggestion. >> >> >>> >>> http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh >>> File build/android/envsetup.sh (right): >> >> >> >> http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... >>> >>> build/android/envsetup.sh:117: unset AR_target >>> On 2012/09/04 17:16:19, Yaron wrote: >>> > Note that I'm actually planning to remove the *_target declarations from >>> > envsetup at some point in the near future. At least for make/ninja >>> > they're >> >> not >>> >>> > necessary anymore except for one place in v8. If we can clean that up >>> > then >> >> we >>> >>> > don't need to set or unset these as and can rely on gyp for everything. >>> > As >>> this >>> > is temporary, it seems reasonable for now. (also if you want to fix the >>> > underlying issue let me know and I can point you at it :) >> >> >>> Yes that sounds good, please assign the underlying bug to me :). For now >>> let's >>> keep this (with the approach suggested by Sam). >> >> >> crbug.com/146561 >> >> Thanks! >> >> http://codereview.chromium.org/10918045/
On 2012/09/05 17:33:05, Isaac wrote: > Related bug is crbug.com/143931 > > On Wed, Sep 5, 2012 at 10:30 AM, Isaac Levy <mailto:ilevy@chromium.org> wrote: > > Semi-related: we have a VM allocated for an upstream clang builder to > > reduce unexpected clang issues on the merge. Would any of you like to > > take on writing a buildbot_fyi_clang.sh script and making the > > necessary changes to buildbot_functions.sh? Assume we can pass in new > > variables to factory properties, just like we're doing with the > > buildtype variable. > > > > On Wed, Sep 5, 2012 at 10:20 AM, <mailto:yfriedman@chromium.org> wrote: > >> On 2012/09/05 15:23:24, Philippe wrote: > >>> > >>> Good point Sam, I followed your suggestion. > >> > >> > >>> > >>> http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh > >>> File build/android/envsetup.sh (right): > >> > >> > >> > >> > http://codereview.chromium.org/10918045/diff/1002/build/android/envsetup.sh#n... > >>> > >>> build/android/envsetup.sh:117: unset AR_target > >>> On 2012/09/04 17:16:19, Yaron wrote: > >>> > Note that I'm actually planning to remove the *_target declarations from > >>> > envsetup at some point in the near future. At least for make/ninja > >>> > they're > >> > >> not > >>> > >>> > necessary anymore except for one place in v8. If we can clean that up > >>> > then > >> > >> we > >>> > >>> > don't need to set or unset these as and can rely on gyp for everything. > >>> > As > >>> this > >>> > is temporary, it seems reasonable for now. (also if you want to fix the > >>> > underlying issue let me know and I can point you at it :) > >> > >> > >>> Yes that sounds good, please assign the underlying bug to me :). For now > >>> let's > >>> keep this (with the approach suggested by Sam). > >> > >> > >> crbug.com/146561 > >> > >> Thanks! > >> > >> http://codereview.chromium.org/10918045/ I thought that sivachandra@ was actively working on that so I didn't want to steal his bug :) If he's not I would be happy to do that to relieve the post-merge pain downstream. I will reply to the bug.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/10918045/6
Change committed as 155185 |
