Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1915863003:
[Mac/GN] Build content_shell and content_shell_framework. (Closed)
|
Created:
4 years, 8 months ago by Robert Sesek Modified:
4 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, Peter Beverloo, jam, darin-cc_chromium.org, mkwst+moarreviews-shell_chromium.org, jochen+watch_chromium.org, Nico, sdefresne Base URL:
https://chromium.googlesource.com/chromium/src.git@gn-info-plist Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/GN] Build content_shell and content_shell_framework.
BUG=431177
Committed: https://crrev.com/7f9d12b7bff8e9432808303e6e0f1a5349c116d5
Cr-Commit-Position: refs/heads/master@{#390581}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #Patch Set 4 : Bot fix #
Total comments: 2
Patch Set 5 : Bug link #Patch Set 6 : xcrun stamp #
Total comments: 2
Patch Set 7 : fix_helper_link_framework comment #
Messages
Total messages: 28 (8 generated)
rsesek@chromium.org changed reviewers: + dpranke@chromium.org
basically looks okay, but a couple of minor questions. https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn#newc... content/shell/BUILD.gn:568: } You only need declare_args() for things that will potentially be changed in args.gn (or overridden per toolchain). Is there some reason we'd want to support that for this? Otherwise you should be able to just assign these as variables. https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn#newc... content/shell/BUILD.gn:610: ":content_shell_framework+link", This is a target generated by the mac_framework_bundle() template, right? https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn#newc... content/shell/BUILD.gn:622: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", Do you need the trailing "." here so GN doesn't yell at you about having a trailing slash and thinking it's a directory (which might be forbidden)? Should that be a bug in GN (or is it already, but I've forgotten)?
https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn#newc... content/shell/BUILD.gn:568: } On 2016/04/25 21:29:19, Dirk Pranke wrote: > You only need declare_args() for things that will potentially be changed in > args.gn (or overridden per toolchain). Is there some reason we'd want to support > that for this? > > Otherwise you should be able to just assign these as variables. Done. https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn#newc... content/shell/BUILD.gn:610: ":content_shell_framework+link", On 2016/04/25 21:29:19, Dirk Pranke wrote: > This is a target generated by the mac_framework_bundle() template, right? Yup. https://codereview.chromium.org/1915863003/diff/1/content/shell/BUILD.gn#newc... content/shell/BUILD.gn:622: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", On 2016/04/25 21:29:19, Dirk Pranke wrote: > Do you need the trailing "." here so GN doesn't yell at you about having a > trailing slash and thinking it's a directory (which might be forbidden)? Should > that be a bug in GN (or is it already, but I've forgotten)? Oh I forgot to call this out explicitly as weird. I went with . because the input is the same as the output in this case (operating in-place). I can list MacOS/ instead as an alternative. GYP has the concept of "postbuilds" which allow you to perform an action in-place after the target is built, but GN doesn't seem to have an analog. Rather than creating yet another intermediate that needs to be copied, I decided to do this.
dpranke@chromium.org changed reviewers: + brettw@chromium.org
lgtm so that we can get this in and make progress, but the postbuild workaround is indeed unorthodox and should probably be improved. +brettw in case he's really offended by this, +sdefresne and +thakis in case they have thoughts. https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn#... content/shell/BUILD.gn:620: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", Yeah, the 'MacOS/.' is just weird; I'd use 'MacOS/' instead. https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn#... content/shell/BUILD.gn:620: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", Does this work with 'MacOS/$content_shell_helper_name' instead? It seems like that's probably a bit more representative of what's really going on. Then again, I could believe that this would mess ninja and/or GN up big time. Perhaps we need to modify mac_app_bundle() to take arguments that can combine this into a single step instead. Maybe file a TODO() to follow-up on this?
https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn#... content/shell/BUILD.gn:620: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", On 2016/04/25 21:50:28, Dirk Pranke wrote: > Yeah, the 'MacOS/.' is just weird; I'd use 'MacOS/' instead. Done. https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn#... content/shell/BUILD.gn:620: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", On 2016/04/25 21:50:28, Dirk Pranke wrote: > Does this work with 'MacOS/$content_shell_helper_name' instead? It seems like > that's probably a bit more representative of what's really going on. > > Then again, I could believe that this would mess ninja and/or GN up big time. That's what I wanted to do, but ninja/GN then complain about two targets generate the same output. > Perhaps we need to modify mac_app_bundle() to take arguments that can combine > this into a single step instead. I don't think any other targets require this (Chrome doesn't). > Maybe file a TODO() to follow-up on this? Done.
brettw: Any thoughts? (Specifically on the question about postbuilds).
thakis@chromium.org changed reviewers: + thakis@chromium.org
https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/20001/content/shell/BUILD.gn#... content/shell/BUILD.gn:620: "$root_out_dir/$content_shell_helper_name.app/Contents/MacOS/.", On 2016/04/25 22:08:00, Robert Sesek wrote: > On 2016/04/25 21:50:28, Dirk Pranke wrote: > > Does this work with 'MacOS/$content_shell_helper_name' instead? It seems like > > that's probably a bit more representative of what's really going on. > > > > Then again, I could believe that this would mess ninja and/or GN up big time. > > That's what I wanted to do, but ninja/GN then complain about two targets > generate the same output. The output is the directory containing the input? That seems super hacky -- can't this just write a stamp file instead? > > > Perhaps we need to modify mac_app_bundle() to take arguments that can combine > > this into a single step instead. > > I don't think any other targets require this (Chrome doesn't). > > > Maybe file a TODO() to follow-up on this? > > Done. https://codereview.chromium.org/1915863003/diff/60001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/60001/content/shell/BUILD.gn#... content/shell/BUILD.gn:621: # install_name_tool to operate in-place. gyp's postbuilds compile to "&& foo" on the link command (roughly) in gyp. You could change the toolchain definition to allow tagging on something to the link line and set that here.
https://codereview.chromium.org/1915863003/diff/60001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/60001/content/shell/BUILD.gn#... content/shell/BUILD.gn:621: # install_name_tool to operate in-place. On 2016/04/26 17:35:48, Nico wrote: > gyp's postbuilds compile to "&& foo" on the link command (roughly) in gyp. You > could change the toolchain definition to allow tagging on something to the link > line and set that here. Just applying it to the link command isn't sufficient, I think, since we also need a postbuild for things like tweak_info_plist.py. That's why I'm letting this be gross for now until we find a real solution to replace postbuilds.
I don't know what to do. Maybe we do need a built-in concept of post-build actions. For that, I want to take stock of all of our needs in this area and then design the cleanest thing that handles those. At least we should file a bug, reference the bug from the code and the code from the bug. If we have this one gross place, will this immediately come up in a bunch of other places or will we be OK for a while?
On 2016/04/27 20:14:40, brettw wrote: > I don't know what to do. Maybe we do need a built-in concept of post-build > actions. For that, I want to take stock of all of our needs in this area and > then design the cleanest thing that handles those. > > At least we should file a bug, reference the bug from the code and the code from > the bug. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=607292 and linked it in the comment. > If we have this one gross place, will this immediately come up in a bunch of > other places or will we be OK for a while? This will come up in a few places, but not many. A lot of postbuidls are simply copy actions, which can be restructured into proper dependencies. The ones that can't either produce no output (like verify_order) or operate on the same file as the input (like tweak_info_plist). (You can see all the postbuilds here: https://code.google.com/p/chromium/codesearch#search/&q=postbuilds%20file:gyp...)
In that case I'm inclined not to block on this for now and check in something that works.
On 2016/04/27 21:24:29, brettw wrote: > In that case I'm inclined not to block on this for now and check in something > that works. Can I get an LG then :-)? I also think there's a way to remove this postbuild entirely, but it'll require rewriting content_shell_main to not rely on dyld to load the framework. I can do that after GYP is gone (just so I don't have to muddle with that build to make it work).
Per our offline discussion, I've added a --stamp capability to xcrun.
lgtm https://codereview.chromium.org/1915863003/diff/100001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/100001/content/shell/BUILD.gn... content/shell/BUILD.gn:613: action("fix_helper_link_framework") { Can you give an overview here as a comment about how this stamp thing works with the sequence of dependencies to get the outcome you need?
Thanks! https://codereview.chromium.org/1915863003/diff/100001/content/shell/BUILD.gn File content/shell/BUILD.gn (right): https://codereview.chromium.org/1915863003/diff/100001/content/shell/BUILD.gn... content/shell/BUILD.gn:613: action("fix_helper_link_framework") { On 2016/04/28 20:27:48, brettw wrote: > Can you give an overview here as a comment about how this stamp thing works with > the sequence of dependencies to get the outcome you need? Done.
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1915863003/#ps120001 (title: "fix_helper_link_framework comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915863003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915863003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rsesek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1915863003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1915863003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7f9d12b7bff8e9432808303e6e0f1a5349c116d5 Cr-Commit-Position: refs/heads/master@{#390581}
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Build content_shell and content_shell_framework. BUG=431177 ========== to ========== [Mac/GN] Build content_shell and content_shell_framework. BUG=431177 Committed: https://crrev.com/7f9d12b7bff8e9432808303e6e0f1a5349c116d5 Cr-Commit-Position: refs/heads/master@{#390581} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
