Pass the gold binary to the linker using only absolute paths.

Pass the gold binary to the linker using only absolute paths.

The previous expansion using <(PRODUCT_DIR)/../.. did not work correctly
if one was using a different build directory layout (out-of-source
builds, or some build directory with a different number of
subdirectories).

Solve this by doing the same as the sysroot variables: stop using
relative paths from the build directory and extract absolute paths using
DEPTH.

TEST=./build/gyp_chromium -Goutput_dir=/somewhere/else
R=phajdan.jr@chromium.org,thakis@chromium.org,dpranke@chromium.org

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

[Paweł Hajdan Jr., 2013-11-22 18:19:43]

LGTM, indeed we already use similar technique for sysroots.

[commit-bot: I haz the power, 2013-11-22 18:21:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/raphael.kubo.da.costa@intel.com/836730...

[commit-bot: I haz the power, 2013-11-22 20:46:59]

Change committed as 236822

[Nico, 2013-11-23 05:14:55]

not lgtm

This breaks moving the output directory without regypping. Several places in the
code assume that "exe directory + ../.. gives you source root", so this doesn't
really fix anything for out of source build dirs either.

[Raphael Kubo da Costa (rakuco), 2013-11-23 10:38:03]

> This breaks moving the output directory without regypping.

Sorry, can you explain how? If anything, using relative paths as we were doing
before should be worse in terms of relocation than using absolute ones like we
do after this patch; moving it from /tmp/foo to /tmp/bar/baz breaks with and
without the patch due to relative paths in the dependency/target names, but
without the patch it wouldn't be possible to use /tmp/foo in the first place,
and renaming /tmp/foo to /tmp/bar should be fine.

[Nico, 2013-11-23 10:41:37]

On Sat, Nov 23, 2013 at 7:38 PM, <raphael.kubo.da.costa@intel.com> wrote:

> This breaks moving the output directory without regypping.
>>
>
> Sorry, can you explain how?


mv out out_old
GYP_DEFINES=foo=1 build/gyp_chromium
ninja -C out/Release chrome
# ...oh, forgot something, let's try that in the old outdir
ninja -C out_old/Release


> If anything, using relative paths as we were doing
> before should be worse in terms of relocation than using absolute ones
> like we
> do after this patch; moving it from /tmp/foo to /tmp/bar/baz breaks with
> and
> without the patch due to relative paths in the dependency/target names, but
> without the patch it wouldn't be possible to use /tmp/foo in the first
> place,
> and renaming /tmp/foo to /tmp/bar should be fine.
>
> https://codereview.chromium.org/83673002/
>

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

[Raphael Kubo da Costa (rakuco), 2013-11-23 11:02:36]

>>> This breaks moving the output directory without regypping.
>> Sorry, can you explain how?
>
> mv out out_old
> GYP_DEFINES=foo=1 build/gyp_chromium
> ninja -C out/Release chrome
> # ...oh, forgot something, let's try that in the old outdir
> ninja -C out_old/Release

If you mean getting an error like

  ninja: error:
 
'../../out/gypfiles/third_party/WebKit/Source/bindings/main_idl_files_list.tmp',
  needed by 'gen/blink/BindingsDerivedSources.txt', missing and no known
  rule to make it

because "out" is now "out_old", it is unrelated to this patch. The only
thing it does is make the gold binary be found correctly in more
situations than before.

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

[Nico, 2013-11-27 03:00:03]

On Sat, Nov 23, 2013 at 3:03 AM, Raphael Kubo da Costa <
raphael.kubo.da.costa@intel.com> wrote:

> >>> This breaks moving the output directory without regypping.
> >> Sorry, can you explain how?
> >
> > mv out out_old
> > GYP_DEFINES=foo=1 build/gyp_chromium
> > ninja -C out/Release chrome
> > # ...oh, forgot something, let's try that in the old outdir
> > ninja -C out_old/Release
>
> If you mean getting an error like
>
>   ninja: error:
>
>
'../../out/gypfiles/third_party/WebKit/Source/bindings/main_idl_files_list.tmp',
>   needed by 'gen/blink/BindingsDerivedSources.txt', missing and no known
>   rule to make it
>

This looks like an unrelated bug I apparently introduced when I
reimplemented the gypfiles stuff. Can you file a bug on me for this?


> because "out" is now "out_old", it is unrelated to this patch. The only
> thing it does is make the gold binary be found correctly in more
> situations than before.
>

Generated ninja files on linux shouldn't contain absolute paths. If they
do, that's a bug. This CL adds absolute paths to them, as far as I
understand.

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

[Raphael Kubo da Costa (rakuco), 2013-11-27 11:10:19]

> This looks like an unrelated bug I apparently introduced when I
> reimplemented the gypfiles stuff. Can you file a bug on me for this?

Done, https://code.google.com/p/gyp/issues/detail?id=388

>> because "out" is now "out_old", it is unrelated to this patch. The only
>> thing it does is make the gold binary be found correctly in more
>> situations than before.
>
> Generated ninja files on linux shouldn't contain absolute paths. If they
> do, that's a bug. This CL adds absolute paths to them, as far as I
> understand.

That's a design decision I was completely unaware of. Is there more
documentation about it somewhere (I'm wondering, for example, if it's
something ninja-on-Linux specific, independent of generator or what)?

And how about the sysroot variables that contain absolute paths, which I
modeled this change after? I don't see how specifying an absolute path
for the linker can cause any harm or be worse than relative paths, but I
can try fixing it differently if that's the case.

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

[Nico, 2013-11-27 17:42:14]

On Wed, Nov 27, 2013 at 8:11 PM, Raphael Kubo da Costa <
raphael.kubo.da.costa@intel.com> wrote:

> > This looks like an unrelated bug I apparently introduced when I
> > reimplemented the gypfiles stuff. Can you file a bug on me for this?
>
> Done, https://code.google.com/p/gyp/issues/detail?id=388
>
> >> because "out" is now "out_old", it is unrelated to this patch. The only
> >> thing it does is make the gold binary be found correctly in more
> >> situations than before.
> >
> > Generated ninja files on linux shouldn't contain absolute paths. If they
> > do, that's a bug. This CL adds absolute paths to them, as far as I
> > understand.
>
> That's a design decision I was completely unaware of. Is there more
> documentation about it somewhere (I'm wondering, for example, if it's
> something ninja-on-Linux specific, independent of generator or what)?
>

It's a general goal, but on mac and win it's sadly made impossible by us
emulating xcode / msvs behavior, whose semantics force absolute paths in a
few cases.


> And how about the sysroot variables that contain absolute paths, which I
> modeled this change after?


Where's that at?


> I don't see how specifying an absolute path
> for the linker can cause any harm or be worse than relative paths, but I
> can try fixing it differently if that's the case.
>

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

[Raphael Kubo da Costa (rakuco), 2013-11-27 17:46:41]

>> That's a design decision I was completely unaware of. Is there more
>> documentation about it somewhere (I'm wondering, for example, if it's
>> something ninja-on-Linux specific, independent of generator or what)?
>
> It's a general goal, but on mac and win it's sadly made impossible by us
> emulating xcode / msvs behavior, whose semantics force absolute paths in a
> few cases.

Right; I wonder if there's a way to use some of those variables that gyp
sets by default to refer to paths outside the build dir then? In my case
I'm interested in the make generator for that (I'm guessing it'd require
changes in the generator code).

>> And how about the sysroot variables that contain absolute paths, which I
>> modeled this change after?
>
> Where's that at?

Scattered across build/common.gypi (look for "pwd -P").

>> I don't see how specifying an absolute path
>> for the linker can cause any harm or be worse than relative paths, but I
>> can try fixing it differently if that's the case.

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

[Nico, 2013-11-27 17:55:03]

On Wed, Nov 27, 2013 at 9:47 AM, Raphael Kubo da Costa <
raphael.kubo.da.costa@intel.com> wrote:

> >> That's a design decision I was completely unaware of. Is there more
> >> documentation about it somewhere (I'm wondering, for example, if it's
> >> something ninja-on-Linux specific, independent of generator or what)?
> >
> > It's a general goal, but on mac and win it's sadly made impossible by us
> > emulating xcode / msvs behavior, whose semantics force absolute paths in
> a
> > few cases.
>
> Right; I wonder if there's a way to use some of those variables that gyp
> sets by default to refer to paths outside the build dir then? In my case
> I'm interested in the make generator for that (I'm guessing it'd require
> changes in the generator code).
>
> >> And how about the sysroot variables that contain absolute paths, which I
> >> modeled this change after?
> >
> > Where's that at?
>
> Scattered across build/common.gypi (look for "pwd -P").
>

I've noticed this before. From what I can tell, it isn't used in a regular
linux build at least, but it's not a great idea either.

After the gyp bug is fixed, I think this CL is the only absolute path
that's used on linux. And as I said upthread, several code paths assume
that the build directory is 2 down from the source root.


>
> >> I don't see how specifying an absolute path
> >> for the linker can cause any harm or be worse than relative paths, but I
> >> can try fixing it differently if that's the case.
>

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

[Raphael Kubo da Costa (rakuco), 2013-11-27 18:47:29]

> I've noticed this before. From what I can tell, it isn't used in a regular
> linux build at least, but it's not a great idea either.
>
> After the gyp bug is fixed, I think this CL is the only absolute path
> that's used on linux. And as I said upthread, several code paths assume
> that the build directory is 2 down from the source root.

This takes us back to this then:

>>>> I don't see how specifying an absolute path for the linker can
>>>> cause any harm or be worse than relative paths, but I can try
>>>> fixing it differently if that's the case.
[...]
>> Right; I wonder if there's a way to use some of those variables that gyp
>> sets by default to refer to paths outside the build dir then? In my case
>> I'm interested in the make generator for that (I'm guessing it'd require
>> changes in the generator code).

Do you know how this could work, at least with make? I'm fine with this
not being supported on ninja, but everything is currently working with
make and losing that would be sad.

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

[Nico, 2013-11-27 19:04:47]

On Thu, Nov 28, 2013 at 3:48 AM, Raphael Kubo da Costa <
raphael.kubo.da.costa@intel.com> wrote:

> > I've noticed this before. From what I can tell, it isn't used in a
> regular
> > linux build at least, but it's not a great idea either.
> >
> > After the gyp bug is fixed, I think this CL is the only absolute path
> > that's used on linux. And as I said upthread, several code paths assume
> > that the build directory is 2 down from the source root.
>
> This takes us back to this then:
>
> >>>> I don't see how specifying an absolute path for the linker can
> >>>> cause any harm or be worse than relative paths, but I can try
> >>>> fixing it differently if that's the case.
> [...]
> >> Right; I wonder if there's a way to use some of those variables that gyp
> >> sets by default to refer to paths outside the build dir then? In my case
> >> I'm interested in the make generator for that (I'm guessing it'd require
> >> changes in the generator code).
>
> Do you know how this could work, at least with make? I'm fine with this
> not being supported on ninja, but everything is currently working with
> make and losing that would be sad.
>

I don't know.

So you're not interested in running tests, only in building them?

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

[Raphael Kubo da Costa (rakuco), 2013-11-27 19:08:30]

Nico Weber <thakis@chromium.org> writes:

> So you're not interested in running tests, only in building them?

I can build using the default build directory if I want to run the
tests.

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

[Nico, 2013-11-28 00:07:20]

Could you keep this around as a local diff?


On Wed, Nov 27, 2013 at 11:09 AM, Raphael Kubo da Costa <
raphael.kubo.da.costa@intel.com> wrote:

> Nico Weber <thakis@chromium.org> writes:
>
> > So you're not interested in running tests, only in building them?
>
> I can build using the default build directory if I want to run the
> tests.
>

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

[Nico, 2013-12-03 01:21:54]

On Wed, Nov 27, 2013 at 4:07 PM, Nico Weber <thakis@chromium.org> wrote:

> Could you keep this around as a local diff?
>

^ ping


>
>
> On Wed, Nov 27, 2013 at 11:09 AM, Raphael Kubo da Costa <
> raphael.kubo.da.costa@intel.com> wrote:
>
>> Nico Weber <thakis@chromium.org> writes:
>>
>> > So you're not interested in running tests, only in building them?
>>
>> I can build using the default build directory if I want to run the
>> tests.
>>
>
>

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

[Nico, 2013-12-03 19:39:52]

On Mon, Dec 2, 2013 at 5:21 PM, Nico Weber <thakis@chromium.org> wrote:

> On Wed, Nov 27, 2013 at 4:07 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> Could you keep this around as a local diff?
>>
>
> ^ ping
>

^ ping^2


>
>
>>
>>
>> On Wed, Nov 27, 2013 at 11:09 AM, Raphael Kubo da Costa <
>> raphael.kubo.da.costa@intel.com> wrote:
>>
>>> Nico Weber <thakis@chromium.org> writes:
>>>
>>> > So you're not interested in running tests, only in building them?
>>>
>>> I can build using the default build directory if I want to run the
>>> tests.
>>>
>>
>>
>

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

[Nico, 2013-12-05 05:12:06]

On Tue, Dec 3, 2013 at 11:39 AM, Nico Weber <thakis@chromium.org> wrote:

> On Mon, Dec 2, 2013 at 5:21 PM, Nico Weber <thakis@chromium.org> wrote:
>
>> On Wed, Nov 27, 2013 at 4:07 PM, Nico Weber <thakis@chromium.org> wrote:
>>
>>> Could you keep this around as a local diff?
>>>
>>
>> ^ ping
>>
>
> ^ ping^2
>

last ping


>
>
>>
>>
>>>
>>>
>>> On Wed, Nov 27, 2013 at 11:09 AM, Raphael Kubo da Costa <
>>> raphael.kubo.da.costa@intel.com> wrote:
>>>
>>>> Nico Weber <thakis@chromium.org> writes:
>>>>
>>>> > So you're not interested in running tests, only in building them?
>>>>
>>>> I can build using the default build directory if I want to run the
>>>> tests.
>>>>
>>>
>>>
>>
>

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

[Raphael Kubo da Costa (rakuco), 2013-12-09 20:21:19]

Nico Weber <thakis@chromium.org> writes:

> On Tue, Dec 3, 2013 at 11:39 AM, Nico Weber <thakis@chromium.org> wrote:
>
>> On Mon, Dec 2, 2013 at 5:21 PM, Nico Weber <thakis@chromium.org> wrote:
>>
>>> On Wed, Nov 27, 2013 at 4:07 PM, Nico Weber <thakis@chromium.org> wrote:
>>>
>>>> Could you keep this around as a local diff?
>>>
>>> ^ ping
>>>
>>
>> ^ ping^2
>
> last ping

I was on vacation last week. I certainly can keep this as a local diff;
we just try to deviate as little as possible from upstream, but
unfortunately this doesn't seem to be possible in this situation.

Feel free to revert this then, in the future I may try to give the gyp's
make generator some love and make the whole thing work there.

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