Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1981723002:
media: Set RUNPATH in cdm adapter in GN component build (Closed)
Descriptionmedia: Set RUNPATH in cdm adapter in GN component build
The CDM should always be able to find the CDM, regardless of how the
loader of the CDM adapter is configured.
In tests, currently, browser_tests sets a RPATH of $ORIGIN, which
makes the test work. But if we put the CDM adapter and CDM in a
different folder, then the test will break in GN component build.
See BUG for more detailed discussion.
BUG=612007
TEST=browser_tests pass in GN component build
Committed: https://crrev.com/91b24b67d6ac964927c3a008fc16b2c9a068f7f7
Cr-Commit-Position: refs/heads/master@{#394272}
Patch Set 1 #
Total comments: 4
Messages
Total messages: 14 (5 generated)
xhwang@chromium.org changed reviewers: + ddorwin@chromium.org, thestig@chromium.org
PTAL
Some comments, but I defer to thestig@. https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.gni (right): https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.gni:64: configs += [ "//build/config/gcc:rpath_for_built_shared_libraries" ] I know we need to set rpath for the adapter, but two things: 1. The warning not to use this for release builds: https://code.google.com/p/chromium/codesearch#chromium/src/build/config/gcc/B... 2. It seems this isn't intended to do what we want (bind to the same directory). If shlib_subdir was set, I don't think this would do what we want.
https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.gni (right): https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.gni:64: configs += [ "//build/config/gcc:rpath_for_built_shared_libraries" ] On 2016/05/16 18:11:12, ddorwin wrote: > I know we need to set rpath for the adapter, but two things: > 1. The warning not to use this for release builds: > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/gcc/B... This was discussed when we added this back. See https://bugs.chromium.org/p/chromium/issues/detail?id=583009#c18 > 2. It seems this isn't intended to do what we want (bind to the same directory). > If shlib_subdir was set, I don't think this would do what we want. Good point. Maybe we should just manually set rpath here? I don't know what's the plan to use a separate shlib_subdir for GN. At some point when we deprecate cdm adapters this problem will go away.
LGTM % comments. https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.gni (right): https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.gni:64: configs += [ "//build/config/gcc:rpath_for_built_shared_libraries" ] On 2016/05/16 18:23:07, xhwang wrote: > On 2016/05/16 18:11:12, ddorwin wrote: > > I know we need to set rpath for the adapter, but two things: > > 1. The warning not to use this for release builds: > > > https://code.google.com/p/chromium/codesearch#chromium/src/build/config/gcc/B... > > This was discussed when we added this back. See > https://bugs.chromium.org/p/chromium/issues/detail?id=583009#c18 thestig@: Should we update the documentation in the source link above? > > > 2. It seems this isn't intended to do what we want (bind to the same > directory). > > If shlib_subdir was set, I don't think this would do what we want. > > Good point. Maybe we should just manually set rpath here? I don't know what's > the plan to use a separate shlib_subdir for GN. At some point when we deprecate > cdm adapters this problem will go away. If possible, that would be best. If not, then we'll just have to live with it until then.
Description was changed from ========== media: Set RUNPATH in cdm adapter in GN component build The CDM should always be able to find the CDM, regardless of how the loader of the CDM adapter is configed. In tests, currently, browser_tests sets a RPATH of $ORIGIN, which makes the test work. But if we put the CDM adapter and CDM in a different folder, then the test will break in GN component build. See BUG for more detailed discussion. BUG=612007 TEST=browser_tests pass in GN component build ========== to ========== media: Set RUNPATH in cdm adapter in GN component build The CDM should always be able to find the CDM, regardless of how the loader of the CDM adapter is configured. In tests, currently, browser_tests sets a RPATH of $ORIGIN, which makes the test work. But if we put the CDM adapter and CDM in a different folder, then the test will break in GN component build. See BUG for more detailed discussion. BUG=612007 TEST=browser_tests pass in GN component build ==========
thestig: kindly ping!
lgtm https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... File media/cdm/ppapi/ppapi_cdm_adapter.gni (right): https://chromiumcodereview.appspot.com/1981723002/diff/1/media/cdm/ppapi/ppap... media/cdm/ppapi/ppapi_cdm_adapter.gni:64: configs += [ "//build/config/gcc:rpath_for_built_shared_libraries" ] On 2016/05/16 18:28:46, ddorwin wrote: > thestig@: Should we update the documentation in the source link above? Meh. The config("executable_ldconfig") section in build/config/gcc/BUILD.gn normally already takes caer of the component build case. ppapi_cdm_adapter is probably one of the few exceptions.
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981723002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981723002/1
Message was sent while issue was closed.
Description was changed from ========== media: Set RUNPATH in cdm adapter in GN component build The CDM should always be able to find the CDM, regardless of how the loader of the CDM adapter is configured. In tests, currently, browser_tests sets a RPATH of $ORIGIN, which makes the test work. But if we put the CDM adapter and CDM in a different folder, then the test will break in GN component build. See BUG for more detailed discussion. BUG=612007 TEST=browser_tests pass in GN component build ========== to ========== media: Set RUNPATH in cdm adapter in GN component build The CDM should always be able to find the CDM, regardless of how the loader of the CDM adapter is configured. In tests, currently, browser_tests sets a RPATH of $ORIGIN, which makes the test work. But if we put the CDM adapter and CDM in a different folder, then the test will break in GN component build. See BUG for more detailed discussion. BUG=612007 TEST=browser_tests pass in GN component build ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== media: Set RUNPATH in cdm adapter in GN component build The CDM should always be able to find the CDM, regardless of how the loader of the CDM adapter is configured. In tests, currently, browser_tests sets a RPATH of $ORIGIN, which makes the test work. But if we put the CDM adapter and CDM in a different folder, then the test will break in GN component build. See BUG for more detailed discussion. BUG=612007 TEST=browser_tests pass in GN component build ========== to ========== media: Set RUNPATH in cdm adapter in GN component build The CDM should always be able to find the CDM, regardless of how the loader of the CDM adapter is configured. In tests, currently, browser_tests sets a RPATH of $ORIGIN, which makes the test work. But if we put the CDM adapter and CDM in a different folder, then the test will break in GN component build. See BUG for more detailed discussion. BUG=612007 TEST=browser_tests pass in GN component build Committed: https://crrev.com/91b24b67d6ac964927c3a008fc16b2c9a068f7f7 Cr-Commit-Position: refs/heads/master@{#394272} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/91b24b67d6ac964927c3a008fc16b2c9a068f7f7 Cr-Commit-Position: refs/heads/master@{#394272} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
