Ensure that builders that use goma strip absolute paths by default.

Ensure that builders that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790
Review-Url: https://codereview.chromium.org/2837863005
Cr-Commit-Position: refs/heads/master@{#467574}
Committed: https://chromium.googlesource.com/chromium/src/+/050dbe550d14e99ad4e83093d4f4e79642b9edc3

[Dirk Pranke, 2017-04-25 23:05:44]

Description was changed from

==========
Ensure that builder that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790
==========

to

==========
Ensure that builder that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790
==========

[Dirk Pranke, 2017-04-25 23:05:54]

The CQ bit was checked by dpranke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 23:06:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-04-25 23:14:00]

I'm not a fan of bots and devs using different flags.

After looking through the history of this flag, I'm not a fan of
https://codereview.chromium.org/2690943007/ which I hadn't seen before :-/

lgtm since it's better than before, but imo
https://codereview.chromium.org/2690943007/ is a mistake.

[Dirk Pranke, 2017-04-25 23:16:35]

On 2017/04/25 23:14:00, Nico wrote:
> I'm not a fan of bots and devs using different flags.
> 
> After looking through the history of this flag, I'm not a fan of
> https://codereview.chromium.org/2690943007/ which I hadn't seen before :-/
> 
> lgtm since it's better than before, but imo
> https://codereview.chromium.org/2690943007/ is a mistake.

I have many of the same misgivings, but on the other hand the debugger errors
are inscrutable
if the paths are missing. I think I'd like to solve this by having "run" scripts
that can make sure
you're in the right directory before launching the binary, but I think this is
probably the 
right compromise in the meantime.

[commit-bot: I haz the power, 2017-04-26 01:21:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 01:21:24]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2017-04-26 01:28:17]

I'll try to land this tomorrow morning, when I have more time to keep an eye on
the tree.

[jochen (gone - plz use gerrit), 2017-04-26 19:59:41]

lgtm

I'm open to better fixes for the original issue (not being able to use gdb), but
I don't think we should keep compilation flags in the default settings that make
development impossible, so I'd argue that the real mistake was landing support
for stripping the path before figuring out how to make gdb work with it

[Dirk Pranke, 2017-04-26 20:02:18]

On 2017/04/26 19:59:41, jochen (slow until May 2) wrote:
> I'm open to better fixes for the original issue (not being able to use gdb),
but
> I don't think we should keep compilation flags in the default settings that
make
> development impossible, so I'd argue that the real mistake was landing support
> for stripping the path before figuring out how to make gdb work with it

Didn't it work fine if you ran the executable from the build directory? That
hardly
sounds "impossible".

[jochen (gone - plz use gerrit), 2017-04-26 20:07:08]

On 2017/04/26 at 20:02:18, dpranke wrote:
> On 2017/04/26 19:59:41, jochen (slow until May 2) wrote:
> > I'm open to better fixes for the original issue (not being able to use gdb),
but
> > I don't think we should keep compilation flags in the default settings that
make
> > development impossible, so I'd argue that the real mistake was landing
support
> > for stripping the path before figuring out how to make gdb work with it
> 
> Didn't it work fine if you ran the executable from the build directory? That
hardly
> sounds "impossible".

In practice it was. It's not common for devs to run gdb in the build/ directory.

On the flip side, if we want to increase the cache hit rate for goma, we could
also just configure the bots to all use the same path for checkouts.

[Nico, 2017-04-26 20:10:35]

On Wed, Apr 26, 2017 at 4:07 PM, <jochen@chromium.org> wrote:

> On 2017/04/26 at 20:02:18, dpranke wrote:
> > On 2017/04/26 19:59:41, jochen (slow until May 2) wrote:
> > > I'm open to better fixes for the original issue (not being able to use
> gdb),
> but
> > > I don't think we should keep compilation flags in the default settings
> that
> make
> > > development impossible, so I'd argue that the real mistake was landing
> support
> > > for stripping the path before figuring out how to make gdb work with it
> >
> > Didn't it work fine if you ran the executable from the build directory?
> That
> hardly
> > sounds "impossible".
>
> In practice it was. It's not common for devs to run gdb in the build/
> directory.
>
> On the flip side, if we want to increase the cache hit rate for goma, we
> could
> also just configure the bots to all use the same path for checkouts.
>

But then a) bots and devs still don't share goma cache b) it doesn't help
us with having repeatable, deterministic builds unless you say "our builds
are only repeatable if you use a fixed path for your checkout"


>
> https://codereview.chromium.org/2837863005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jochen (gone - plz use gerrit), 2017-04-26 20:25:05]

On 2017/04/26 at 20:10:35, thakis wrote:
> On Wed, Apr 26, 2017 at 4:07 PM, <jochen@chromium.org> wrote:
> 
> > On 2017/04/26 at 20:02:18, dpranke wrote:
> > > On 2017/04/26 19:59:41, jochen (slow until May 2) wrote:
> > > > I'm open to better fixes for the original issue (not being able to use
> > gdb),
> > but
> > > > I don't think we should keep compilation flags in the default settings
> > that
> > make
> > > > development impossible, so I'd argue that the real mistake was landing
> > support
> > > > for stripping the path before figuring out how to make gdb work with it
> > >
> > > Didn't it work fine if you ran the executable from the build directory?
> > That
> > hardly
> > > sounds "impossible".
> >
> > In practice it was. It's not common for devs to run gdb in the build/
> > directory.
> >
> > On the flip side, if we want to increase the cache hit rate for goma, we
> > could
> > also just configure the bots to all use the same path for checkouts.
> >
> 
> But then a) bots and devs still don't share goma cache b) it doesn't help
> us with having repeatable, deterministic builds unless you say "our builds
> are only repeatable if you use a fixed path for your checkout"

right, so that's why I'd argue that we should go back to the original patch and
make sure it works with gdb *and* for the bots, not just one

[Dirk Pranke, 2017-04-26 20:38:44]

On 2017/04/26 20:25:05, jochen (slow until May 2) wrote:
> right, so that's why I'd argue that we should go back to the original patch
and
> make sure it works with gdb *and* for the bots, not just one

We do not want the bots to have to use a fixed path for the checkout, and at 
least from my understanding of things, it's not obvious that it's possible to
have something that works for both gdb and the bots from just changes 
to build files and compiler flags.

If you see a way to make that work, then I agree that we should do that instead.

[jochen (gone - plz use gerrit), 2017-04-26 20:42:04]

On 2017/04/26 at 20:38:44, dpranke wrote:
> On 2017/04/26 20:25:05, jochen (slow until May 2) wrote:
> > right, so that's why I'd argue that we should go back to the original patch
and
> > make sure it works with gdb *and* for the bots, not just one
> 
> We do not want the bots to have to use a fixed path for the checkout, and at 
> least from my understanding of things, it's not obvious that it's possible to
> have something that works for both gdb and the bots from just changes 
> to build files and compiler flags.
> 
> If you see a way to make that work, then I agree that we should do that
instead.

I don't know of a way to make that work - I didn't know of the existence of this
compiler flag either. I would have wanted to original author to take developers
into account before landing this :/

[Dirk Pranke, 2017-04-26 23:21:44]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2017-04-26 23:22:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dirk Pranke, 2017-04-27 02:10:55]

Description was changed from

==========
Ensure that builder that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790
==========

to

==========
Ensure that builders that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790
==========

[commit-bot: I haz the power, 2017-04-27 03:58:32]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493248904226710, "parent_rev":
"9c12ed217b0f67085d1ec83ab3eabdabe72c40da", "commit_rev":
"050dbe550d14e99ad4e83093d4f4e79642b9edc3"}

[commit-bot: I haz the power, 2017-04-27 03:58:43]

Description was changed from

==========
Ensure that builders that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790
==========

to

==========
Ensure that builders that use goma strip absolute paths by default.

This CL tweaks the MB configs so that any builder that is using goma
also sets strip_absolute_paths_from_debug_symbols_by_default=true.
This helps ensure that we get the best goma cache hit rate possible.
We don't just make the default equal to use_goma=true because doing
so may make it harder for devs to debug binaries, and we want devs
to be able to both debug binaries and use goma by default; the
tradeoff is that their builds are slightly slower.

R=thakis@chromium.org, jochen@chromium.org
BUG=603286, 712790

Review-Url: https://codereview.chromium.org/2837863005
Cr-Commit-Position: refs/heads/master@{#467574}
Committed:
https://chromium.googlesource.com/chromium/src/+/050dbe550d14e99ad4e83093d4f4...
==========

[commit-bot: I haz the power, 2017-04-27 03:58:44]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/050dbe550d14e99ad4e83093d4f4...

[Dirk Pranke, 2017-04-27 20:20:32]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2843403004/ by dpranke@chromium.org.

The reason for reverting is: Reverting in case this is related to the goma
errors in crbug.om/716089.