Brad King wrote:
> 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.
linking_for_install is already part of the boolean expression. I just
removed the for_install in the latest version of this patch.
>
>> 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.
Done.
>
>> + 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.
Done.
>
>> @@ -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.
Done.
e1b4fec8dd73cc675b3a20f2e045c6282c47553a in my gitorious clone contains the
updated state. It behaved as I expected in all combinations I tested.
I can push it to stage/next whenever.
Thanks,
Steve.
--
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
