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

Issue 1436563003: Support spaces in Mac GN build output names. (Closed)

Created:
5 years, 1 month ago by brettw
Modified:
5 years, 1 month ago
Reviewers:
Bons
CC:
chromium-reviews, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Support spaces in Mac GN build output names. We can't use object paths with spaces in them due to a mismatch between Ninja and the Mac LD's filelist parameter (explained in more detail in a comment added in the mac toolchain GN file). In order to support output_names with spaces, the object file directory must be based on something else. Adds a new substitution "{{target_label}}" to the tool that expands to the target name after the colon, not overridden by the output_name. This makes more sense from a directory structure perspective, and label names won't have spaces (they can but don't, and with this change won't work on Mac). Also adds quoting for Mac tool commands so spaces will work in names. I made the change in the Mac toolchain definition for testing, but it is currently commented out pending the binary roll. After the binary roll, we should uncomment this and update the other toolchains to match. BUG=546894 Committed: https://crrev.com/782b1bc8562e9adbb1ea7d98929d6709ff7290b9 Cr-Commit-Position: refs/heads/master@{#358792}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -9 lines) Patch
M build/toolchain/mac/BUILD.gn View 9 chunks +25 lines, -9 lines 0 comments Download
M tools/gn/function_toolchain.cc View 2 chunks +8 lines, -0 lines 0 comments Download
M tools/gn/ninja_target_writer.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M tools/gn/substitution_type.h View 1 chunk +1 line, -0 lines 0 comments Download
M tools/gn/substitution_type.cc View 3 chunks +3 lines, -0 lines 0 comments Download
M tools/gn/substitution_writer.cc View 1 chunk +3 lines, -0 lines 0 comments Download
M tools/gn/substitution_writer_unittest.cc View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (4 generated)
brettw
5 years, 1 month ago (2015-11-10 00:36:14 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436563003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436563003/1
5 years, 1 month ago (2015-11-10 00:40:43 UTC) #4
Bons
lgtm
5 years, 1 month ago (2015-11-10 00:56:00 UTC) #5
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 1 month ago (2015-11-10 01:56:01 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1436563003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1436563003/1
5 years, 1 month ago (2015-11-10 04:57:19 UTC) #9
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 1 month ago (2015-11-10 05:03:07 UTC) #10
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/782b1bc8562e9adbb1ea7d98929d6709ff7290b9 Cr-Commit-Position: refs/heads/master@{#358792}
5 years, 1 month ago (2015-11-10 05:03:58 UTC) #11
Dirk Pranke
Correct me if I'm wrong, but ... a) I think the CL description is actually ...
5 years, 1 month ago (2015-11-10 20:14:47 UTC) #12
brettw
5 years, 1 month ago (2015-11-10 21:45:32 UTC) #13
Message was sent while issue was closed.
Guess it's too late for CL description. I'll update reference next time I do a
GN patch.

Powered by Google App Engine
This is Rietveld 408576698