Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(278)

Issue 1556923002: clang: Makes builds with clang less dependent on absolute file path (Closed)

Created:
4 years, 11 months ago by Zachary Forman
Modified:
4 years, 11 months ago
Reviewers:
brettw, Nico, M-A Ruel, JF, mithro-old
CC:
chromium-reviews, Mark Seaborn, Jim Stichnoth
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

clang: Makes builds with clang less dependent on absolute file path [Note: Dependency claims are only valid on Linux builds] This change removes absolute file paths from several locations in produced binaries, reducing the total number of files containing absolute file paths from ~30k to ~4k. Specifically, this removes absolute paths from: .rodata (sysroot based) .debug_str, .debug_info (debug prefix based) .debug_line (debug prefix and sysroot based) .debug_info still contains (on Linux) 22 absolute paths, but as discussed below, this is most likely simple to resolve. This can be measured by using $ find out/Default/ -type f | grep -v ninja | xargs -I '{}' grep -l \ 'absolute-path-component' '{}' | wc -l Before: 30420 After: 3917 (https://gist.github.com/anonymous/fd870076c990fcf792c7) The remaining instances mostly originate from NaCL not having the flag enabled. Enabling requires updating the toolchain's version of clang to newer than release 3.8 Generated ninja files still have some absolute file paths. At the very least, they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This will be removed later. BUG=439949 Committed: https://crrev.com/fdca0741b22a4dddbf1808b8d0081651662b22c5 Cr-Commit-Position: refs/heads/master@{#370320}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Switch to "//." #

Total comments: 6

Patch Set 3 : Formatting change #

Patch Set 4 : Addresses issues #

Patch Set 5 : Corrects bad rebase_path #

Total comments: 2

Patch Set 6 : Consistency with Android and OSX/iOS #

Patch Set 7 : Reverting change on OSX and iOS #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -15 lines) Patch
M build/config/compiler/BUILD.gn View 1 2 1 chunk +8 lines, -0 lines 0 comments Download
M build/config/linux/pkg_config.gni View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M build/config/posix/BUILD.gn View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M build/config/sysroot.gni View 1 2 3 4 5 6 3 chunks +13 lines, -11 lines 0 comments Download
M printing/BUILD.gn View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download

Messages

Total messages: 46 (13 generated)
Zachary Forman
Hi! Could you please take a look at this CL? https://codereview.chromium.org/1556923002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1556923002/diff/1/build/config/compiler/BUILD.gn#newcode317 ...
4 years, 11 months ago (2016-01-05 03:21:29 UTC) #3
mithro-old
On 2016/01/05 03:21:29, Zachary Forman wrote: > Hi! Could you please take a look at ...
4 years, 11 months ago (2016-01-05 04:32:26 UTC) #4
Zachary Forman
Hi Marc, Here's that CL that I mentioned earlier - could you please take a ...
4 years, 11 months ago (2016-01-05 04:38:43 UTC) #6
Zachary Forman
Hi Nico, I've added you as a reviewer on this CL for two reasons: 1) ...
4 years, 11 months ago (2016-01-05 04:42:30 UTC) #8
Nico
replacing myself with brettw for the rebase_path question below. In general, I think this is ...
4 years, 11 months ago (2016-01-05 12:22:48 UTC) #10
M-A Ruel
On 2016/01/05 04:38:43, Zachary Forman wrote: > Hi Marc, > > Here's that CL that ...
4 years, 11 months ago (2016-01-05 14:39:44 UTC) #12
JF
The upgrade of LLVM for PNaCl is being worked on by Jim (CC'ed). The easiest ...
4 years, 11 months ago (2016-01-05 15:59:01 UTC) #14
brettw
lgtm https://codereview.chromium.org/1556923002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1556923002/diff/1/build/config/compiler/BUILD.gn#newcode317 build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") On 2016/01/05 03:21:29, Zachary Forman ...
4 years, 11 months ago (2016-01-05 19:10:12 UTC) #15
Zachary Forman
On 2016/01/05 at 15:59:01, jfb wrote: > The upgrade of LLVM for PNaCl is being ...
4 years, 11 months ago (2016-01-05 23:18:59 UTC) #16
Zachary Forman
On 2016/01/05 at 12:22:48, thakis wrote: > For porting this specific change to gyp: What ...
4 years, 11 months ago (2016-01-05 23:48:27 UTC) #17
Nico
gyp itself doesn't assume absolute paths. Stuff we do in gyp files might. I suppose ...
4 years, 11 months ago (2016-01-06 01:19:59 UTC) #18
JF
On 2016/01/05 23:18:59, Zachary Forman wrote: > On 2016/01/05 at 15:59:01, jfb wrote: > > ...
4 years, 11 months ago (2016-01-06 06:02:53 UTC) #19
brettw
Still LGTM. I clarified the rebase behavior in the docs in https://codereview.chromium.org/1563543002/ https://codereview.chromium.org/1556923002/diff/20001/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn ...
4 years, 11 months ago (2016-01-06 18:13:11 UTC) #20
brettw
https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni#newcode44 build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), Wait, I don't understand this line. ...
4 years, 11 months ago (2016-01-06 18:15:07 UTC) #21
Zachary Forman
https://codereview.chromium.org/1556923002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1556923002/diff/1/build/config/compiler/BUILD.gn#newcode317 build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") On 2016/01/05 at 19:10:11, brettw wrote: ...
4 years, 11 months ago (2016-01-06 23:20:50 UTC) #22
brettw
https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni#newcode44 build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), Semantically I don't think this line ...
4 years, 11 months ago (2016-01-07 19:22:22 UTC) #23
Zachary Forman
https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni#newcode44 build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), On 2016/01/07 at 19:22:22, brettw wrote: ...
4 years, 11 months ago (2016-01-07 22:54:27 UTC) #24
brettw
On 2016/01/07 22:54:27, Zachary Forman wrote: > https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni > File build/config/linux/pkg_config.gni (right): > > https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_config.gni#newcode44 ...
4 years, 11 months ago (2016-01-07 23:24:48 UTC) #25
brettw
My suggested change will involve updating the existing sysroot uses to rebase to be relative ...
4 years, 11 months ago (2016-01-07 23:26:25 UTC) #26
Zachary Forman
On 2016/01/07 at 23:24:48, brettw wrote: > On 2016/01/07 22:54:27, Zachary Forman wrote: > > ...
4 years, 11 months ago (2016-01-08 00:29:55 UTC) #27
brettw
https://codereview.chromium.org/1556923002/diff/80001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/1556923002/diff/80001/build/config/sysroot.gni#newcode22 build/config/sysroot.gni:22: sysroot = rebase_path("$android_ndk_root/$x86_android_sysroot_subdir") We should really be consistent about ...
4 years, 11 months ago (2016-01-08 18:22:56 UTC) #29
Zachary Forman
https://codereview.chromium.org/1556923002/diff/80001/build/config/sysroot.gni File build/config/sysroot.gni (right): https://codereview.chromium.org/1556923002/diff/80001/build/config/sysroot.gni#newcode22 build/config/sysroot.gni:22: sysroot = rebase_path("$android_ndk_root/$x86_android_sysroot_subdir") On 2016/01/08 at 18:22:56, brettw wrote: ...
4 years, 11 months ago (2016-01-11 08:21:00 UTC) #30
Nico
can you try a local build with this & goma on mac and check if ...
4 years, 11 months ago (2016-01-11 15:55:10 UTC) #31
Zachary Forman
On 2016/01/11 at 15:55:10, thakis wrote: > can you try a local build with this ...
4 years, 11 months ago (2016-01-11 23:05:26 UTC) #32
Nico
What's the fallback reason if you clicked on a failed job? How does the compile ...
4 years, 11 months ago (2016-01-12 02:57:44 UTC) #33
Zachary Forman
On 2016/01/12 at 02:57:44, thakis wrote: > What's the fallback reason if you clicked on ...
4 years, 11 months ago (2016-01-12 03:24:33 UTC) #34
Nico
On 2016/01/12 03:24:33, Zachary Forman wrote: > On 2016/01/12 at 02:57:44, thakis wrote: > > ...
4 years, 11 months ago (2016-01-12 03:29:40 UTC) #35
Zachary Forman
On 2016/01/12 at 03:29:40, thakis wrote: > On 2016/01/12 03:24:33, Zachary Forman wrote: > > ...
4 years, 11 months ago (2016-01-12 06:56:13 UTC) #36
Nico
On 2016/01/12 06:56:13, Zachary Forman wrote: > On 2016/01/12 at 03:29:40, thakis wrote: > > ...
4 years, 11 months ago (2016-01-12 15:33:33 UTC) #37
mithro-old
Shall we submit this then? On 13 January 2016 at 02:33, <thakis@chromium.org> wrote: > On ...
4 years, 11 months ago (2016-01-13 05:38:38 UTC) #38
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1556923002/110001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1556923002/110001
4 years, 11 months ago (2016-01-20 02:23:54 UTC) #42
commit-bot: I haz the power
Committed patchset #7 (id:110001)
4 years, 11 months ago (2016-01-20 05:19:27 UTC) #44
commit-bot: I haz the power
4 years, 11 months ago (2016-01-20 05:20:37 UTC) #46
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/fdca0741b22a4dddbf1808b8d0081651662b22c5
Cr-Commit-Position: refs/heads/master@{#370320}

Powered by Google App Engine
This is Rietveld 408576698