Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1999513002:
[Mac/GN] Implement dSYM generation and stripping. (Closed)
|
Created:
4 years, 7 months ago by Robert Sesek Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/GN] Implement dSYM generation and stripping.
This creates a new script called the linker driver, which runs all three
steps of linking: the image link, debug info link, and stripping. In GYP,
the last two steps were handled as postbuilds, but GN lacks those. Instead,
the linker driver performs all three operations in one build step.
BUG=330301, 431177
R=mark@chromium.org,dpranke@chromium.org
Committed: https://crrev.com/b89811bd0b77ae38f45359c6d78a5e06b997e499
Cr-Commit-Position: refs/heads/master@{#397880}
Patch Set 1 #
Total comments: 26
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 18
Patch Set 4 : #Patch Set 5 : Global configuration #
Total comments: 5
Patch Set 6 : Add a comment #
Total comments: 5
Patch Set 7 : Comment++ #
Messages
Total messages: 48 (5 generated)
Would like your collective thoughts/a pre-review on this approach. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ The major issue I see is that we're not listing the dSYM as an output.
this basically lgtm, with a few tweaks. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:142: rebase_path("//build/toolchain/mac/linker_driver.py", root_build_dir) You should be able to hoist this up into a file-scoped variable, rather than having to repeat it three times. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ On 2016/05/19 22:38:59, Robert Sesek wrote: > The major issue I see is that we're not listing the dSYM as an output. Can you do something like: if (enable_dsyms) { outputs += [ "{{target_output_name}}.dSYM/" ] } or some such? https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:62: for action in _LINKER_DRIVER_ACTIONS: this should probably be wrapped in a try: ... finally: ... block that deletes the binary if one of the steps fails, to be somewhat atomic and help avoid confusing ninja.
https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:142: rebase_path("//build/toolchain/mac/linker_driver.py", root_build_dir) On 2016/05/20 01:47:19, Dirk Pranke wrote: > You should be able to hoist this up into a file-scoped variable, rather than > having to repeat it three times. I tried doing it at the file-level but GN complained it was unused. Moved it up into the toolchain, though. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ On 2016/05/20 01:47:19, Dirk Pranke wrote: > On 2016/05/19 22:38:59, Robert Sesek wrote: > > The major issue I see is that we're not listing the dSYM as an output. > > Can you do something like: > > if (enable_dsyms) { > outputs += [ "{{target_output_name}}.dSYM/" ] > } > > or some such? > As written, we don't generate dSYMs for every target, since doing so is super slow. We're only doing it for the build products that we strip and ship. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:62: for action in _LINKER_DRIVER_ACTIONS: On 2016/05/20 01:47:19, Dirk Pranke wrote: > this should probably be wrapped in a try: ... finally: ... block that deletes > the binary if one of the steps fails, to be somewhat atomic and help avoid > confusing ninja. Done.
https://codereview.chromium.org/1999513002/diff/1/build/config/mac/symbols.gni File build/config/mac/symbols.gni (right): https://codereview.chromium.org/1999513002/diff/1/build/config/mac/symbols.gn... build/config/mac/symbols.gni:23: enable_stripping = is_official_build The big problem I see here now is that ordinary “Release” builds won’t give people the verify_order failures. Do any try/cq-bots run the “official” configuration? If not, those bad changes will land and then have to be backed out. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:17: # combines these three steps into a single tool. Ask Nico whether there’s any chance of adding these to the compiler driver so that we can drop this wrapper. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:25: # output, producing a dSYM bundle, stored at dsym_path_prefix. dsym_path_prefix is a directory, and the name + .dSYM gets appended? It’d be helpful if you gave an example here that said where linker output and the .dSYM would go. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:110: dsym_out = os.path.join(dsym_path_prefix, tail + '.dSYM') Does dsymutil trash the .dSYM before writing a new one? Easy way to check: put some garbage files in a .dSYM bundle, then run dsymutil again to produce a new one. If it doesn’t, then we should trash an existing .dSYM first before running dsymutil. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:111: subprocess.check_call(['xcrun', 'dsymutil', '-o', dsym_out, linker_out]) If (this or) a later step fails, the .dSYM that you produced should be trashed. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:114: def RunStrip(strip_args_string, full_args): We may at some point want an option to save the original unstripped image somewhere. I’ve wanted that before, but we couldn’t do it easily because Xcode constrained us. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:142: _LINKER_DRIVER_ACTIONS = [ Can you move this up so that it’s after _LINKER_DRIVER_ARG_PREFIX? https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:62: try: I’d move the try up to include the call of the compiler driver, so that even if the compiler driver is totally busted and can’t be started, we’ll still remove any preexisting linked image on our own. https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:72: except RuntimeError: Why isn’t this just “except:”? os.unlink is most likely going to fail with OSError, which doesn’t derive from RuntimeError. But we want to re-raise the original interesting exception no matter why os.unlink failed. https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:78: # Re-report the original failure. Actually, since this is what you want to do no matter what, how about: try: subprocess.check_call(compiler_driver_args) for … except: try: os.unlink(…) finally: raise This is also nice because you no longer need to specify Exception on the except. https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:143: argument list. As this is a required linker argument, raises an error if it Not true. % echo 'int main(int argc, char* argv[]) {return 0;}' > t.c % clang -c t.c % clang t.o % ls a.out t.c t.o The default in the absence of -o is a.out in the current directory.
https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:142: rebase_path("//build/toolchain/mac/linker_driver.py", root_build_dir) On 2016/05/20 15:15:36, Robert Sesek wrote: > On 2016/05/20 01:47:19, Dirk Pranke wrote: > > You should be able to hoist this up into a file-scoped variable, rather than > > having to repeat it three times. > > I tried doing it at the file-level but GN complained it was unused. Moved it up > into the toolchain, though. Oh, yeah, usage doesn't propagate across templates. Well, into the template is good enough. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ On 2016/05/20 15:15:36, Robert Sesek wrote: > On 2016/05/20 01:47:19, Dirk Pranke wrote: > > On 2016/05/19 22:38:59, Robert Sesek wrote: > > > The major issue I see is that we're not listing the dSYM as an output. > > > > Can you do something like: > > > > if (enable_dsyms) { > > outputs += [ "{{target_output_name}}.dSYM/" ] > > } > > > > or some such? > > > > As written, we don't generate dSYMs for every target, since doing so is super > slow. We're only doing it for the build products that we strip and ship. Ah, true. Hmm ...
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/1999513002/diff/1/build/config/mac/symbols.gni File build/config/mac/symbols.gni (right): https://codereview.chromium.org/1999513002/diff/1/build/config/mac/symbols.gn... build/config/mac/symbols.gni:23: enable_stripping = is_official_build On 2016/05/20 15:42:41, Mark Mentovai wrote: > The big problem I see here now is that ordinary “Release” builds won’t give > people the verify_order failures. Do any try/cq-bots run the “official” > configuration? If not, those bad changes will land and then have to be backed > out. verify_order runs independent of this flag. It's running now, on the bots. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/BUILD.gn&q=... https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:17: # combines these three steps into a single tool. On 2016/05/20 15:42:41, Mark Mentovai wrote: > Ask Nico whether there’s any chance of adding these to the compiler driver so > that we can drop this wrapper. Acknowledged. Per offline chat, clang, when driving the full compile+link, can produce a dSYM for you with -g, that part is probably reasonable, but maybe not stripping. Will ask when Nico returns next week. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:25: # output, producing a dSYM bundle, stored at dsym_path_prefix. On 2016/05/20 15:42:41, Mark Mentovai wrote: > dsym_path_prefix is a directory, and the name + .dSYM gets appended? It’d be > helpful if you gave an example here that said where linker output and the .dSYM > would go. Done. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:110: dsym_out = os.path.join(dsym_path_prefix, tail + '.dSYM') On 2016/05/20 15:42:41, Mark Mentovai wrote: > Does dsymutil trash the .dSYM before writing a new one? Easy way to check: put > some garbage files in a .dSYM bundle, then run dsymutil again to produce a new > one. If it doesn’t, then we should trash an existing .dSYM first before running > dsymutil. It "kind of" does. If you put extra things in the bundle it doesn't seem to care, but it does overwrite an existing dSYM file (probably to support the --update argument). Now clobbering it. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:111: subprocess.check_call(['xcrun', 'dsymutil', '-o', dsym_out, linker_out]) On 2016/05/20 15:42:42, Mark Mentovai wrote: > If (this or) a later step fails, the .dSYM that you produced should be trashed. Yeah, this was on my mind when writing it. At the time I couldn't find an elegant solution, but that's now fixed. dSYM generation is at least somewhat smart, though, and builds the output in a mkdtemp dir before moving it into the final location. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:114: def RunStrip(strip_args_string, full_args): On 2016/05/20 15:42:41, Mark Mentovai wrote: > We may at some point want an option to save the original unstripped image > somewhere. I’ve wanted that before, but we couldn’t do it easily because Xcode > constrained us. Acknowledged. https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/linker_... build/toolchain/mac/linker_driver.py:142: _LINKER_DRIVER_ACTIONS = [ On 2016/05/20 15:42:41, Mark Mentovai wrote: > Can you move this up so that it’s after _LINKER_DRIVER_ARG_PREFIX? No because they reference functions declared later. Moved the arg prefix down so they can be friends, though. https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:62: try: On 2016/05/20 15:42:42, Mark Mentovai wrote: > I’d move the try up to include the call of the compiler driver, so that even if > the compiler driver is totally busted and can’t be started, we’ll still remove > any preexisting linked image on our own. Done. https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:72: except RuntimeError: On 2016/05/20 15:42:42, Mark Mentovai wrote: > Why isn’t this just “except:”? > > os.unlink is most likely going to fail with OSError, which doesn’t derive from > RuntimeError. But we want to re-raise the original interesting exception no > matter why os.unlink failed. This was to only capture the exception from _FindLinkerOutput. This has been restructured, though. https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:143: argument list. As this is a required linker argument, raises an error if it On 2016/05/20 15:42:42, Mark Mentovai wrote: > Not true. > > % echo 'int main(int argc, char* argv[]) {return 0;}' > t.c > % clang -c t.c > % clang t.o > % ls > a.out t.c t.o > > The default in the absence of -o is a.out in the current directory. This comment is true for our build.
Mostly LG https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/20001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:143: argument list. As this is a required linker argument, raises an error if it Robert Sesek wrote: > This comment is true for our build. Word it so that it’s correct, then, because I’m still reading it as saying that ld requires -o, not that our build always provides -o. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:22: # driver performs additional actions, based on these arguments: Specify (perhaps by implication if you provide a simple Usage comment) that the compiler driver in linker mode is an argument to this linker driver. “Usage: linker_driver.py clang++ main.o -L. -llib -o program -Wcrl,…” https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:28: # "... -o out/gn/obj/foo/libbar.dylib ... -Wcrl,dsym,out/gn/ ..." If the trailing / isn’t necessary, don’t include it in the example. Otherwise, it becomes unnecessary cargo-cult garbage that people will provide to this script even though it’s not necessary. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:56: linker_driver_actions[driver_action[0]] = driver_action[1] assert (or raise if not) driver_action[0] in linker_driver_actions. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:70: linker_driver_outputs += linker_driver_actions[name](args) I’m not going to force you to fix this because it’s not major, but I note that if the action raises an exception, its output never gets added to the outputs list and will thus never be cleaned up. So if dsymutil starts but croaks part of the way through, we won’t clean up the .dSYM that it began writing as I think we intend to do. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:92: corresponds to the argument. and return blah blah blah https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:130: return [dsym_out] This and RunStrip can return a (tuple) instead of a [list]. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:143: strip_args_list = strip_args_string.split(',') If you get “-Wcrl,strip,”, then you want to run “strip linker_output”. But instead, you’ll run “strip '' linker_output”, which will fail. You want strip_args_list to be [] if strip_args_string is ''. (Possibly you want to support -Wcrl,strip without a trailing comma for this, but you’re not set up for that now.) https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:146: subprocess.check_call(strip_command) TODO(maybe?): swallow that bogus strip deprecation warning https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:155: for (i, arg) in enumerate(full_args): Why not just this: return full_args[full_args.index('-o') + 1] raising ValueError if -o isn’t there or IndexError if nothing follows it. Those can propagate right out of this function, obviating the need for your own RuntimeError.
https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ On 2016/05/20 16:43:55, Dirk Pranke wrote: > On 2016/05/20 15:15:36, Robert Sesek wrote: > > On 2016/05/20 01:47:19, Dirk Pranke wrote: > > > On 2016/05/19 22:38:59, Robert Sesek wrote: > > > > The major issue I see is that we're not listing the dSYM as an output. > > > > > > Can you do something like: > > > > > > if (enable_dsyms) { > > > outputs += [ "{{target_output_name}}.dSYM/" ] > > > } > > > > > > or some such? > > > > > > > As written, we don't generate dSYMs for every target, since doing so is super > > slow. We're only doing it for the build products that we strip and ship. > > Ah, true. Hmm ... Mark: What if we created fake dSYMs again? We could toggle full/real dSYMs on a per-target basis, but if enable_dsyms = true, everything would get a fake dSYM by default. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... File build/toolchain/mac/linker_driver.py (right): https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:22: # driver performs additional actions, based on these arguments: On 2016/05/20 19:41:11, Mark Mentovai wrote: > Specify (perhaps by implication if you provide a simple Usage comment) that the > compiler driver in linker mode is an argument to this linker driver. “Usage: > linker_driver.py clang++ main.o -L. -llib -o program -Wcrl,…” Done. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:28: # "... -o out/gn/obj/foo/libbar.dylib ... -Wcrl,dsym,out/gn/ ..." On 2016/05/20 19:41:11, Mark Mentovai wrote: > If the trailing / isn’t necessary, don’t include it in the example. Otherwise, > it becomes unnecessary cargo-cult garbage that people will provide to this > script even though it’s not necessary. Done. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:56: linker_driver_actions[driver_action[0]] = driver_action[1] On 2016/05/20 19:41:11, Mark Mentovai wrote: > assert (or raise if not) driver_action[0] in linker_driver_actions. Done. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:70: linker_driver_outputs += linker_driver_actions[name](args) On 2016/05/20 19:41:11, Mark Mentovai wrote: > I’m not going to force you to fix this because it’s not major, but I note that > if the action raises an exception, its output never gets added to the outputs > list and will thus never be cleaned up. So if dsymutil starts but croaks part of > the way through, we won’t clean up the .dSYM that it began writing as I think we > intend to do. Acknowledged. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:92: corresponds to the argument. On 2016/05/20 19:41:11, Mark Mentovai wrote: > and return blah blah blah Done. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:130: return [dsym_out] On 2016/05/20 19:41:11, Mark Mentovai wrote: > This and RunStrip can return a (tuple) instead of a [list]. Returning tuple means having a (turd,). These seem more conceptually list-y than tuple-y. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:143: strip_args_list = strip_args_string.split(',') On 2016/05/20 19:41:11, Mark Mentovai wrote: > If you get “-Wcrl,strip,”, then you want to run “strip linker_output”. But > instead, you’ll run “strip '' linker_output”, which will fail. You want > strip_args_list to be [] if strip_args_string is ''. > > (Possibly you want to support -Wcrl,strip without a trailing comma for this, but > you’re not set up for that now.) Done. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:146: subprocess.check_call(strip_command) On 2016/05/20 19:41:11, Mark Mentovai wrote: > TODO(maybe?): swallow that bogus strip deprecation warning Gonna skip this for now. https://codereview.chromium.org/1999513002/diff/60001/build/toolchain/mac/lin... build/toolchain/mac/linker_driver.py:155: for (i, arg) in enumerate(full_args): On 2016/05/20 19:41:11, Mark Mentovai wrote: > Why not just this: > > return full_args[full_args.index('-o') + 1] > > raising ValueError if -o isn’t there or IndexError if nothing follows it. Those > can propagate right out of this function, obviating the need for your own > RuntimeError. Done.
https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ On 2016/05/24 15:05:32, Robert Sesek wrote: > On 2016/05/20 16:43:55, Dirk Pranke wrote: > > On 2016/05/20 15:15:36, Robert Sesek wrote: > > > On 2016/05/20 01:47:19, Dirk Pranke wrote: > > > > On 2016/05/19 22:38:59, Robert Sesek wrote: > > > > > The major issue I see is that we're not listing the dSYM as an output. > > > > > > > > Can you do something like: > > > > > > > > if (enable_dsyms) { > > > > outputs += [ "{{target_output_name}}.dSYM/" ] > > > > } > > > > > > > > or some such? > > > > > > > > > > As written, we don't generate dSYMs for every target, since doing so is > super > > > slow. We're only doing it for the build products that we strip and ship. > > > > Ah, true. Hmm ... > > Mark: What if we created fake dSYMs again? We could toggle full/real dSYMs on a > per-target basis, but if enable_dsyms = true, everything would get a fake dSYM > by default. I would prefer not to. The fake .dSYMs were just garbage. Nothing in the modern toolchain could do anything useful with them. Now that dsymutil is really llvm-dsymutil, and most developers are on the (ugh) component=shared_library build, is doing real .dSYMs everywhere (in a Release build) still a problem? Note that I’m fully expecting the answer to be “no,” but I gotta ask.
https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.gn File build/toolchain/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/1/build/toolchain/mac/BUILD.g... build/toolchain/mac/BUILD.gn:174: outputs = [ On 2016/05/24 18:24:32, Mark Mentovai wrote: > On 2016/05/24 15:05:32, Robert Sesek wrote: > > On 2016/05/20 16:43:55, Dirk Pranke wrote: > > > On 2016/05/20 15:15:36, Robert Sesek wrote: > > > > On 2016/05/20 01:47:19, Dirk Pranke wrote: > > > > > On 2016/05/19 22:38:59, Robert Sesek wrote: > > > > > > The major issue I see is that we're not listing the dSYM as an output. > > > > > > > > > > Can you do something like: > > > > > > > > > > if (enable_dsyms) { > > > > > outputs += [ "{{target_output_name}}.dSYM/" ] > > > > > } > > > > > > > > > > or some such? > > > > > > > > > > > > > As written, we don't generate dSYMs for every target, since doing so is > > super > > > > slow. We're only doing it for the build products that we strip and ship. > > > > > > Ah, true. Hmm ... > > > > Mark: What if we created fake dSYMs again? We could toggle full/real dSYMs on > a > > per-target basis, but if enable_dsyms = true, everything would get a fake dSYM > > by default. > > I would prefer not to. The fake .dSYMs were just garbage. Nothing in the modern > toolchain could do anything useful with them. > > Now that dsymutil is really llvm-dsymutil, and most developers are on the (ugh) > component=shared_library build, is doing real .dSYMs everywhere (in a Release > build) still a problem? Note that I’m fully expecting the answer to be “no,” but > I gotta ask. In GN land, enable_dsyms is by default tied to is_official_build. I guess we could just dSYM all the things in this mode. It may slow down the official builders for generating dSYMs for test targets, but I don't think it'd have any impact on developer or non-official bots. (And if it does, developers can just manually turn off enable_dsyms).
I think rsesek@ and I chatted about this offline, but I think we should probably change this so that we do the link normally and do the stripping as a separate GN rule that depends on the link. I don't think there's a good way to get the outputs set correctly if we try to do both in a single GN rule, and getting the outputs set correctly is probably more important than trying to save a copy of the binary.
Does that mean that you need to make a pair of targets for everything that you want to strip?
On 2016/05/24 18:36:32, Mark Mentovai wrote: > Does that mean that you need to make a pair of targets for everything that you > want to strip? Potentially, though if the number of desired stripped binaries is greater than a couple, you'd probably want to wrap it into a new template or make it part of an existing template (with the functionality toggled by a parameter to the template).
That sucks, then. We’ve got a handful of things that we ship and thus need to be stripped, and the set changes over time. If this requires any amount of extra effort at all, people aren’t going to do it. A big NAK to that plan from me.
I'm not a fan of that either, because most of the things that we need to strip/dSYM are also bundled, which means we have to do a lot of path fiddling, too. If we just dSYM everything, we can sidestep the issue.
On 2016/05/24 18:42:19, Mark Mentovai wrote: > That sucks, then. We’ve got a handful of things that we ship and thus need to be > stripped, and the set changes over time. If this requires any amount of extra > effort at all, people aren’t going to do it. > > A big NAK to that plan from me. The CL as stands requires you to add a config to indicate that you want something stripped, right? Are you saying that adding a "should_strip = true" flag to your target be worse?
On 2016/05/24 18:43:32, Robert Sesek wrote: > I'm not a fan of that either, because most of the things that we need to > strip/dSYM are also bundled, which means we have to do a lot of path fiddling, > too. If we just dSYM everything, we can sidestep the issue. Sure, if you think you can dSYM everything instead, that's fine too. Reusing an existing knob is better than adding a new one.
Dirk Pranke wrote: > On 2016/05/24 18:42:19, Mark Mentovai wrote: > > That sucks, then. We’ve got a handful of things that we ship and thus need to > be > > stripped, and the set changes over time. If this requires any amount of extra > > effort at all, people aren’t going to do it. > > > > A big NAK to that plan from me. > > The CL as stands requires you to add a config to indicate that you want > something stripped, right? > > Are you saying that adding a "should_strip = true" flag to your target be worse? I think that’s bad too. I want people to have to go out of their way to produce something unstripped in the configuration that we’re actually shipping.
On 2016/05/24 18:53:33, Mark Mentovai wrote:
> Dirk Pranke wrote:
> > On 2016/05/24 18:42:19, Mark Mentovai wrote:
> > > That sucks, then. We’ve got a handful of things that we ship and thus need
> to
> > be
> > > stripped, and the set changes over time. If this requires any amount of
> extra
> > > effort at all, people aren’t going to do it.
> > >
> > > A big NAK to that plan from me.
> >
> > The CL as stands requires you to add a config to indicate that you want
> > something stripped, right?
> >
> > Are you saying that adding a "should_strip = true" flag to your target be
> worse?
>
> I think that’s bad too. I want people to have to go out of their way to
produce
> something unstripped in the configuration that we’re actually shipping.
Either I'm confused or I'm not making myself clear enough.
I'm talking about changing the user-level interface from:
mac_app_bundle("chrome_app") {
...
extra_configs = [ ... ]
if (enable_stripping) {
ldflags += [ ... ]
}
...
}
to
mac_app_bundle("chrome_app") {
enable_dsyms = true
should_strip = true
}
Or something like that. If you want the defaults to be different, that's fine,
too.
If you want official builds to have every executable generate dSYMS and be
stripped, then I agree that you don't need these flags at all, and having no
flags is preferable to having flags.
I'm confused what you actually want to be able to do?
On 2016/05/24 19:01:19, Dirk Pranke wrote:
> On 2016/05/24 18:53:33, Mark Mentovai wrote:
> > Dirk Pranke wrote:
> > > On 2016/05/24 18:42:19, Mark Mentovai wrote:
> > > > That sucks, then. We’ve got a handful of things that we ship and thus
need
> > to
> > > be
> > > > stripped, and the set changes over time. If this requires any amount of
> > extra
> > > > effort at all, people aren’t going to do it.
> > > >
> > > > A big NAK to that plan from me.
> > >
> > > The CL as stands requires you to add a config to indicate that you want
> > > something stripped, right?
> > >
> > > Are you saying that adding a "should_strip = true" flag to your target be
> > worse?
> >
> > I think that’s bad too. I want people to have to go out of their way to
> produce
> > something unstripped in the configuration that we’re actually shipping.
>
> Either I'm confused or I'm not making myself clear enough.
>
> I'm talking about changing the user-level interface from:
>
> mac_app_bundle("chrome_app") {
> ...
> extra_configs = [ ... ]
> if (enable_stripping) {
> ldflags += [ ... ]
> }
> ...
> }
>
> to
>
> mac_app_bundle("chrome_app") {
> enable_dsyms = true
> should_strip = true
> }
>
This would cover bundled targets nicely (we could wrap the extra chained actions
behind a template), but it wouldn't cover vanilla executable() and
loadable_module() targets. That's why I was trying to hook at the toolchain
level.
On 2016/05/24 19:03:57, Robert Sesek wrote:
> On 2016/05/24 19:01:19, Dirk Pranke wrote:
> > On 2016/05/24 18:53:33, Mark Mentovai wrote:
> > > Dirk Pranke wrote:
> > > > On 2016/05/24 18:42:19, Mark Mentovai wrote:
> > > > > That sucks, then. We’ve got a handful of things that we ship and thus
> need
> > > to
> > > > be
> > > > > stripped, and the set changes over time. If this requires any amount
of
> > > extra
> > > > > effort at all, people aren’t going to do it.
> > > > >
> > > > > A big NAK to that plan from me.
> > > >
> > > > The CL as stands requires you to add a config to indicate that you want
> > > > something stripped, right?
> > > >
> > > > Are you saying that adding a "should_strip = true" flag to your target
be
> > > worse?
> > >
> > > I think that’s bad too. I want people to have to go out of their way to
> > produce
> > > something unstripped in the configuration that we’re actually shipping.
> >
> > Either I'm confused or I'm not making myself clear enough.
> >
> > I'm talking about changing the user-level interface from:
> >
> > mac_app_bundle("chrome_app") {
> > ...
> > extra_configs = [ ... ]
> > if (enable_stripping) {
> > ldflags += [ ... ]
> > }
> > ...
> > }
> >
> > to
> >
> > mac_app_bundle("chrome_app") {
> > enable_dsyms = true
> > should_strip = true
> > }
> >
>
> This would cover bundled targets nicely (we could wrap the extra chained
actions
> behind a template), but it wouldn't cover vanilla executable() and
> loadable_module() targets. That's why I was trying to hook at the toolchain
> level.
Right, which is why we'd want to move them to other templates if we needed to
configure these things conditionally for them as well. There are relatively few
bare loadable_module() and executable() targets in the build, so changing them
shouldn't be difficult and there are those of us that think we should do that
anyway
just so that we have this sort of flexibility when we need it.
Dirk Pranke wrote: > If you want official builds to have every executable generate dSYMS and be > stripped, then I agree that you don't need these flags at all, and having no > flags is preferable to having flags. In the official build, I don’t think that all linker output needs a .dSYM necessarily, but I do think that all linker output needs to be stripped appropriately (barring something that explicitly says not to). > I'm confused what you actually want to be able to do? Since our frame of reference is the gyp build and this is how the gyp build works, I don’t know where the confusion is coming from.
On 2016/05/24 19:06:51, Dirk Pranke wrote:
> On 2016/05/24 19:03:57, Robert Sesek wrote:
> > On 2016/05/24 19:01:19, Dirk Pranke wrote:
> > > On 2016/05/24 18:53:33, Mark Mentovai wrote:
> > > > Dirk Pranke wrote:
> > > > > On 2016/05/24 18:42:19, Mark Mentovai wrote:
> > > > > > That sucks, then. We’ve got a handful of things that we ship and
thus
> > need
> > > > to
> > > > > be
> > > > > > stripped, and the set changes over time. If this requires any amount
> of
> > > > extra
> > > > > > effort at all, people aren’t going to do it.
> > > > > >
> > > > > > A big NAK to that plan from me.
> > > > >
> > > > > The CL as stands requires you to add a config to indicate that you
want
> > > > > something stripped, right?
> > > > >
> > > > > Are you saying that adding a "should_strip = true" flag to your target
> be
> > > > worse?
> > > >
> > > > I think that’s bad too. I want people to have to go out of their way to
> > > produce
> > > > something unstripped in the configuration that we’re actually shipping.
> > >
> > > Either I'm confused or I'm not making myself clear enough.
> > >
> > > I'm talking about changing the user-level interface from:
> > >
> > > mac_app_bundle("chrome_app") {
> > > ...
> > > extra_configs = [ ... ]
> > > if (enable_stripping) {
> > > ldflags += [ ... ]
> > > }
> > > ...
> > > }
> > >
> > > to
> > >
> > > mac_app_bundle("chrome_app") {
> > > enable_dsyms = true
> > > should_strip = true
> > > }
> > >
> >
> > This would cover bundled targets nicely (we could wrap the extra chained
> actions
> > behind a template), but it wouldn't cover vanilla executable() and
> > loadable_module() targets. That's why I was trying to hook at the toolchain
> > level.
>
> Right, which is why we'd want to move them to other templates if we needed to
> configure these things conditionally for them as well. There are relatively
few
> bare loadable_module() and executable() targets in the build, so changing them
> shouldn't be difficult and there are those of us that think we should do that
> anyway
> just so that we have this sort of flexibility when we need it.
What would we name this template, though? More to the point, how would
developers know they need to use this?
One more thing to note: there is another step to this process that isn't
included in this CL: running dump_syms on the dSYM to generate the breakpad
symbol files. I was planning on adding this as another step to the linker
driver.
This probably seems more difficult to integrate with GN than other things
because GYP did these things as postbuilds, and GN has no analog...
On 2016/05/24 19:07:49, Mark Mentovai wrote: > Dirk Pranke wrote: > > If you want official builds to have every executable generate dSYMS and be > > stripped, then I agree that you don't need these flags at all, and having no > > flags is preferable to having flags. > > In the official build, I don’t think that all linker output needs a .dSYM > necessarily, but I do think that all linker output needs to be stripped > appropriately (barring something that explicitly says not to). > > > I'm confused what you actually want to be able to do? > > Since our frame of reference is the gyp build and this is how the gyp build > works, I don’t know where the confusion is coming from. Well, you talked about potentially wanting to keep the unstripped binaries around as well, at least for chrome, which IIUC is not something the GYP build does today, right? And it sounds like you're unsure whether you want (real) dSYMs for every target or not? I'm not actually that familiar w/ exactly how the current official Mac build works, so perhaps at least part of my confusion is stemming from that.
On 2016/05/24 19:25:20, Robert Sesek wrote:
> On 2016/05/24 19:06:51, Dirk Pranke wrote:
> > On 2016/05/24 19:03:57, Robert Sesek wrote:
> > > On 2016/05/24 19:01:19, Dirk Pranke wrote:
> > > > On 2016/05/24 18:53:33, Mark Mentovai wrote:
> > > > > Dirk Pranke wrote:
> > > > > > On 2016/05/24 18:42:19, Mark Mentovai wrote:
> > > > > > > That sucks, then. We’ve got a handful of things that we ship and
> thus
> > > need
> > > > > to
> > > > > > be
> > > > > > > stripped, and the set changes over time. If this requires any
amount
> > of
> > > > > extra
> > > > > > > effort at all, people aren’t going to do it.
> > > > > > >
> > > > > > > A big NAK to that plan from me.
> > > > > >
> > > > > > The CL as stands requires you to add a config to indicate that you
> want
> > > > > > something stripped, right?
> > > > > >
> > > > > > Are you saying that adding a "should_strip = true" flag to your
target
> > be
> > > > > worse?
> > > > >
> > > > > I think that’s bad too. I want people to have to go out of their way
to
> > > > produce
> > > > > something unstripped in the configuration that we’re actually
shipping.
> > > >
> > > > Either I'm confused or I'm not making myself clear enough.
> > > >
> > > > I'm talking about changing the user-level interface from:
> > > >
> > > > mac_app_bundle("chrome_app") {
> > > > ...
> > > > extra_configs = [ ... ]
> > > > if (enable_stripping) {
> > > > ldflags += [ ... ]
> > > > }
> > > > ...
> > > > }
> > > >
> > > > to
> > > >
> > > > mac_app_bundle("chrome_app") {
> > > > enable_dsyms = true
> > > > should_strip = true
> > > > }
> > > >
> > >
> > > This would cover bundled targets nicely (we could wrap the extra chained
> > actions
> > > behind a template), but it wouldn't cover vanilla executable() and
> > > loadable_module() targets. That's why I was trying to hook at the
toolchain
> > > level.
> >
> > Right, which is why we'd want to move them to other templates if we needed
to
> > configure these things conditionally for them as well. There are relatively
> few
> > bare loadable_module() and executable() targets in the build, so changing
them
> > shouldn't be difficult and there are those of us that think we should do
that
> > anyway
> > just so that we have this sort of flexibility when we need it.
>
> What would we name this template, though? More to the point, how would
> developers know they need to use this?
Yes, those are valid questions that we'd need to work through, which is why we
haven't actually required that before now. We also haven't actually needed this
functionality to be broadly configurable; in the cases where we're talking about
only a few targets that are custom we prefer to address those through custom
steps, because adding features to the core just adds complexity.
>
> One more thing to note: there is another step to this process that isn't
> included in this CL: running dump_syms on the dSYM to generate the breakpad
> symbol files. I was planning on adding this as another step to the linker
> driver.
>
> This probably seems more difficult to integrate with GN than other things
> because GYP did these things as postbuilds, and GN has no analog...
Yup. Though, part of the problem is also that it's never been particularly clear
to me (or to Brett, if I'm remembering correctly) exactly what the semantics of
postbuilds are in GYP (or whether they're mapped onto ninja rules in a
consistent
way).
OK. Then I’ll describe the baseline requirements, and maybe this will be an opportunity to do a clean-sheet design that addresses all of this elegantly. For linker output that we ship in the official configuration, we want: 1. to produce a .dSYM 2. to strip 3. to run dump_syms The GYP build does steps 1 and 2 automatically. dump_syms happens separately in tools/build/mac/dump_product_syms hanging off the “chrome_initial” target, but if GN provides a way to just do it automatically, that’d be even better. (dump_syms output is consumed by chrome-internal/trunk/tools/build/scripts/slave-internal/official_symbols.py to do symupload, and that script currently does a wildcard match for *.breakpad.) Now, we only need this for linker output that we ship in the official configuration. We don’t need it for linker output that we don’t ship, although it doesn’t really hurt, either. I’d prefer it if these things happened to linker output automatically unless suppressed, so that nobody adding new linker output to what we ship has to think or take any extra steps to ensure that .dSYMs are produced and archived, and Breakpad dumped symbol files are produced and symuploaded. This has been a problem in the past. Bear in mind that dump_syms is produced by the build, so if step 3 happens for all linker output, there’s a self-dependency problem waiting to bite. If GN maintains a distinction between build host tools and things that run on the target, we only care about doing this for the things going to the target. If GN has any knowledge anywhere of which things are tests, we don’t care about doing this for tests. We don’t want to do any of this stuff in the debug configuration. It doesn’t matter to anyone whether we do it in the release configuration (although I prefer that release matches official except where it’s cumbersome). Saving the unstripped original executable is optional at this point, but I can envision a future, when we transition fully from Breakpad to Crashpad, where these artifacts would be useful. So there’s no need to implement this aspect now, I was just thinking out loud when I mentioned it. But it does seem like it could possibly be handled by the same “link step has multiple outputs” logic that could be used to address (1) and (3).
Is there any particular need to strip and generating dSYMs for any target other than :chrome_app, :chrome_helper_app, :content_shell, :content_shell_helper_app, and maybe :app_mode_loader (i.e., all the ones that are mac_app_bundle()s)? On Linux, for example, we only bother to strip the actual chrome binary. If just handling mac_app_bundles() is good enough, just embedding the logic as follow-on actions in the template is the easy way to go. If you need the dSYMs for other targets, I'd be curious what for. And, if the other targets is a short list, it seems like creating some other template might be reasonable.
Yes. crashpad_handler, Google Chrome Framework, Widevine CDM, liblzma and xzdec are others. Most of these aren’t bundles. There may be more. The list changes over time.
On 2016/05/25 05:07:38, Mark Mentovai wrote: > Yes. crashpad_handler, Google Chrome Framework, Widevine CDM, liblzma and xzdec > are others. Most of these aren’t bundles. There may be more. The list changes > over time. Okay, between that and staring at the gyp build some more, I think I have a better sense of things. I will think on this some more, but ... It seems like you could basically get something equivalent to what GYP does today by unconditionally stripping and splitting files and adding outputs in the link() and solink() tools when mac_strip==1, and running dump_syms as a postbuild action. This would end up stripping all of the tests and other binaries, which is a bunch more work than is necessary. However, I don't see a good way to avoid that and get the outputs right without creating new templates and requiring them for "potentially shippable" things. Maybe brettw@ has other thoughts here, too.
On 2016/05/25 05:32:28, Dirk Pranke (slow) wrote: > On 2016/05/25 05:07:38, Mark Mentovai wrote: > > Yes. crashpad_handler, Google Chrome Framework, Widevine CDM, liblzma and > xzdec > > are others. Most of these aren’t bundles. There may be more. The list changes > > over time. > > Okay, between that and staring at the gyp build some more, I think I have a > better sense of things. > I will think on this some more, but ... > > It seems like you could basically get something equivalent to what GYP does > today by > unconditionally stripping and splitting files and adding outputs in the link() > and solink() tools > when mac_strip==1, and running dump_syms as a postbuild action. > > This would end up stripping all of the tests and other binaries, which is a > bunch more work than > is necessary. However, I don't see a good way to avoid that and get the outputs > right without > creating new templates and requiring them for "potentially shippable" things. > > Maybe brettw@ has other thoughts here, too. That is what I'm leaning towards now, basically what I wrote in #11: If enable_dsyms=true, then any target you build will have dsymutil run on it. If you enable_stripping=true, all targets you build will get stripped. Breakpad symbols we can just do with a single action target, since we really only need to do a handful of them.
I think we can live with that.
The morning has not presented me with any better ideas, so I think that's probably the way to go for now.
I've reworked this CL to do what we discussed above, which is to always produce dSYMs and/or strip the binaries.
LGTM
dpranke@chromium.org changed reviewers: + brettw@chromium.org
lgtm w/ a couple of suggestions. You'll also need brettw@'s review for the changes to BUILDCONFIG.gn https://codereview.chromium.org/1999513002/diff/100001/build/config/BUILDCONF... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1999513002/diff/100001/build/config/BUILDCONF... build/config/BUILDCONFIG.gn:512: _mac_linker_configs = [ "//build/config/mac:strip_all" ] It might make more sense to have a single generic "//build/config/mac:linker_config" to encapsulate things a bit better (and keep you from having to change BUILDCONFIG in the future). https://codereview.chromium.org/1999513002/diff/100001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/100001/chrome/BUILD.gn#newcod... chrome/BUILD.gn:431: if (enable_stripping) { maybe add a comment explaining what this is doing (and why it's different from the default strip logic)?
https://codereview.chromium.org/1999513002/diff/100001/build/config/BUILDCONF... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1999513002/diff/100001/build/config/BUILDCONF... build/config/BUILDCONFIG.gn:512: _mac_linker_configs = [ "//build/config/mac:strip_all" ] On 2016/05/31 19:58:19, Dirk Pranke wrote: > It might make more sense to have a single generic > "//build/config/mac:linker_config" to encapsulate things a bit better (and keep > you from having to change BUILDCONFIG in the future). If I do (in //build/config/mac/BUILD.gn): config("linker_config") { configs = [ ":strip_all" ] } ... then I can't seem to remove the individual strip_all config via remove_configs. https://codereview.chromium.org/1999513002/diff/100001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/100001/chrome/BUILD.gn#newcod... chrome/BUILD.gn:431: if (enable_stripping) { On 2016/05/31 19:58:19, Dirk Pranke wrote: > maybe add a comment explaining what this is doing (and why it's different from > the default strip logic)? Done.
+brettw for build/config/BUILDCONFIG.gn
https://codereview.chromium.org/1999513002/diff/100001/build/config/BUILDCONF... File build/config/BUILDCONFIG.gn (right): https://codereview.chromium.org/1999513002/diff/100001/build/config/BUILDCONF... build/config/BUILDCONFIG.gn:512: _mac_linker_configs = [ "//build/config/mac:strip_all" ] On 2016/06/01 20:21:05, Robert Sesek wrote: > On 2016/05/31 19:58:19, Dirk Pranke wrote: > > It might make more sense to have a single generic > > "//build/config/mac:linker_config" to encapsulate things a bit better (and > keep > > you from having to change BUILDCONFIG in the future). > > If I do (in //build/config/mac/BUILD.gn): > > config("linker_config") { > configs = [ ":strip_all" ] > } > > ... then I can't seem to remove the individual strip_all config via > remove_configs. Yeah, I wasn't sure if that would work or not. Fair enough.
LGTM with some tweaks. I reviewed the big picture and didn't look at the details in the linker script. https://codereview.chromium.org/1999513002/diff/120001/build/config/mac/BUILD.gn File build/config/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/120001/build/config/mac/BUILD... build/config/mac/BUILD.gn:91: # The ldflags referenced below are handled by Can this comment give a description of what this config does conceptually and why one might want to override it? https://codereview.chromium.org/1999513002/diff/120001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/120001/chrome/BUILD.gn#newcod... chrome/BUILD.gn:466: [ "-Wcrl,strip,-s," + rebase_path("app/app.saves", root_build_dir) ] The file app/app.saves should be added as an input or something to this step so that when it's changed this step is re-run. I'm not sure if mac_app_bundle has the concept of "inputs" that aren't bundled, so we may need to add this. https://codereview.chromium.org/1999513002/diff/120001/chrome/BUILD.gn#newcod... chrome/BUILD.gn:617: [ "-Wcrl,strip,-s," + rebase_path("app/app.saves", root_build_dir) ] Ditto
On 2016/06/03 20:23:26, brettw wrote: > LGTM with some tweaks. I reviewed the big picture and didn't look at the details > in the linker script. Thanks. I think Mark's review covers the driver script sufficiently. https://codereview.chromium.org/1999513002/diff/120001/build/config/mac/BUILD.gn File build/config/mac/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/120001/build/config/mac/BUILD... build/config/mac/BUILD.gn:91: # The ldflags referenced below are handled by On 2016/06/03 20:23:25, brettw wrote: > Can this comment give a description of what this config does conceptually and > why one might want to override it? Done. https://codereview.chromium.org/1999513002/diff/120001/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1999513002/diff/120001/chrome/BUILD.gn#newcod... chrome/BUILD.gn:466: [ "-Wcrl,strip,-s," + rebase_path("app/app.saves", root_build_dir) ] On 2016/06/03 20:23:25, brettw wrote: > The file app/app.saves should be added as an input or something to this step so > that when it's changed this step is re-run. I'm not sure if mac_app_bundle has > the concept of "inputs" that aren't bundled, so we may need to add this. We've hit this issue before with the .order file to the linker. No solution yet, but there's a bug: https://bugs.chromium.org/p/chromium/issues/detail?id=612623
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, dpranke@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1999513002/#ps140001 (title: "Comment++")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999513002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Implement dSYM generation and stripping. This creates a new script called the linker driver, which runs all three steps of linking: the image link, debug info link, and stripping. In GYP, the last two steps were handled as postbuilds, but GN lacks those. Instead, the linker driver performs all three operations in one build step. BUG=330301,431177 R=mark@chromium.org,dpranke@chromium.org ========== to ========== [Mac/GN] Implement dSYM generation and stripping. This creates a new script called the linker driver, which runs all three steps of linking: the image link, debug info link, and stripping. In GYP, the last two steps were handled as postbuilds, but GN lacks those. Instead, the linker driver performs all three operations in one build step. BUG=330301,431177 R=mark@chromium.org,dpranke@chromium.org Committed: https://crrev.com/b89811bd0b77ae38f45359c6d78a5e06b997e499 Cr-Commit-Position: refs/heads/master@{#397880} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b89811bd0b77ae38f45359c6d78a5e06b997e499 Cr-Commit-Position: refs/heads/master@{#397880} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
