Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927273002:
[Mac/GN] Add a BUILD.gn file for //chrome/app_shim. (Closed)
|
Created:
4 years, 7 months ago by Robert Sesek Modified:
4 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@gn-chrome-tools Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/GN] Add a BUILD.gn file for //chrome/app_shim.
BUG=431177
Committed: https://crrev.com/793e3f970d038015da28feaba6c53b18126a852a
Cr-Commit-Position: refs/heads/master@{#391106}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Use invoker.configs #
Total comments: 9
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : Docs #Messages
Total messages: 25 (6 generated)
rsesek@chromium.org changed reviewers: + dpranke@google.com, tapted@chromium.org
tapted: please review dpranke: explicit question for you https://codereview.chromium.org/1927273002/diff/1/chrome/app_shim/BUILD.gn File chrome/app_shim/BUILD.gn (right): https://codereview.chromium.org/1927273002/diff/1/chrome/app_shim/BUILD.gn#ne... chrome/app_shim/BUILD.gn:22: add_configs = [ "//build/config/compiler:wexit_time_destructors" ] dpranke: Normally this would be expressed by configs += [], but since this is a template, that doesn't work. Has this problem been solved before, if not, does this look OK?
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/1927273002/diff/1/chrome/app_shim/BUILD.gn File chrome/app_shim/BUILD.gn (right): https://codereview.chromium.org/1927273002/diff/1/chrome/app_shim/BUILD.gn#ne... chrome/app_shim/BUILD.gn:22: add_configs = [ "//build/config/compiler:wexit_time_destructors" ] On 2016/04/28 20:14:59, Robert Sesek wrote: > dpranke: Normally this would be expressed by configs += [], but since this is a > template, that doesn't work. Has this problem been solved before, if not, does > this look OK? Are you sure invoker.configs (and just setting configs in the caller) doesn't work? I would expect it to.
https://codereview.chromium.org/1927273002/diff/1/chrome/app_shim/BUILD.gn File chrome/app_shim/BUILD.gn (right): https://codereview.chromium.org/1927273002/diff/1/chrome/app_shim/BUILD.gn#ne... chrome/app_shim/BUILD.gn:22: add_configs = [ "//build/config/compiler:wexit_time_destructors" ] On 2016/04/28 20:24:36, Dirk Pranke wrote: > On 2016/04/28 20:14:59, Robert Sesek wrote: > > dpranke: Normally this would be expressed by configs += [], but since this is > a > > template, that doesn't work. Has this problem been solved before, if not, does > > this look OK? > > Are you sure invoker.configs (and just setting configs in the caller) doesn't > work? I would expect it to. Ah, right, that should work. I was also trying to figure out how configs -= [] would work, but haven't hit an instance of that yet. This does prohibit something from overriding all the configs, though.
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs You don't need both line 184 and lines 190-192, I don't think, just add line 184. (And that should address your other concerns).
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs On 2016/04/28 21:20:45, Dirk Pranke wrote: > You don't need both line 184 and lines 190-192, I don't think, just add line > 184. (And that should address your other concerns). Hm, then you don't get the default configs that set things like the include_dirs. That's why I initially went with add_configs. If we let the executable override the configs entirely, how would I add those default configs back in?
dpranke@chromium.org changed reviewers: + brettw@chromium.org - dpranke@google.com
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs On 2016/04/28 21:34:54, Robert Sesek wrote: > On 2016/04/28 21:20:45, Dirk Pranke wrote: > > You don't need both line 184 and lines 190-192, I don't think, just add line > > 184. (And that should address your other concerns). > > Hm, then you don't get the default configs that set things like the > include_dirs. > > That's why I initially went with add_configs. If we let the executable override > the configs entirely, how would I add those default configs back in? Good question. I would've expected the configs to propagate, but I guess I'm not sure what they'll do. Brett, what's the right thing to do here?
brettw will need to chime in on the rest, but app_shim/ lgtm . So much nicer than the gyp config :3
brettw: ping on configs question
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:184: "configs", This should be removed. It should actually give you an error (the docs for forward_variables_from describes this) but I looked at the code and it doesn't actually throw this error. I'll make a note to fix this. The problem is that the executable has configs defined implicitly. This line will clobber those defaults. Even if that wasn't the case, this line is redundant with the code you wrote below to check the invoker configs. https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs These block is correct.
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:184: "configs", On 2016/05/02 18:16:24, brettw wrote: > This should be removed. It should actually give you an error (the docs for > forward_variables_from describes this) but I looked at the code and it doesn't > actually throw this error. I'll make a note to fix this. > > The problem is that the executable has configs defined implicitly. This line > will clobber those defaults. Even if that wasn't the case, this line is > redundant with the code you wrote below to check the invoker configs. Thanks. That was the open question, whether the clobber should happen or whether we should do set addition. https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs On 2016/05/02 18:16:24, brettw wrote: > These block is correct. That didn't seem to work if I removed the clobber. GN complained: ERROR at //chrome/app_shim/BUILD.gn:22:15: Duplicate item in list configs = [ "//build/config/compiler:wexit_time_destructors" ] ^----------------------------------------------- See //chrome/app_shim/BUILD.gn:22:15: This was the previous definition. configs = [ "//build/config/compiler:wexit_time_destructors" ] ^----------------------------------------------- See //chrome/app_shim/BUILD.gn:21:1: whence it was called. mac_app_bundle("app_mode_loader") { ^---------------------------------- See //chrome/browser/BUILD.gn:440:7: which caused the file to be included. "//chrome/app_shim", ^------------------ But if I renamed it to invoker.extra_configs, then the error went away.
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs Don't know, I would recommend printing the various configs values. Renaming it extra_configs is probably better though since these are additive and not replaced. Just setting configs on an app bundle would imply to me that's the full set of stuff getting set.
https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/20001/build/config/mac/rules.... build/config/mac/rules.gni:191: configs += invoker.configs On 2016/05/02 19:18:01, brettw wrote: > Don't know, I would recommend printing the various configs values. > > Renaming it extra_configs is probably better though since these are additive and > not replaced. Just setting configs on an app bundle would imply to me that's the > full set of stuff getting set. It's because the line I removed at 184 is actually in the variables_to_not_forward_list, so configs was getting clobbered. We should prevent that from happening, so I re-added configs to the exclude list and kept extra_configs as the name.
https://codereview.chromium.org/1927273002/diff/60001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/60001/build/config/mac/rules.... build/config/mac/rules.gni:191: "configs", You should still not do this for the same reason I mentioned before. I'm currently fixing GN to make this a hard error. https://codereview.chromium.org/1927273002/diff/60001/build/config/mac/rules.... build/config/mac/rules.gni:197: configs += _extra_configs Can you replace this whole patch with: if (defined(invoker.extra_configs)) { configs += invoker.extra_configs } ?
https://codereview.chromium.org/1927273002/diff/60001/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/60001/build/config/mac/rules.... build/config/mac/rules.gni:191: "configs", On 2016/05/02 21:32:30, brettw wrote: > You should still not do this for the same reason I mentioned before. I'm > currently fixing GN to make this a hard error. Done. https://codereview.chromium.org/1927273002/diff/60001/build/config/mac/rules.... build/config/mac/rules.gni:197: configs += _extra_configs On 2016/05/02 21:32:30, brettw wrote: > Can you replace this whole patch with: > if (defined(invoker.extra_configs)) { > configs += invoker.extra_configs > } > ? Done, without the "invoker." since it's already forwarded from the above.
lgtm https://codereview.chromium.org/1927273002/diff/120001/build/config/mac/rules... File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/120001/build/config/mac/rules... build/config/mac/rules.gni:43: # Arguments Document the extra_configs variable here.
https://codereview.chromium.org/1927273002/diff/120001/build/config/mac/rules... File build/config/mac/rules.gni (right): https://codereview.chromium.org/1927273002/diff/120001/build/config/mac/rules... build/config/mac/rules.gni:43: # Arguments On 2016/05/02 22:27:19, brettw wrote: > Document the extra_configs variable here. Done.
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1927273002/#ps140001 (title: "Docs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927273002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927273002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Add a BUILD.gn file for //chrome/app_shim. BUG=431177 ========== to ========== [Mac/GN] Add a BUILD.gn file for //chrome/app_shim. BUG=431177 Committed: https://crrev.com/793e3f970d038015da28feaba6c53b18126a852a Cr-Commit-Position: refs/heads/master@{#391106} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/793e3f970d038015da28feaba6c53b18126a852a Cr-Commit-Position: refs/heads/master@{#391106} |
