Chromium Code Reviews
|
|
Chromium Code Reviews
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 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. |
Descriptionclang: 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 #
Messages
Total messages: 46 (13 generated)
Description was changed from
==========
clang: Makes builds with clang less depndent on absolute file path
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.
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
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
Ninja files still have some absolute file paths.
BUG=439949
==========
to
==========
clang: Makes builds with clang less dependent on absolute file path
[Note: Dependency claims are only shown 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
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
My reason for believing this can be seen in this output:
http://pastebin.com/raw/Gj3mLrgP.
Generated ninja files still have some absolute file path - at the very least,
they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This would be nice to
remove later, but removing it from binaries is more important.
BUG=439949
==========
zforman@google.com changed reviewers: + mithro@mithis.com
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... build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") Is there a GN builtin or otherwise that allows us to get the root source directory? I would use "//", but this has the unfortunate effect of giving a rebased path that ends with a '/'. As some symbols include only the directory name (no trailing slash), having a trailing slash in the prefix map misses some cases.
On 2016/01/05 03:21:29, Zachary Forman wrote: > 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... > build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") > Is there a GN builtin or otherwise that allows us to get the root source > directory? I would use "//", but this has the unfortunate effect of giving a > rebased path that ends with a '/'. As some symbols include only the directory > name (no trailing slash), having a trailing slash in the prefix map misses some > cases. I'm surprised that this is all that is needed to get the result you have. Great work! LGTM - will need the GN experts to really check it though.
zforman@google.com changed reviewers: + maruel@chromium.org
Hi Marc, Here's that CL that I mentioned earlier - could you please take a look? Thanks, Zac
zforman@google.com changed reviewers: + thakis@chromium.org
Hi Nico, I've added you as a reviewer on this CL for two reasons: 1) I've heard you're the owner of the sysroot stuff done in chromium, which this change interacts with. 2) This is a change to .gn files that greatly reduces the effect of absolute file paths on the build. I'd like to do something similar to the .gyp files (so they're consistent), and was wondering if you had any suggestions of how to do that. Cheers, Zac
thakis@chromium.org changed reviewers: + brettw@chromium.org
replacing myself with brettw for the rebase_path question below. In general, I think this is a great change. I tried pushing against absolute paths in generated .ninja files for a while, but on Android it turned out to be difficult because android used to use ant for bits of its java build (it uses it way less now, but last I looked it was still used for some resource bundling), and ant runs in a different working directory and gyp doesn't have a nice rebase_path() function like gn does. (see the `pwd` calls in build/common.gypi) For porting this specific change to gyp: What issues are you running into? 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... build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") On 2016/01/05 03:21:29, Zachary Forman wrote: > Is there a GN builtin or otherwise that allows us to get the root source > directory? I would use "//", but this has the unfortunate effect of giving a > rebased path that ends with a '/'. As some symbols include only the directory > name (no trailing slash), having a trailing slash in the prefix map misses some > cases. ^ brettw
Description was changed from
==========
clang: Makes builds with clang less dependent on absolute file path
[Note: Dependency claims are only shown 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
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
My reason for believing this can be seen in this output:
http://pastebin.com/raw/Gj3mLrgP.
Generated ninja files still have some absolute file path - at the very least,
they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This would be nice to
remove later, but removing it from binaries is more important.
BUG=439949
==========
to
==========
clang: Makes builds with clang less dependent on absolute file path
[Note: Dependency claims are only shown 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
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
My reason for believing this can be seen in this output:
http://pastebin.com/raw/Gj3mLrgP.
Generated ninja files still have some absolute file path - at the very least,
they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This would be nice to
remove later, but removing it from binaries is more important.
BUG=439949
==========
On 2016/01/05 04:38:43, Zachary Forman wrote: > Hi Marc, > > Here's that CL that I mentioned earlier - could you please take a look? > > Thanks, > Zac Thanks! I'm not qualified to review the change itself, I'll let Nico and Brett do this. cc'ed J-F and Mark about LLVM toolchain for NaCl as they worked on the update to 3.5 a year ago and could give insight on the work required to update to 3.8.
jfb@chromium.org changed reviewers: + jfb@chromium.org
The upgrade of LLVM for PNaCl is being worked on by Jim (CC'ed). The easiest / fastest thing would be to figure out which upstream change added that option, and cherry-pick it into PNaCl. Has anyone looked at which patch that was?
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... build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") On 2016/01/05 03:21:29, Zachary Forman wrote: > Is there a GN builtin or otherwise that allows us to get the root source > directory? I would use "//", but this has the unfortunate effect of giving a > rebased path that ends with a '/'. As some symbols include only the directory > name (no trailing slash), having a trailing slash in the prefix map misses some > cases. I think you can do: rebase_path("//.") The thing happening here is that GN tries to match whether the input ends in a slash or not so that concatenating paths works the same before and after rebasing, but for the root dir this is ambiguous.
On 2016/01/05 at 15:59:01, jfb wrote: > The upgrade of LLVM for PNaCl is being worked on by Jim (CC'ed). The easiest / fastest thing would be to figure out which upstream change added that option, and cherry-pick it into PNaCl. Has anyone looked at which patch that was? The subset of related patches (option + its tests) are: https://github.com/llvm-mirror/clang/commit/cc4221bf161ae46738eb71778fedd36e3... https://github.com/llvm-mirror/clang/commit/58e39e90e10264def54448f5f9d557c73... https://github.com/llvm-mirror/clang/commit/113e8e1cef7b507a7d698349eac6c6332...
On 2016/01/05 at 12:22:48, thakis wrote: > For porting this specific change to gyp: What issues are you running into? The -fdebug-prefix-map change is relatively simple - the main issue I've had is that GYP seems to react poorly to a non absolute path sysroot - i.e. ... cat: ../../build/linux/debian_wheezy_amd64-sysroot/etc/ld.so.conf.d/x86_64-linux-gnu.conf: No such file or directory cat: ../../build/linux/debian_wheezy_amd64-sysroot/etc/ld.so.conf.d/zz_hack.conf: No such file or directory gyp: Call to '../../build/linux/sysroot_ld_path.sh ../../build/linux/debian_wheezy_amd64-sysroot' returned exit status 0 while in /path/to/my/chromium-1/src/build/sanitizers/sanitizers.gyp. cat: ../../build/linux/debian_wheezy_amd64-sysroot/etc/ld.so.conf.d/x86_64-linux-gnu.conf: No such file or directory cat: ../../build/linux/debian_wheezy_amd64-sysroot/etc/ld.so.conf.d/zz_hack.conf: No such file or directory ... I don't see a super obvious way to resolve this (perhaps because I'm not sufficiently familiar with how gyp works / what it does) and am hopeful that you know something I don't about where/why GYP assumes absolute paths.
gyp itself doesn't assume absolute paths. Stuff we do in gyp files might. I suppose something doing the sysroot setup in some .gyp file currently does assume an absolute path. In general, gyp runs actions with the pwd equal to the directory the .gyp file defining the action is in, and runs compiles with a cwd of out/Release. <(DEPTH) can be used to get from the current action dir to the source root.
On 2016/01/05 23:18:59, Zachary Forman wrote: > On 2016/01/05 at 15:59:01, jfb wrote: > > The upgrade of LLVM for PNaCl is being worked on by Jim (CC'ed). The easiest / > fastest thing would be to figure out which upstream change added that option, > and cherry-pick it into PNaCl. Has anyone looked at which patch that was? > > The subset of related patches (option + its tests) are: > https://github.com/llvm-mirror/clang/commit/cc4221bf161ae46738eb71778fedd36e3... > https://github.com/llvm-mirror/clang/commit/58e39e90e10264def54448f5f9d557c73... > https://github.com/llvm-mirror/clang/commit/113e8e1cef7b507a7d698349eac6c6332... Thanks, I filled the following bug: https://bugs.chromium.org/p/nativeclient/issues/detail?id=4348
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/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:314: # Currently disabled for nacl since its toolchain is old and doesn't have this flag. Long line
https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), Wait, I don't understand this line. Why the ".."?
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... build/config/compiler/BUILD.gn:317: absolute_path = rebase_path("../../..") On 2016/01/05 at 19:10:11, brettw wrote: > On 2016/01/05 03:21:29, Zachary Forman wrote: > > Is there a GN builtin or otherwise that allows us to get the root source > > directory? I would use "//", but this has the unfortunate effect of giving a > > rebased path that ends with a '/'. As some symbols include only the directory > > name (no trailing slash), having a trailing slash in the prefix map misses some > > cases. > > I think you can do: > rebase_path("//.") > The thing happening here is that GN tries to match whether the input ends in a slash or not so that concatenating paths works the same before and after rebasing, but for the root dir this is ambiguous. This worked - thanks! https://codereview.chromium.org/1556923002/diff/20001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/compiler/B... build/config/compiler/BUILD.gn:314: # Currently disabled for nacl since its toolchain is old and doesn't have this flag. On 2016/01/06 18:13:11, brettw wrote: > Long line Done. https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), On 2016/01/06 at 18:15:07, brettw wrote: > Wait, I don't understand this line. Why the ".."? Without the .., you get output like "-I../../build/build/linux/debian_wheezy_amd64-sysroot/usr/include/glib-2.0" - note the "build/build".
https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), Semantically I don't think this line makes sense, and if it's working for you I suspect it's a coincidence. Sysroot is an absolute path. This line says "take the absolute path and make it relative to the current directory ("build/config/linux") assuming that the source is in the the previous directory ("build/config")." What directory are you specifically trying to pass to the -s command, and where should it be expressed relative to?
https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... File build/config/linux/pkg_config.gni (right): https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), On 2016/01/07 at 19:22:22, brettw wrote: > Semantically I don't think this line makes sense, and if it's working for you I suspect it's a coincidence. > > Sysroot is an absolute path. This line says "take the absolute path and make it relative to the current directory ("build/config/linux") assuming that the source is in the the previous directory ("build/config")." > > What directory are you specifically trying to pass to the -s command, and where should it be expressed relative to? I don't think this is the case. To start with,the point of this change is that we're making sysroots relative paths, and this change converts a relative path sysroot from the output directory to the correct absolute path. print("sysroot = $sysroot") print("rebased = " + rebase_path(sysroot, "", "..")) print("rebased w/o .. = " + rebase_path(sysroot, "", "..")) -> sysroot = ../../build/linux/debian_wheezy_amd64-sysroot rebased = /usr/local/google/home/zforman/chromium-1/src/build/linux/debian_wheezy_amd64-sysroot rebased w/o .. = /usr/local/google/home/zforman/chromium-1/src/build/build/linux/debian_wheezy_amd64-sysroot What this is doing is then taking ../../build/... and rebasing it to an absolute path from. This file is located in build/config/linux/ - therefore we absolutify (build/config/linux/../)(../../build/linux/debian_wheezy_amd64-sysroot) -> build/linux/debian_wheezy_amd64-sysroot.
On 2016/01/07 22:54:27, Zachary Forman wrote: > https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... > File build/config/linux/pkg_config.gni (right): > > https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... > build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), > On 2016/01/07 at 19:22:22, brettw wrote: > > Semantically I don't think this line makes sense, and if it's working for you > I suspect it's a coincidence. > > > > Sysroot is an absolute path. This line says "take the absolute path and make > it relative to the current directory ("build/config/linux") assuming that the > source is in the the previous directory ("build/config")." > > > > What directory are you specifically trying to pass to the -s command, and > where should it be expressed relative to? > > > I don't think this is the case. To start with,the point of this change is that > we're making sysroots relative paths, and this change converts a relative path > sysroot from the output directory to the correct absolute path. > > print("sysroot = $sysroot") > print("rebased = " + rebase_path(sysroot, "", "..")) > print("rebased w/o .. = " + rebase_path(sysroot, "", "..")) > -> > sysroot = ../../build/linux/debian_wheezy_amd64-sysroot > rebased = > /usr/local/google/home/zforman/chromium-1/src/build/linux/debian_wheezy_amd64-sysroot > rebased w/o .. = > /usr/local/google/home/zforman/chromium-1/src/build/build/linux/debian_wheezy_amd64-sysroot > > What this is doing is then taking ../../build/... and rebasing it to an absolute > path from. > > This file is located in build/config/linux/ - therefore we absolutify > (build/config/linux/../)(../../build/linux/debian_wheezy_amd64-sysroot) -> > build/linux/debian_wheezy_amd64-sysroot. I see where things aren't matching. You rebased your sysroot to be relative to the root_build_dir. Then you're treating that as being relative to build/config which happens to be at the same level relative to the root directory. To make your call correct, you would use: rebase_path(sysroot, "", root_build_dir) However, I don't think that we should do that. We should try to avoid having global variables that are relative to random directories. I think the existing absolute path sysroot was also a bad design. Instead, we should decide what each use of that directory wants it to be relative to at the time of the call. The global sysroot variable should be source-absolute (so "//build/linux/debian_wheezy_amd64-sysroot") and then in pkg_config you just say rebase_path(sysroot) to make it absolute.
My suggested change will involve updating the existing sysroot uses to rebase to be relative to the root_build_dir.
On 2016/01/07 at 23:24:48, brettw wrote: > On 2016/01/07 22:54:27, Zachary Forman wrote: > > https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... > > File build/config/linux/pkg_config.gni (right): > > > > https://codereview.chromium.org/1556923002/diff/20001/build/config/linux/pkg_... > > build/config/linux/pkg_config.gni:44: rebase_path(sysroot, "", ".."), > > On 2016/01/07 at 19:22:22, brettw wrote: > > > Semantically I don't think this line makes sense, and if it's working for you > > I suspect it's a coincidence. > > > > > > Sysroot is an absolute path. This line says "take the absolute path and make > > it relative to the current directory ("build/config/linux") assuming that the > > source is in the the previous directory ("build/config")." > > > > > > What directory are you specifically trying to pass to the -s command, and > > where should it be expressed relative to? > > > > > > I don't think this is the case. To start with,the point of this change is that > > we're making sysroots relative paths, and this change converts a relative path > > sysroot from the output directory to the correct absolute path. > > > > print("sysroot = $sysroot") > > print("rebased = " + rebase_path(sysroot, "", "..")) > > print("rebased w/o .. = " + rebase_path(sysroot, "", "..")) > > -> > > sysroot = ../../build/linux/debian_wheezy_amd64-sysroot > > rebased = > > /usr/local/google/home/zforman/chromium-1/src/build/linux/debian_wheezy_amd64-sysroot > > rebased w/o .. = > > /usr/local/google/home/zforman/chromium-1/src/build/build/linux/debian_wheezy_amd64-sysroot > > > > What this is doing is then taking ../../build/... and rebasing it to an absolute > > path from. > > > > This file is located in build/config/linux/ - therefore we absolutify > > (build/config/linux/../)(../../build/linux/debian_wheezy_amd64-sysroot) -> > > build/linux/debian_wheezy_amd64-sysroot. > > I see where things aren't matching. > > You rebased your sysroot to be relative to the root_build_dir. Then you're treating that as being relative to build/config which happens to be at the same level relative to the root directory. To make your call correct, you would use: > rebase_path(sysroot, "", root_build_dir) > > However, I don't think that we should do that. We should try to avoid having global variables that are relative to random directories. I think the existing absolute path sysroot was also a bad design. Instead, we should decide what each use of that directory wants it to be relative to at the time of the call. The global sysroot variable should be source-absolute (so "//build/linux/debian_wheezy_amd64-sysroot") and then in pkg_config you just say rebase_path(sysroot) to make it absolute. Ah! Makes sense. I've uploaded a new changelist that does this, PTAL.
Description was changed from
==========
clang: Makes builds with clang less dependent on absolute file path
[Note: Dependency claims are only shown 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
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
My reason for believing this can be seen in this output:
http://pastebin.com/raw/Gj3mLrgP.
Generated ninja files still have some absolute file path - at the very least,
they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This would be nice to
remove later, but removing it from binaries is more important.
BUG=439949
==========
to
==========
clang: Makes builds with clang less dependent on absolute file path
[Note: Dependency claims are only shown 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)
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
My reason for believing this can be seen in this output:
http://pastebin.com/raw/Gj3mLrgP.
Generated ninja files still have some absolute file path - at the very least,
they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This would be nice to
remove later, but removing it from binaries is more important.
BUG=439949
==========
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.gn... build/config/sysroot.gni:22: sysroot = rebase_path("$android_ndk_root/$x86_android_sysroot_subdir") We should really be consistent about how sysroots are represented. With the current patch, Android, Mac, and iOS sysroot paths are absolute, and Linux ones aren't. We should change the definition of what the sysroot is for everything at once.
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.gn... 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: > We should really be consistent about how sysroots are represented. With the current patch, Android, Mac, and iOS sysroot paths are absolute, and Linux ones aren't. We should change the definition of what the sysroot is for everything at once. I've applied this change, but it seems to have caused a mac builder to become so slow as to consistently time out.
can you try a local build with this & goma on mac and check if localhost:8088 shows lots of local fallbacks after this change?
On 2016/01/11 at 15:55:10, thakis wrote: > can you try a local build with this & goma on mac and check if localhost:8088 shows lots of local fallbacks after this change? Yes and yes - tried a local build on a macbook pro with goma, and observed basically the same thing as the buildbot - fast build up to a point, then pretty much exclusively local fallback.
What's the fallback reason if you clicked on a failed job? How does the compile command for a fallback job change with this patch? What's the command before and after this?
On 2016/01/12 at 02:57:44, thakis wrote: > What's the fallback reason if you clicked on a failed job? How does the compile command for a fallback job change with this patch? What's the command before and after this? We tested locally with a Mac developer here and saw the same issue with this patch on a Gyp build -- which doesn't make any sense? How can this patch be effecting the output of gyp_chromium? When looking at the flags in goma, the compile command doesn't seem to have changed, the isysroot for the Mac SDK is identical with and without this patch (which supports the above assertion) -- yet we see the following errors in the log files; E0112 10:11:18.466936 12333056 compile_task.cc:4854] Task:14519 Required file not on goma cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHTTPMessage.h E0112 10:11:18.466963 12333056 compile_task.cc:4854] Task:14519 Required file not on goma cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHost.h E0112 10:11:18.466987 12333056 compile_task.cc:4854] Task:14519 Required file not on goma cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFNetServices.h Looking at this it also seems to not make sense to use a relative sysroot on Mac too?
On 2016/01/12 03:24:33, Zachary Forman wrote: > On 2016/01/12 at 02:57:44, thakis wrote: > > What's the fallback reason if you clicked on a failed job? How does the > compile command for a fallback job change with this patch? What's the command > before and after this? > > We tested locally with a Mac developer here and saw the same issue with this > patch on a Gyp build -- which doesn't make any sense? How can this patch be > effecting the output of gyp_chromium? Huh! Are you sure you held it right? > When looking at the flags in goma, the compile command doesn't seem to have > changed, the isysroot for the Mac SDK is identical with and without this patch > (which supports the above assertion) -- yet we see the following errors in the > log files; > > E0112 10:11:18.466936 12333056 compile_task.cc:4854] Task:14519 Required file > not on goma > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHTTPMessage.h > E0112 10:11:18.466963 12333056 compile_task.cc:4854] Task:14519 Required file > not on goma > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHost.h > E0112 10:11:18.466987 12333056 compile_task.cc:4854] Task:14519 Required file > not on goma > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFNetServices.h > > Looking at this it also seems to not make sense to use a relative sysroot on Mac > too? Did you restart goma in between? I think the web interface never gets rid of the "error local fallback" box, so maybe that was still around from a prior gn build attempt?
On 2016/01/12 at 03:29:40, thakis wrote: > On 2016/01/12 03:24:33, Zachary Forman wrote: > > On 2016/01/12 at 02:57:44, thakis wrote: > > > What's the fallback reason if you clicked on a failed job? How does the > > compile command for a fallback job change with this patch? What's the command > > before and after this? > > > > We tested locally with a Mac developer here and saw the same issue with this > > patch on a Gyp build -- which doesn't make any sense? How can this patch be > > effecting the output of gyp_chromium? > > Huh! Are you sure you held it right? > > > When looking at the flags in goma, the compile command doesn't seem to have > > changed, the isysroot for the Mac SDK is identical with and without this patch > > (which supports the above assertion) -- yet we see the following errors in the > > log files; > > > > E0112 10:11:18.466936 12333056 compile_task.cc:4854] Task:14519 Required file > > not on goma > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHTTPMessage.h > > E0112 10:11:18.466963 12333056 compile_task.cc:4854] Task:14519 Required file > > not on goma > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHost.h > > E0112 10:11:18.466987 12333056 compile_task.cc:4854] Task:14519 Required file > > not on goma > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFNetServices.h > > > > Looking at this it also seems to not make sense to use a relative sysroot on Mac > > too? > > Did you restart goma in between? I think the web interface never gets rid of the "error local fallback" box, so maybe that was still around from a prior gn build attempt? Yes, we made sure that the errors were from the active run. Regardless, my suggestion is that this /isn't/ the right way to go about handling the sysroot on OSX - it seems to be an absolute path located outside of the chromium repo. I propose that we submit this change without altering the OSX sysroot, and content ourselves with at least fixing it on Linux/Android. I'll still investigate the situation on OSX, and why it fails on goma builds, in the hopes of fixing it, but for now it'd be nice to at least solve it for linux/android.
On 2016/01/12 06:56:13, Zachary Forman wrote: > On 2016/01/12 at 03:29:40, thakis wrote: > > On 2016/01/12 03:24:33, Zachary Forman wrote: > > > On 2016/01/12 at 02:57:44, thakis wrote: > > > > What's the fallback reason if you clicked on a failed job? How does the > > > compile command for a fallback job change with this patch? What's the > command > > > before and after this? > > > > > > We tested locally with a Mac developer here and saw the same issue with this > > > patch on a Gyp build -- which doesn't make any sense? How can this patch be > > > effecting the output of gyp_chromium? > > > > Huh! Are you sure you held it right? > > > > > When looking at the flags in goma, the compile command doesn't seem to have > > > changed, the isysroot for the Mac SDK is identical with and without this > patch > > > (which supports the above assertion) -- yet we see the following errors in > the > > > log files; > > > > > > E0112 10:11:18.466936 12333056 compile_task.cc:4854] Task:14519 Required > file > > > not on goma > > > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHTTPMessage.h > > > E0112 10:11:18.466963 12333056 compile_task.cc:4854] Task:14519 Required > file > > > not on goma > > > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHost.h > > > E0112 10:11:18.466987 12333056 compile_task.cc:4854] Task:14519 Required > file > > > not on goma > > > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFNetServices.h > > > > > > Looking at this it also seems to not make sense to use a relative sysroot on > Mac > > > too? > > > > Did you restart goma in between? I think the web interface never gets rid of > the "error local fallback" box, so maybe that was still around from a prior gn > build attempt? > > Yes, we made sure that the errors were from the active run. > > Regardless, my suggestion is that this /isn't/ the right way to go about > handling the sysroot on OSX - it seems to be an absolute path located outside of > the chromium repo. Yes, that sounds right to me. We're working on possibly eventually having a sysroot downloaded on OS X for googlers, like we do on Windows, but it'll very likely be outside of src/ even then (like it is on Windows). But currently, the OS X sysroot is always in /Applications/Xcode.app/... > > I propose that we submit this change without altering the OSX sysroot, and > content ourselves with at least fixing it on Linux/Android. I'll still > investigate the situation on OSX, and why it fails on goma builds, in the hopes > of fixing it, but for now it'd be nice to at least solve it for linux/android.
Shall we submit this then? On 13 January 2016 at 02:33, <thakis@chromium.org> wrote: > On 2016/01/12 06:56:13, Zachary Forman wrote: > >> On 2016/01/12 at 03:29:40, thakis wrote: >> > On 2016/01/12 03:24:33, Zachary Forman wrote: >> > > On 2016/01/12 at 02:57:44, thakis wrote: >> > > > What's the fallback reason if you clicked on a failed job? How does >> the >> > > compile command for a fallback job change with this patch? What's the >> command >> > > before and after this? >> > > >> > > We tested locally with a Mac developer here and saw the same issue >> with >> > this > >> > > patch on a Gyp build -- which doesn't make any sense? How can this >> patch >> > be > >> > > effecting the output of gyp_chromium? >> > >> > Huh! Are you sure you held it right? >> > >> > > When looking at the flags in goma, the compile command doesn't seem to >> > have > >> > > changed, the isysroot for the Mac SDK is identical with and without >> this >> patch >> > > (which supports the above assertion) -- yet we see the following >> errors in >> the >> > > log files; >> > > >> > > E0112 10:11:18.466936 12333056 compile_task.cc:4854] Task:14519 >> Required >> file >> > > not on goma >> > > >> > > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHTTPMessage.h > >> > > E0112 10:11:18.466963 12333056 compile_task.cc:4854] Task:14519 >> Required >> file >> > > not on goma >> > > >> > > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFHost.h > >> > > E0112 10:11:18.466987 12333056 compile_task.cc:4854] Task:14519 >> Required >> file >> > > not on goma >> > > >> > > > cache:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk/System/Library/Frameworks/CFNetwork.framework/Headers/CFNetServices.h > >> > > >> > > Looking at this it also seems to not make sense to use a relative >> sysroot >> > on > >> Mac >> > > too? >> > >> > Did you restart goma in between? I think the web interface never gets >> rid of >> the "error local fallback" box, so maybe that was still around from a >> prior gn >> build attempt? >> > > Yes, we made sure that the errors were from the active run. >> > > Regardless, my suggestion is that this /isn't/ the right way to go about >> handling the sysroot on OSX - it seems to be an absolute path located >> outside >> > of > >> the chromium repo. >> > > Yes, that sounds right to me. > > We're working on possibly eventually having a sysroot downloaded on OS X > for > googlers, like we do on Windows, but it'll very likely be outside of src/ > even > then (like it is on Windows). But currently, the OS X sysroot is always in > /Applications/Xcode.app/... > > > > I propose that we submit this change without altering the OSX sysroot, and >> content ourselves with at least fixing it on Linux/Android. I'll still >> investigate the situation on OSX, and why it fails on goma builds, in the >> > hopes > >> of fixing it, but for now it'd be nice to at least solve it for >> linux/android. >> > > > > https://codereview.chromium.org/1556923002/ > -- 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.
Description was changed from
==========
clang: Makes builds with clang less dependent on absolute file path
[Note: Dependency claims are only shown 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)
I believe that the remaining instances mostly originate from nacl not
having the flag enabled, so if we can update the toolchain's version of
clang to 3.8, (and thus enable the flag for nacl too) we should have
path independent output files.
My reason for believing this can be seen in this output:
http://pastebin.com/raw/Gj3mLrgP.
Generated ninja files still have some absolute file path - at the very least,
they contain -fdebug-prefix-map=/ABSOLUTE/PATH/TO/DIR. This would be nice to
remove later, but removing it from binaries is more important.
BUG=439949
==========
to
==========
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
==========
The CQ bit was checked by mithro@mithis.com
The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1556923002/#ps110001 (title: "Reverting change on OSX and iOS")
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
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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
==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from
==========
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
==========
to
==========
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}
==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fdca0741b22a4dddbf1808b8d0081651662b22c5 Cr-Commit-Position: refs/heads/master@{#370320} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
