fix bad rpath in chapel-py when using --prefix installs #26388
Conversation
arezaii
force-pushed
the
fix-chapel-py-lib-rpath
branch
from
bcedc08 to
1514187
Compare
Signed-off-by: Ahmad Rezaii <ahmad.rezaii@hpe.com>
arezaii
force-pushed
the
fix-chapel-py-lib-rpath
branch
from
1514187 to
e9eb904
Compare
| if chpl_install_lib_path is not None: | ||
| LDFLAGS += [ | ||
| "-L{}".format(chpl_install_lib_path), | ||
| "-Wl,-rpath,{}".format(chpl_install_lib_path), | ||
| ] | ||
|
|
There was a problem hiding this comment.
Does this not mean that we are now embedding 2 rpaths? one for the source directory and one for the installed directory?
There was a problem hiding this comment.
yes, and I believe even more are added
There was a problem hiding this comment.
Should it be that way? We are embedding rpaths that we never intend to use, which seems like an issue to me
There was a problem hiding this comment.
The python build system seems to need the rpath during the build, and we need a different one later after install, so unless we go back to using chrpath or other post build binary manipulation, I think we're stuck with it :(
There was a problem hiding this comment.
Ugh yeah its required to generate the pyi files. Ok well thats very dissatisfying and is going to be a pain elsewhere (fedora namely, although the chrpath stuff should continue to work around this). But its fine for this PR
There was a problem hiding this comment.
The Linux loader should ignore extraneous RPATH entries, but some of the Linux (namely Fedora) packaging stuff does complain if you have unused/dead/broken RPATH entries.
There was a problem hiding this comment.
Well chrpath should always be safe to remove unused RPATH entries at install time, it's just fragile to assume that it can always successfully replace/insert arbitrary entries.
There was a problem hiding this comment.
Yes, that is what I said here. I am ok with these dead paths, since chrpath is already being used to rewrite RPATHs in the package builds.
There was a problem hiding this comment.
some of the Linux (namely Fedora) packaging stuff does complain if you have unused/dead/broken RPATH entries
I ran into this in the spack builds (on some linux) until I fixed the formatting of the previously existing LDFLAGS (per the diff you slacked me at one point). Just curious if that might be the same symptom or something else.
There was a problem hiding this comment.
Possibly related, although I have had issues with other binaries besides just chapel-py
| chpl_install_lib_path = None | ||
| if os.path.exists(os.path.join(chpl_home, "configured-prefix")): | ||
| with open(os.path.join(chpl_home, "CMakeLists.txt"), "r") as f: | ||
| # read CMakeLists.txt to get the CHPL_MAJOR_VERSION and | ||
| # CHPL_MINOR_VERSION and then construct the path from that | ||
| chpl_major_version = None | ||
| chpl_minor_version = None | ||
| for line in f: | ||
| if "set(CHPL_MAJOR_VERSION" in line: | ||
| chpl_major_version = line.split()[1].strip(")") | ||
| if "set(CHPL_MINOR_VERSION" in line: | ||
| chpl_minor_version = line.split()[1].strip(")") | ||
| if ( | ||
| chpl_major_version is not None | ||
| and chpl_minor_version is not None | ||
| ): | ||
| break | ||
| assert chpl_major_version is not None and chpl_minor_version is not None | ||
| chpl_version_string = "{}.{}".format( | ||
| chpl_major_version, | ||
| chpl_minor_version, | ||
| ) | ||
| chpl_prefix = None | ||
| with open(os.path.join(chpl_home, "configured-prefix"), "r") as f: | ||
| chpl_prefix = f.read().strip() | ||
| assert chpl_prefix is not None | ||
| chpl_install_lib_path = os.path.join( | ||
| chpl_prefix, | ||
| "lib", | ||
| "chapel", | ||
| chpl_version_string, | ||
| "compiler", | ||
| ) |
There was a problem hiding this comment.
In general, I don't think this logic belongs here. I think it should be in something like util/chplenv/chpl_home_utils.py, and can be accessed as python util/chplenv/chpl_home_utils.py --configured-install-prefix
That said, the logic looks fine to me
| if chpl_install_lib_path is not None: | ||
| LDFLAGS += [ | ||
| "-L{}".format(chpl_install_lib_path), | ||
| "-Wl,-rpath,{}".format(chpl_install_lib_path), | ||
| ] | ||
|
|
There was a problem hiding this comment.
The Linux loader should ignore RPATH directories that are inaccessible (or don't contain a matching *.so), so extraneous rpath entries should be "mostly harmless".
On the other hand, relying on chrpath at install time is a fragile design in general, because usually chrpath can only replace an RPATH with one of equal or shorter length. So installing linked executables to a sufficiently long install prefix might "just break" if that insertion is delayed until a post-link chrpath step.
This fixes an issue where installing Chapel using
--prefixwould not encode the correct rpath in the shared library that was created for python. The change here looks for the presence of aconfigured-prefixfile in the build tree and uses that and the chapel version fromCMakeLists.txtto encode the proper rpath based on the installation destination.