On Sun, Mar 4, 2012 at 5:18 PM, Stephen Kelly <steve...@gmail.com> wrote:
>> That's a good start. However I do not think the logic plays well
>> with BUILD_WITH_INSTALL_RPATH as currently written. If that is
>> set then the build tree should end up with the install-tree rpath
>> which should be empty if SKIP_INSTALL_RPATH is on. Therefore
>> the relink/chrpath paths should not need to return true always
>> when SKIP_INSTALL_RPATH is on.
>
> Sorry this took so long. I have updated the branch (It is now
> e96c16ae23885be2e66d67291344369585ebfece)
> - this->Target->GetPropertyAsBool("INSTALL_RPATH_USE_LINK_PATH");
> + this->Target->GetPropertyAsBool("INSTALL_RPATH_USE_LINK_PATH") &&
> + !(for_install && this->Makefile->IsOn("CMAKE_SKIP_INSTALL_RPATH"));
s/for_install/linking_for_install/ because BUILD_WITH_INSTALL_RPATH
should cause the build tree to get the install tree rpath which is
empty when CMAKE_SKIP_INSTALL_RPATH is ON.
> cm->DefineProperty
> + ("CMAKE_SKIP_INSTALL_RPATH", cmProperty::VARIABLE,
> + "Do not include RPATHs in the install tree.",
Please update the documentation of both this variable and
CMAKE_SKIP_RPATH so each references the other. We want packagers
using CMAKE_SKIP_RPATH to discover CMAKE_SKIP_INSTALL_RPATH.
> + this->SetPropertyDefault("SKIP_INSTALL_RPATH", 0);
This is unnecessary because it is not a target-level property. The
new SKIP variable is supposed to be a packager's hammer so we cannot
let projects turn it off for some targets but not others.
> @@ -3625,28 +3627,33 @@ bool cmTarget::NeedRelinkBeforeInstall(const char*
> config)
...
> + if(this->Makefile->IsOn("CMAKE_SKIP_INSTALL_RPATH"))
> + {
> + return true;
> + }
> +
> // If chrpath is going to be used no relinking is needed.
> if(this->IsChrpathUsed(config))
> {
> return false;
> }
...
> @@ -4013,28 +4021,33 @@ bool cmTarget::IsChrpathUsed(const char* config)
...
> + if(this->Makefile->IsOn("CMAKE_SKIP_INSTALL_RPATH"))
> + {
> + return true;
> + }
> +
> // Allow the user to disable builtin chrpath explicitly.
> if(this->Makefile->IsOn("CMAKE_NO_BUILTIN_CHRPATH"))
> {
> return false;
> }
Drop these tests. These two methods are about deciding how to rewrite
the RPATH during installation. The code below these hunks decides
whether it is even possible to do.
Just changes to
cmComputeLinkInformation::GetRPath
cmTarget::HaveInstallTreeRPATH
cmTarget::GetInstallNameDirForInstallTree
should be enough for the rest of the logic to work.
Thanks,
-Brad
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
