liblas: Duplicate -DCMAKE_INSTALL_NAME_DIR=${prefix}/lib flag and reintroduction of rpath

[ryandesign (Ryan Carsten Schmidt)]

In [0f281244b3d78d09dd057034a5e386c6f0e353e1/macports-ports] a duplicate -DCMAKE_INSTALL_NAME_DIR=${prefix}/lib flag was added to the port. This is pointless since the cmake portgroup already adds this so the duplicate flag should be removed.

So the only real effect of that commit was to change the install name of the libraries from ones that don't use rpath to ones that do:

Before:

$ otool -L ./liblas-1.8.1.99_1.darwin_17.x86_64/opt/local/lib/liblas.dylib | head -n 2
./liblas-1.8.1.99_1.darwin_17.x86_64/opt/local/lib/liblas.dylib:
	/opt/local/lib/liblas.2.4.0.dylib (compatibility version 3.0.0, current version 2.4.0)
$ otool -L ./liblas-1.8.1.99_1.darwin_17.x86_64/opt/local/lib/liblas_c.dylib | head -n 2
./liblas-1.8.1.99_1.darwin_17.x86_64/opt/local/lib/liblas_c.dylib:
	/opt/local/lib/liblas_c.2.4.0.dylib (compatibility version 3.0.0, current version 2.4.0)

After:

$ otool -L ./liblas-1.8.1.99_2.darwin_17.x86_64/opt/local/lib/liblas.dylib | head -n 2
./liblas-1.8.1.99_2.darwin_17.x86_64/opt/local/lib/liblas.dylib:
	@rpath/liblas.3.dylib (compatibility version 3.0.0, current version 2.4.0)
$ otool -L ./liblas-1.8.1.99_2.darwin_17.x86_64/opt/local/lib/liblas_c.dylib | head -n 2
./liblas-1.8.1.99_2.darwin_17.x86_64/opt/local/lib/liblas_c.dylib:
	@rpath/liblas_c.3.dylib (compatibility version 3.0.0, current version 2.4.0)

Is this reintroduction of rpath ok? I don't know. Vincent deliberately changed the port so that it did not use rpath in [3d0aac3095640e42fabc64de06f9e16427d2664f/macports-ports] but the commit message doesn't say why. Vincent, do you remember why? Possibly it was just the usual reason, which is that we don't usually use rpath, for one thing because rpath is not compatible with Mac OS X 10.4. And doesn't it also require every other program/library that uses the library to specify the rpath?

Because the install name of the libraries has changed, all programs/libraries that link with this library must be revbumped to use the new install name.

[kencu (Ken)]

approved by both mojca and clemens...

[mathstuf (Ben Boeckel)]

Adapting the emails I sent:

In [0f281244b3d78d09dd057034a5e386c6f0e353e1/macports-ports] a duplicate -DCMAKE_INSTALL_NAME_DIR=${prefix}/lib flag was added to the port. This is pointless since the cmake portgroup already adds this so the duplicate flag should be removed.

Ah, I didn't see that it was already there. Mainly because the install name should be $prefix/lib with this patch, not @rpath as you're seeing. I'm pretty sure I had absolute paths in my library IDs when I tested it locally, but maybe that wasn't true after the rebase.

The main issue with @rpath is that an -Wl,-rpath,$dir needs added to all linking executables in order to find any libraries using @rpath. CMake doesn't do this today (mainly because it doesn't know which linked-to libraries are using @rpath or how deep they expect to be from there). For example, Qt frameworks use @rpath/QtCore.framework/QtCore, so you need to go up 2 directories to get the proper rpath entry for that.

Is this reintroduction of rpath ok? I don't know. Vincent deliberately changed the port so that it did not use rpath in [3d0aac3095640e42fabc64de06f9e16427d2664f/macports-ports] but the commit message doesn't say why. Vincent, do you remember why? Possibly it was just the usual reason, which is that we don't usually use rpath, for one thing because rpath is not compatible with Mac OS X 10.4. And doesn't it also require every other program/library that uses the library to specify the rpath?

Unfortunately, that does not come with an explanation about that specific bit. But yes, that is the basic issue with @rpath on macOS.

Because the install name of the libraries has changed, all programs/libraries that link with this library must be revbumped to use the new install name.

Technically, no. The library ID is copied at link time into the consuming executable. But I can see the argument for consistency. So the old path is still used to look up the current libraries. Ideally, it wouldn't be doing @rpath even now, but something is busted, so let's look at that.

The reason this came to my attention is because the manual patching was not correct on a machine I was working on (the symlink name was used in the ID, not the actual library).

On Fri, Apr 17, 2020 at 18:14:58 -0400, Ben Boeckel wrote:

[ Not sure if replying here is reflected on the tracker. ]

Well, the list rejected my email. Taking them off of this Cc.

For the record, here was my first stab at fixing the problem I had seen:

​https://github.com/macports/macports-ports/commit/2d27d0de01ed80c9a226c0a79b20114721b1ebca

Here's the patch (in case GitHub expires that commit):

From 2d27d0de01ed80c9a226c0a79b20114721b1ebca Mon Sep 17 00:00:00 2001
From: Ben Boeckel <ben.boeckel@kitware.com>
Date: Mon, 14 Oct 2019 14:51:45 -0400
Subject: [PATCH] liblas: reference liblas by its actual ID

References between libraries should be consistent. Without looking at
the filesystem to see that `liblas.3.dylib` and `liblas.2.4.0.dylib` are
actually the same file (via symlinks), realizing that these are the same
library with analysis tools can cause false positives.
---
 gis/liblas/Portfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gis/liblas/Portfile b/gis/liblas/Portfile
index 4a595c79808..69a11eb44ca 100644
--- a/gis/liblas/Portfile
+++ b/gis/liblas/Portfile
@@ -51,5 +51,5 @@ depends_lib-append  port:boost \
 post-destroot {
     exec install_name_tool -id ${prefix}/lib/liblas.2.4.0.dylib ${destroot}${prefix}/lib/liblas.2.4.0.dylib
     exec install_name_tool -id ${prefix}/lib/liblas_c.2.4.0.dylib ${destroot}${prefix}/lib/liblas_c.2.4.0.dylib
-    exec install_name_tool -change "@rpath/liblas.3.dylib" ${prefix}/lib/liblas.3.dylib ${destroot}${prefix}/lib/liblas_c.2.4.0.dylib
+    exec install_name_tool -change "@rpath/liblas.3.dylib" ${prefix}/lib/liblas.2.4.0.dylib ${destroot}${prefix}/lib/liblas_c.2.4.0.dylib
 }

[mathstuf (Ben Boeckel)]

So I just updated locally and I do see @rpath on my machine. I'll have to dig in to see what changed, but probably won't get a chance until next week.

[ryandesign (Ryan Carsten Schmidt)]

Replying to mathstuf:

Ah, I didn't see that it was already there. Mainly because the install name should be $prefix/lib with this patch, not @rpath as you're seeing.

It seems pretty clear that it's the liblas CMakeLists.txt that's causing this to happen, with this block:

if (APPLE)
  set_target_properties(
    las_c
    PROPERTIES
    INSTALL_NAME_DIR "@rpath" BUILD_WITH_INSTALL_RPATH ON)
  set_target_properties(
    las
    PROPERTIES
    INSTALL_NAME_DIR "@rpath" BUILD_WITH_INSTALL_RPATH ON)
endif()

If we don't want rpaths, and I would argue that we don't, then I would suggest trying to remove that block.

I'm pretty sure I had absolute paths in my library IDs when I tested it locally, but maybe that wasn't true after the rebase.

Per Mojca's comment in the PR, you had not initially increased the revision. Is it possible that when you reinstalled the port you received a binary of the existing version, rather than building your locally changed version?

The main issue with @rpath is that an -Wl,-rpath,$dir needs added to all linking executables in order to find any libraries using @rpath. CMake doesn't do this today (mainly because it doesn't know which linked-to libraries are using @rpath or how deep they expect to be from there). For example, Qt frameworks use @rpath/QtCore.framework/QtCore, so you need to go up 2 directories to get the proper rpath entry for that.

Exactly. That's why the code that you removed from the port was in there -- to remove the use of rpath.

Because the install name of the libraries has changed, all programs/libraries that link with this library must be revbumped to use the new install name.

Technically, no. The library ID is copied at link time into the consuming executable. But I can see the argument for consistency. So the old path is still used to look up the current libraries.

What I am saying is that by making this change to liblas, you have changed how other ports would build if they were to be rebuilt. And that is sufficient reason to force those other ports to be rebuilt by increasing their revision. We want RepeatableBuilds.

However since it appears that reintroducing the use of rpath was not your intent, we should fix it so that rpath is once again not used (which will need another revision increase for liblas) and then revbump the ports that use the library.

The reason this came to my attention is because the manual patching was not correct on a machine I was working on (the symlink name was used in the ID, not the actual library).

I agree, the difference in versioning between the symlink and the library was confusing, but it would not be the first time I've seen a project get their library versioning wildly wrong. I don't know at this point whether that wrongness came from the liblas project or from the portfile additions in this case.

[mathstuf (Ben Boeckel)]

It seems pretty clear that it's the liblas CMakeLists.txt that's causing this to happen, with this block:

Ah, I missed that. We should just patch that block. It's not a decision a project should be making (it's a deployment choice).

Per Mojca's comment in the PR, you had not initially increased the revision. Is it possible that when you reinstalled the port you received a binary of the existing version, rather than building your locally changed version?

Unlikely; I had just manually patched the library at that point to test with the other code that was consuming it.

Unfortunately, liblas is "dead" (PDAL is the replacement now). For example, liblas cannot read LAS file format 1.4; PDAL holds that code. Though looking now, some CVE patches did get in. I'll just submit a patch upstream and see what happens. We should probably grab the CVE patches anyways, so maybe a prod for a 1.8.2 could be made :) .

I agree, the difference in versioning between the symlink and the library was confusing, but it would not be the first time I've seen a project get their library versioning wildly wrong. I don't know at this point whether that wrongness came from the liblas project or from the portfile additions in this case.

The reason I showed up was because the macports fix was incorrect :) . We can just patch the upstream project to not hard-code deployment decisions though.

[mathstuf (Ben Boeckel)]

Patch sent upstream: ​https://github.com/libLAS/libLAS/pull/185

[mathstuf (Ben Boeckel)]

Upstream accepted the kill-the-forced-rpath patch.

[dershow]

I put this in with the existing ticket, but it is possible that it should be a separate ticket. liblas is installed as a dependent of another port (grass7). It seems to install initially(I believe as a binary), but after, whenever I upgrade any other port, I get this:

--->  Scanning binaries for linking errors
--->  Found 11 broken files, matching files to ports     
--->  Found 1 broken port, determining rebuild order
You can always run 'port rev-upgrade' again to fix errors.
The following ports will be rebuilt: liblas @1.8.1.99
Continue? [Y/n]: 

If I then hit "Y" I get this:

--->  Computing dependencies for liblas
--->  Cleaning liblas
--->  Scanning binaries for linking errors
--->  Found 11 broken files, matching files to ports     
--->  Found 1 broken port, determining rebuild order
--->  Rebuilding in order
     liblas @1.8.1.99 
--->  Computing dependencies for liblas
--->  Fetching distfiles for liblas
--->  Verifying checksums for liblas
--->  Extracting liblas
--->  Applying patches to liblas
--->  Configuring liblas
--->  Building liblas
Error: Failed to build liblas: command execution failed
Error: See /opt/local/var/macports/logs/_opt_local_var_macports_sources_rsync.macports.org_release_tarballs_ports_gis_liblas/liblas/main.log for details.
Error: rev-upgrade failed: Error rebuilding liblas
Error: Follow https://guide.macports.org/#project.tickets to report a bug.

This is on 10.15.5. And, I have tried to uninstall liblas and got the same results.

[Veence (Vincent)]

Could you please add the log file? Thanks!

[dershow]

Log I see when attempted the rebuild

[ryandesign (Ryan Carsten Schmidt)]

Replying to dershow:

I put this in with the existing ticket, but it is possible that it should be a separate ticket.

It doesn't appear to have anything to do with this ticket.

[ryandesign (Ryan Carsten Schmidt)]

But that build failure has been reported before in comment:ticket:52377:6 (though again, that was added to a ticket that was about a different build failure).

[ryandesign (Ryan Carsten Schmidt)]

In 708b6295b9fb3abf71687c8815f40c5a45d764e9/macports-ports (master):

liblas: Update to 1.8.1-20200508

Closes: #60366