Use the local crazy_linker source fork in place of the NDK.

Use the local crazy_linker source fork in place of the NDK.

The crazy_linker in the android NDK has proven hard to get changes into.  When several chromium projects became blocked by this, we decided to temporarily fork it to chromium, and then upstream changes back to the NDK once these projects complete.  The following change forked the code:

https://codereview.chromium.org/322433006/

This change adds a gyp file to build the forked crazy_linker, and in base/base.gyp, references the forked source in place of the NDK source.

BUG=384700
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277409

[rmcilroy, 2014-06-13 15:19:45]

On 2014/06/13 14:22:03, simonb1 wrote:

Please update README.chromium to mention that crazy_linker.gyp is an added file
(to make sure it doesn't end up back in the NDK when we unfork.  Also, as a
followup, could you also remove the crazy_linker.gyp in the NDK's directory and
update the README.chromium in the NDK to remove references to crazy.gyp from
there please

LGTM otherwise.

[rmcilroy, 2014-06-13 15:26:40]

On 2014/06/13 15:19:45, rmcilroy wrote:
> On 2014/06/13 14:22:03, simonb1 wrote:
> 
> Please update README.chromium to mention that crazy_linker.gyp is an added
file
> (to make sure it doesn't end up back in the NDK when we unfork.  Also, as a
> followup, could you also remove the crazy_linker.gyp in the NDK's directory
and
> update the README.chromium in the NDK to remove references to crazy.gyp from
> there please
> 
> LGTM otherwise.

You also need an owner of base for the base/base.gyp change.

[simonb1, 2014-06-13 16:33:09]

+thakis for base/base.gyp review.

[Nico, 2014-06-13 20:37:52]

CL descriptions should say why you're doing a change.

[simonb1, 2014-06-13 22:18:34]

On 2014/06/13 20:37:52, Nico (away) wrote:
> CL descriptions should say why you're doing a change.

Updated.  Ptal.

[Nico, 2014-06-13 22:54:35]

Thanks.

It looks like this is something spanning several CLs, so there should've been a
tracking bug (which should've been referenced on the cl that forked the linker).

https://codereview.chromium.org/322433006/patch/80001/90003 doesn't explain what
was actually changed in the fork – is that described somewhere?

(The change itself looks fine, after the comment below is fixed)

https://codereview.chromium.org/330253003/diff/1/third_party/android_crazy_li...
File third_party/android_crazy_linker/crazy_linker.gyp (right):

https://codereview.chromium.org/330253003/diff/1/third_party/android_crazy_li...
third_party/android_crazy_linker/crazy_linker.gyp:6: # crazy_linker sources as
_part_ of another target.
This doesn't look like it's true.

[simonb1, 2014-06-14 08:05:59]

On 2014/06/13 22:54:35, Nico (away) wrote:
> Thanks.
> 
> It looks like this is something spanning several CLs, so there should've been
a
> tracking bug (which should've been referenced on the cl that forked the
linker).

Created https://code.google.com/p/chromium/issues/detail?id=384700


> 
> https://codereview.chromium.org/322433006/patch/80001/90003 doesn't explain
what
> was actually changed in the fork – is that described somewhere?

In the associated README.chromium.


> 
> (The change itself looks fine, after the comment below is fixed)
> 
>
https://codereview.chromium.org/330253003/diff/1/third_party/android_crazy_li...
> File third_party/android_crazy_linker/crazy_linker.gyp (right):
> 
>
https://codereview.chromium.org/330253003/diff/1/third_party/android_crazy_li...
> third_party/android_crazy_linker/crazy_linker.gyp:6: # crazy_linker sources as
> _part_ of another target.
> This doesn't look like it's true.

Indeed, removed.  Comment is from the upstream NDK crazy_linker.gyp, so wrong
there also by the look of things.  Added to my list of future upstream fixes.

Thanks for the notes.  Addressed in patch set 2.  Ptal.

[chromium-reviews, 2014-06-14 21:56:37]

On Sat, Jun 14, 2014 at 1:05 AM, <simonb@google.com> wrote:

> On 2014/06/13 22:54:35, Nico (away) wrote:
>
>> Thanks.
>>
>
>  It looks like this is something spanning several CLs, so there should've
>> been
>>
> a
>
>> tracking bug (which should've been referenced on the cl that forked the
>>
> linker).
>
> Created https://code.google.com/p/chromium/issues/detail?id=384700


Thanks. Can you put some concrete examples of things you wanted to change
there, please?


>
>
>
>
>  https://codereview.chromium.org/322433006/patch/80001/90003 doesn't
>> explain
>>
> what
>
>> was actually changed in the fork -&nbsp;is that described somewhere?
>>
>
> In the associated README.chromium.


That's where I looked.
https://codereview.chromium.org/322433006/diff/80001/third_party/android_craz...
says that a license header was added in a file. Is that the only change?


>
>
>
>
>  (The change itself looks fine, after the comment below is fixed)
>>
>
>
> https://codereview.chromium.org/330253003/diff/1/third_
> party/android_crazy_linker/crazy_linker.gyp
>
>> File third_party/android_crazy_linker/crazy_linker.gyp (right):
>>
>
>
> https://codereview.chromium.org/330253003/diff/1/third_
> party/android_crazy_linker/crazy_linker.gyp#newcode6
>
>> third_party/android_crazy_linker/crazy_linker.gyp:6: # crazy_linker
>> sources as
>> _part_ of another target.
>> This doesn't look like it's true.
>>
>
> Indeed, removed.  Comment is from the upstream NDK crazy_linker.gyp, so
> wrong
> there also by the look of things.  Added to my list of future upstream
> fixes.
>
> Thanks for the notes.  Addressed in patch set 2.  Ptal.
>
>
> https://codereview.chromium.org/330253003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[simonb1, 2014-06-15 23:26:31]

On 2014/06/14 21:56:37, chromium-reviews wrote:
> Thanks. Can you put some concrete examples of things you wanted to change
> there, please?

Done.


https://codereview.chromium.org/322433006/diff/80001/third_party/android_craz...
> says that a license header was added in a file. Is that the only change?

Yes.

[Nico, 2014-06-15 23:28:51]

lgtm

Thanks!

[simonb1, 2014-06-16 10:18:58]

The CQ bit was checked by simonb@google.com

[commit-bot: I haz the power, 2014-06-16 10:19:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonb@chromium.org/330253003/40001

[commit-bot: I haz the power, 2014-06-16 12:18:16]

Change committed as 277409