Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
284313010:
Add harfbuzz to the GN build. (Closed)
|
Created:
6 years, 7 months ago by brettw Modified:
6 years, 7 months ago Reviewers:
scottmg CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd harfbuzz to the GN build.
R=scottmg@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271916
Patch Set 1 #
Total comments: 2
Patch Set 2 : review comment #Messages
Total messages: 9 (0 generated)
lgtm https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... File third_party/harfbuzz-ng/BUILD.gn (right): https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ this will be an error when is_clang && is_win, right? i'm not sure what to do about it, but in my limited editing of .gn files so far this (= vs. +=) seems like the most error prone thing.
https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... File third_party/harfbuzz-ng/BUILD.gn (right): https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ On 2014/05/20 22:21:55, scottmg wrote: > this will be an error when is_clang && is_win, right? > > i'm not sure what to do about it, but in my limited editing of .gn files so far > this (= vs. +=) seems like the most error prone thing. GN will detect this and throw an error. If you want to overwrite a nonempty list, you have to assign it to empty first to avoid this mistake.
On 2014/05/20 23:10:15, brettw wrote: > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > File third_party/harfbuzz-ng/BUILD.gn (right): > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ > On 2014/05/20 22:21:55, scottmg wrote: > > this will be an error when is_clang && is_win, right? > > > > i'm not sure what to do about it, but in my limited editing of .gn files so > far > > this (= vs. +=) seems like the most error prone thing. > > GN will detect this and throw an error. If you want to overwrite a nonempty > list, you have to assign it to empty first to avoid this mistake. Right, I know. What I meant was that having to do that kind of sucks. In particular in this case, someday it's going to break the build, but only on the Windows clang bot. Do you think always allowing += makes sense? (possibly only for certain variables)
On 2014/05/20 23:16:23, scottmg wrote: > On 2014/05/20 23:10:15, brettw wrote: > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > File third_party/harfbuzz-ng/BUILD.gn (right): > > > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ > > On 2014/05/20 22:21:55, scottmg wrote: > > > this will be an error when is_clang && is_win, right? > > > > > > i'm not sure what to do about it, but in my limited editing of .gn files so > > far > > > this (= vs. +=) seems like the most error prone thing. > > > > GN will detect this and throw an error. If you want to overwrite a nonempty > > list, you have to assign it to empty first to avoid this mistake. > > Right, I know. What I meant was that having to do that kind of sucks. In > particular in this case, someday it's going to break the build, but only on the > Windows clang bot. > > Do you think always allowing += makes sense? (possibly only for certain > variables) That's an interesting idea I hadn't considered. I'll think about it.
On 2014/05/20 23:16:23, scottmg wrote: > On 2014/05/20 23:10:15, brettw wrote: > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > File third_party/harfbuzz-ng/BUILD.gn (right): > > > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ > > On 2014/05/20 22:21:55, scottmg wrote: > > > this will be an error when is_clang && is_win, right? > > > > > > i'm not sure what to do about it, but in my limited editing of .gn files so > > far > > > this (= vs. +=) seems like the most error prone thing. > > > > GN will detect this and throw an error. If you want to overwrite a nonempty > > list, you have to assign it to empty first to avoid this mistake. > > Right, I know. What I meant was that having to do that kind of sucks. In > particular in this case, someday it's going to break the build, but only on the > Windows clang bot. > > Do you think always allowing += makes sense? (possibly only for certain > variables) Or perhaps it's a matter for a coding style guide: e.g. if deps is set in more than one place in a target, then always put "deps = []" at the top and use deps += [...] in the body.
On 2014/05/20 23:21:07, scottmg wrote: > On 2014/05/20 23:16:23, scottmg wrote: > > On 2014/05/20 23:10:15, brettw wrote: > > > > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > > File third_party/harfbuzz-ng/BUILD.gn (right): > > > > > > > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > > third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ > > > On 2014/05/20 22:21:55, scottmg wrote: > > > > this will be an error when is_clang && is_win, right? > > > > > > > > i'm not sure what to do about it, but in my limited editing of .gn files > so > > > far > > > > this (= vs. +=) seems like the most error prone thing. > > > > > > GN will detect this and throw an error. If you want to overwrite a nonempty > > > list, you have to assign it to empty first to avoid this mistake. > > > > Right, I know. What I meant was that having to do that kind of sucks. In > > particular in this case, someday it's going to break the build, but only on > the > > Windows clang bot. > > > > Do you think always allowing += makes sense? (possibly only for certain > > variables) > > Or perhaps it's a matter for a coding style guide: e.g. if deps is set in more > than one place in a target, then always put "deps = []" at the top and use deps > += [...] in the body. That's what I've been doing, but it's easy to miss sometimes.
On 2014/05/20 23:24:37, brettw wrote: > On 2014/05/20 23:21:07, scottmg wrote: > > On 2014/05/20 23:16:23, scottmg wrote: > > > On 2014/05/20 23:10:15, brettw wrote: > > > > > > > > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > > > File third_party/harfbuzz-ng/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.chromium.org/284313010/diff/1/third_party/harfbuzz-ng/BUIL... > > > > third_party/harfbuzz-ng/BUILD.gn:139: cflags = [ > > > > On 2014/05/20 22:21:55, scottmg wrote: > > > > > this will be an error when is_clang && is_win, right? > > > > > > > > > > i'm not sure what to do about it, but in my limited editing of .gn files > > so > > > > far > > > > > this (= vs. +=) seems like the most error prone thing. > > > > > > > > GN will detect this and throw an error. If you want to overwrite a > nonempty > > > > list, you have to assign it to empty first to avoid this mistake. > > > > > > Right, I know. What I meant was that having to do that kind of sucks. In > > > particular in this case, someday it's going to break the build, but only on > > the > > > Windows clang bot. > > > > > > Do you think always allowing += makes sense? (possibly only for certain > > > variables) > > > > Or perhaps it's a matter for a coding style guide: e.g. if deps is set in more > > than one place in a target, then always put "deps = []" at the top and use > deps > > += [...] in the body. > > That's what I've been doing, but it's easy to miss sometimes. Yeah, it's also harder for later changes that insert a block or refactor some booleans. I wonder if we could do something slightly odd and simply error out if there's multiple "deps =" statements in a single target. It'd be a bit annoying in the case of: if (is_win) deps = [...] if (is_linux) deps = [...] but once the conditions are more complicated it'd be a relatively simple way of enforcing that style.
Message was sent while issue was closed.
Committed patchset #2 manually as r271916 (presubmit successful). |
