Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1980173002:
[Mac/GN] Add verify_order step for the Framework. (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@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Mac/GN] Add verify_order step for the Framework.
BUG=431177, 599950
R=thakis@chromium.org
Committed: https://crrev.com/ea148577d6315da90892f854b52a1f3a8380c562
Cr-Commit-Position: refs/heads/master@{#394438}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fix comment #
Messages
Total messages: 28 (6 generated)
Nice, lgtm with the first comment addressed. (The other two are just "gn, how does it work?" questions -- maybe add a comment for these, or don't.) Hooray for this finding a bunch of exported symbols, not hooray for us not noticing before flipping linux. https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode762 chrome/BUILD.gn:762: "-Wl,-order_file," + rebase_path("app/framework.order", root_build_dir), Something should probably take framework.order as an input, so that changes to it cause a relink. https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode777 chrome/BUILD.gn:777: stamp_file = "$target_out_dir/run_$target_name.stamp" what's $target_name? Neither `gn help target_name` nor `gn help action` say, and I can't see it defined further up in this file. https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode787 chrome/BUILD.gn:787: "$root_out_dir/$chrome_framework_name.framework/$chrome_framework_name", Hm, same for chrome_framework_name, and cs.chromium.org seems to not know about that either.
https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode762 chrome/BUILD.gn:762: "-Wl,-order_file," + rebase_path("app/framework.order", root_build_dir), On 2016/05/16 21:59:33, Nico (vacation May 19 to 22) wrote: > Something should probably take framework.order as an input, so that changes to > it cause a relink. Hmm I agree but I can't seem to make it happen. Listing it as a sources[] does not work. I thought about listing it as an input to verify_chrome_framework_order, but that of course happens after the link... https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode777 chrome/BUILD.gn:777: stamp_file = "$target_out_dir/run_$target_name.stamp" On 2016/05/16 21:59:33, Nico (vacation May 19 to 22) wrote: > what's $target_name? Neither `gn help target_name` nor `gn help action` say, and > I can't see it defined further up in this file. target_name would be verify_chrome_framework_order in this case. It's the second argument to target(type, name). https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode787 chrome/BUILD.gn:787: "$root_out_dir/$chrome_framework_name.framework/$chrome_framework_name", On 2016/05/16 21:59:33, Nico (vacation May 19 to 22) wrote: > Hm, same for chrome_framework_name, and http://cs.chromium.org seems to not know about > that either. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/BUILD.gn&q=... ?
thakis@chromium.org changed reviewers: + dpranke@chromium.org
(must've typod my cs.chromium search, sorry.) dpranke: One question below. https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode762 chrome/BUILD.gn:762: "-Wl,-order_file," + rebase_path("app/framework.order", root_build_dir), On 2016/05/16 22:07:13, Robert Sesek wrote: > On 2016/05/16 21:59:33, Nico (vacation May 19 to 22) wrote: > > Something should probably take framework.order as an input, so that changes to > > it cause a relink. > > Hmm I agree but I can't seem to make it happen. Listing it as a sources[] does > not work. I thought about listing it as an input to > verify_chrome_framework_order, but that of course happens after the link... dpranke: How can we tell gn to rerun the link of this target if app/framework.order is touched? It's not a source file...is it possible to give targets a dependency on an arbitrary input file?
Also: python ../../chrome/tools/build/mac/run_verify_order.py --stamp obj/chrome/run_verify_chrome_framework_order.stamp _ChromeMain Chromium\ Framework.framework/Chromium\ Framework ../../chrome/tools/build/mac/verify_order: unordered symbols in Chromium Framework.framework/Chromium Framework: _strnlen
On 2016/05/16 22:15:00, Nico (vacation May 19 to 22) wrote: > Also: > python ../../chrome/tools/build/mac/run_verify_order.py --stamp > obj/chrome/run_verify_chrome_framework_order.stamp _ChromeMain Chromium\ > Framework.framework/Chromium\ Framework > ../../chrome/tools/build/mac/verify_order: unordered symbols in Chromium > Framework.framework/Chromium Framework: > _strnlen Yeah just saw that, trying to repro now.
https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode524 chrome/BUILD.gn:524: # for non-compoent builds, will ensure the export symbol table is correct. s/compoent/component/. https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode762 chrome/BUILD.gn:762: "-Wl,-order_file," + rebase_path("app/framework.order", root_build_dir), On 2016/05/16 22:14:29, Nico (vacation May 19 to 22) wrote: > On 2016/05/16 22:07:13, Robert Sesek wrote: > > On 2016/05/16 21:59:33, Nico (vacation May 19 to 22) wrote: > > > Something should probably take framework.order as an input, so that changes > to > > > it cause a relink. > > > > Hmm I agree but I can't seem to make it happen. Listing it as a sources[] does > > not work. I thought about listing it as an input to > > verify_chrome_framework_order, but that of course happens after the link... > > dpranke: How can we tell gn to rerun the link of this target if > app/framework.order is touched? It's not a source file...is it possible to give > targets a dependency on an arbitrary input file? Did you try setting the `inputs = [ ... ]` variable? I would expect that to work, and it looks like the value should get plumbed through to the shared_library where it's needed, but I haven't tested this, so there might be bugs ...
On 2016/05/16 23:55:32, Dirk Pranke wrote: > https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn > File chrome/BUILD.gn (right): > > https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode524 > chrome/BUILD.gn:524: # for non-compoent builds, will ensure the export symbol > table is correct. > s/compoent/component/. > > https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode762 > chrome/BUILD.gn:762: "-Wl,-order_file," + rebase_path("app/framework.order", > root_build_dir), > On 2016/05/16 22:14:29, Nico (vacation May 19 to 22) wrote: > > On 2016/05/16 22:07:13, Robert Sesek wrote: > > > On 2016/05/16 21:59:33, Nico (vacation May 19 to 22) wrote: > > > > Something should probably take framework.order as an input, so that > changes > > to > > > > it cause a relink. > > > > > > Hmm I agree but I can't seem to make it happen. Listing it as a sources[] > does > > > not work. I thought about listing it as an input to > > > verify_chrome_framework_order, but that of course happens after the link... > > > > dpranke: How can we tell gn to rerun the link of this target if > > app/framework.order is touched? It's not a source file...is it possible to > give > > targets a dependency on an arbitrary input file? > > Did you try setting the `inputs = [ ... ]` variable? > > I would expect that to work, and it looks like the value should get plumbed > through to the shared_library where it's needed, but I haven't tested this, so > there might be bugs ... No, inputs[] doesn't work either. The shared library target should have all invoker variables forwarded, so I'm not sure why that doesn't work.
On 2016/05/17 14:45:38, Robert Sesek wrote: > On 2016/05/16 23:55:32, Dirk Pranke wrote: > > Did you try setting the `inputs = [ ... ]` variable? > > > > I would expect that to work, and it looks like the value should get plumbed > > through to the shared_library where it's needed, but I haven't tested this, so > > there might be bugs ... > > No, inputs[] doesn't work either. The shared library target should have all > invoker variables forwarded, so I'm not sure why that doesn't work. Okay, I'll take a look and see if I can figure out why inputs doesn't work. I don't know of an alternative off the top of my head.
On 2016/05/16 22:15:32, Robert Sesek wrote: > On 2016/05/16 22:15:00, Nico (vacation May 19 to 22) wrote: > > Also: > > python ../../chrome/tools/build/mac/run_verify_order.py --stamp > > obj/chrome/run_verify_chrome_framework_order.stamp _ChromeMain Chromium\ > > Framework.framework/Chromium\ Framework > > ../../chrome/tools/build/mac/verify_order: unordered symbols in Chromium > > Framework.framework/Chromium Framework: > > _strnlen > > Yeah just saw that, trying to repro now. For posterity: This is caused by https://code.google.com/p/chromium/codesearch#chromium/src/native_client/src/.... In the GYP build, the solibs part of the link command comes before the rsp file, which means libSystem gets linked in first and claims undefined references to strnlen. In the GN build, the rsp comes before solibs and so the NaCl version of strnlen is picked up. But since strnlen is now part of the system (since 10.7) we can do away with this NaCl file.
On 2016/05/16 22:15:00, Nico (vacation May 19 to 22) wrote: > Also: > python ../../chrome/tools/build/mac/run_verify_order.py --stamp > obj/chrome/run_verify_chrome_framework_order.stamp _ChromeMain Chromium\ > Framework.framework/Chromium\ Framework > ../../chrome/tools/build/mac/verify_order: unordered symbols in Chromium > Framework.framework/Chromium Framework: > _strnlen Trybots now green.
On 2016/05/17 16:28:48, Dirk Pranke wrote: > On 2016/05/17 14:45:38, Robert Sesek wrote: > > On 2016/05/16 23:55:32, Dirk Pranke wrote: > > > Did you try setting the `inputs = [ ... ]` variable? > > > > > > I would expect that to work, and it looks like the value should get plumbed > > > through to the shared_library where it's needed, but I haven't tested this, > so > > > there might be bugs ... > > > > No, inputs[] doesn't work either. The shared library target should have all > > invoker variables forwarded, so I'm not sure why that doesn't work. > > Okay, I'll take a look and see if I can figure out why inputs doesn't work. I > don't know > of an alternative off the top of my head. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=612623
https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn File chrome/BUILD.gn (right): https://codereview.chromium.org/1980173002/diff/1/chrome/BUILD.gn#newcode524 chrome/BUILD.gn:524: # for non-compoent builds, will ensure the export symbol table is correct. On 2016/05/16 23:55:32, Dirk Pranke wrote: > s/compoent/component/. Done.
The CQ bit was checked by rsesek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1980173002/#ps20001 (title: "Fix comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980173002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980173002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/05/17 16:28:48, Dirk Pranke wrote: > On 2016/05/17 14:45:38, Robert Sesek wrote: > > On 2016/05/16 23:55:32, Dirk Pranke wrote: > > > Did you try setting the `inputs = [ ... ]` variable? > > > > > > I would expect that to work, and it looks like the value should get plumbed > > > through to the shared_library where it's needed, but I haven't tested this, > so > > > there might be bugs ... > > > > No, inputs[] doesn't work either. The shared library target should have all > > invoker variables forwarded, so I'm not sure why that doesn't work. > > Okay, I'll take a look and see if I can figure out why inputs doesn't work. I > don't know > of an alternative off the top of my head. Okay, I just patched this CL in locally on a tip-of-tree mac checkout, and added inputs = [ "app/framework.order" ] at line 718 of chrome/BUILD.gn and regenerated the build files, and see "build obj/chrome/chrome_dll.inputdeps.stamp : stamp ../../chrome/app/framework.order ..." in out/foo/obj/chrome/chrome_dll.ninja (for a static build, but also for a component build). This is what I'd expect to see (and what I think is needed). Do you see something different, or did you maybe try adding the inputs line somewhere else? I'm trying a compile now to see if it actually does what I think it should do.
On 2016/05/18 02:02:16, Dirk Pranke wrote: > On 2016/05/17 16:28:48, Dirk Pranke wrote: > > On 2016/05/17 14:45:38, Robert Sesek wrote: > > > On 2016/05/16 23:55:32, Dirk Pranke wrote: > > > > Did you try setting the `inputs = [ ... ]` variable? > > > > > > > > I would expect that to work, and it looks like the value should get > plumbed > > > > through to the shared_library where it's needed, but I haven't tested > this, > > so > > > > there might be bugs ... > > > > > > No, inputs[] doesn't work either. The shared library target should have all > > > invoker variables forwarded, so I'm not sure why that doesn't work. > > > > Okay, I'll take a look and see if I can figure out why inputs doesn't work. I > > don't know > > of an alternative off the top of my head. > > Okay, I just patched this CL in locally on a tip-of-tree mac checkout, and > > added > > inputs = [ "app/framework.order" ] > > at line 718 of chrome/BUILD.gn > > and regenerated the build files, and see "build > obj/chrome/chrome_dll.inputdeps.stamp : stamp ../../chrome/app/framework.order > ..." > in out/foo/obj/chrome/chrome_dll.ninja (for a static build, but also for a > component build). > > This is what I'd expect to see (and what I think is needed). > > Do you see something different, or did you maybe try adding the inputs line > somewhere else? > > I'm trying a compile now to see if it actually does what I think it should do. I see now that you were talking about setting this on :chrome_framework, not on :chrome_dll, and I actually changed the latter, which I think indirectly will make chrome_framework be out of date. It looks like adding inputs to :chrome_framework propagates them to :chrome_framework_shared_library as well ...
On 2016/05/18 02:40:14, Dirk Pranke wrote: > On 2016/05/18 02:02:16, Dirk Pranke wrote: > > On 2016/05/17 16:28:48, Dirk Pranke wrote: > > > On 2016/05/17 14:45:38, Robert Sesek wrote: > > > > On 2016/05/16 23:55:32, Dirk Pranke wrote: > > > > > Did you try setting the `inputs = [ ... ]` variable? > > > > > > > > > > I would expect that to work, and it looks like the value should get > > plumbed > > > > > through to the shared_library where it's needed, but I haven't tested > > this, > > > so > > > > > there might be bugs ... > > > > > > > > No, inputs[] doesn't work either. The shared library target should have > all > > > > invoker variables forwarded, so I'm not sure why that doesn't work. > > > > > > Okay, I'll take a look and see if I can figure out why inputs doesn't work. > I > > > don't know > > > of an alternative off the top of my head. > > > > Okay, I just patched this CL in locally on a tip-of-tree mac checkout, and > > > > added > > > > inputs = [ "app/framework.order" ] > > > > at line 718 of chrome/BUILD.gn > > > > and regenerated the build files, and see "build > > obj/chrome/chrome_dll.inputdeps.stamp : stamp ../../chrome/app/framework.order > > ..." > > in out/foo/obj/chrome/chrome_dll.ninja (for a static build, but also for a > > component build). > > > > This is what I'd expect to see (and what I think is needed). > > > > Do you see something different, or did you maybe try adding the inputs line > > somewhere else? > > > > I'm trying a compile now to see if it actually does what I think it should do. > > I see now that you were talking about setting this on :chrome_framework, not on > :chrome_dll, > and I actually changed the latter, which I think indirectly will make > chrome_framework be > out of date. It looks like adding inputs to :chrome_framework propagates them to > :chrome_framework_shared_library as well ... Okay, it looks like GN is operating per spec: "Inputs for binary targets will be treated as order-only dependencies, meaning that they will be forced up-to-date before compiling or any files in the target, but changes in the inputs will not necessarily force the target to compile. This is because it is expected that the compiler will report the precise list of input dependencies required to recompile each file once the initial build is done." (from `gn help inputs`). Further comments in bug 612623.
The CQ bit was checked by rsesek@chromium.org
On 2016/05/18 04:55:23, Dirk Pranke wrote: > On 2016/05/18 02:40:14, Dirk Pranke wrote: > > On 2016/05/18 02:02:16, Dirk Pranke wrote: > > > On 2016/05/17 16:28:48, Dirk Pranke wrote: > > > > On 2016/05/17 14:45:38, Robert Sesek wrote: > > > > > On 2016/05/16 23:55:32, Dirk Pranke wrote: > > > > > > Did you try setting the `inputs = [ ... ]` variable? > > > > > > > > > > > > I would expect that to work, and it looks like the value should get > > > plumbed > > > > > > through to the shared_library where it's needed, but I haven't tested > > > this, > > > > so > > > > > > there might be bugs ... > > > > > > > > > > No, inputs[] doesn't work either. The shared library target should have > > all > > > > > invoker variables forwarded, so I'm not sure why that doesn't work. > > > > > > > > Okay, I'll take a look and see if I can figure out why inputs doesn't > work. > > I > > > > don't know > > > > of an alternative off the top of my head. > > > > > > Okay, I just patched this CL in locally on a tip-of-tree mac checkout, and > > > > > > added > > > > > > inputs = [ "app/framework.order" ] > > > > > > at line 718 of chrome/BUILD.gn > > > > > > and regenerated the build files, and see "build > > > obj/chrome/chrome_dll.inputdeps.stamp : stamp > ../../chrome/app/framework.order > > > ..." > > > in out/foo/obj/chrome/chrome_dll.ninja (for a static build, but also for a > > > component build). > > > > > > This is what I'd expect to see (and what I think is needed). > > > > > > Do you see something different, or did you maybe try adding the inputs line > > > somewhere else? > > > > > > I'm trying a compile now to see if it actually does what I think it should > do. > > > > I see now that you were talking about setting this on :chrome_framework, not > on > > :chrome_dll, > > and I actually changed the latter, which I think indirectly will make > > chrome_framework be > > out of date. It looks like adding inputs to :chrome_framework propagates them > to > > :chrome_framework_shared_library as well ... > > Okay, it looks like GN is operating per spec: > > "Inputs for binary targets will be treated as order-only dependencies, meaning > that they will be forced up-to-date before compiling or any files in the target, > > but changes in the inputs will not necessarily force the target to compile. > This is because it is expected that the compiler will report the precise list > of input dependencies required to recompile each file once the initial build is > done." > > (from `gn help inputs`). > > Further comments in bug 612623. Thanks for digging into that and finding the issue!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980173002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980173002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [Mac/GN] Add verify_order step for the Framework. BUG=431177,599950 R=thakis@chromium.org ========== to ========== [Mac/GN] Add verify_order step for the Framework. BUG=431177,599950 R=thakis@chromium.org Committed: https://crrev.com/ea148577d6315da90892f854b52a1f3a8380c562 Cr-Commit-Position: refs/heads/master@{#394438} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ea148577d6315da90892f854b52a1f3a8380c562 Cr-Commit-Position: refs/heads/master@{#394438} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
