[cmake-developers] Modifying RPATH feature to run tests uninstalled

[Stephen Kelly]

Brad King wrote:

> On Sun, Mar 4, 2012 at 5:18 PM, Stephen Kelly
> <steveire at 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.