Skip to content

fix bad rpath in chapel-py when using --prefix installs#26388

Merged
arezaii merged 3 commits intochapel-lang:mainfrom
arezaii:fix-chapel-py-lib-rpath
Feb 4, 2025
Merged

fix bad rpath in chapel-py when using --prefix installs#26388
arezaii merged 3 commits intochapel-lang:mainfrom
arezaii:fix-chapel-py-lib-rpath

Conversation

@arezaii
Copy link
Contributor

@arezaii arezaii commented Dec 10, 2024

This fixes an issue where installing Chapel using --prefix would not encode the correct rpath in the shared library that was created for python. The change here looks for the presence of a configured-prefix file in the build tree and uses that and the chapel version from CMakeLists.txt to encode the proper rpath based on the installation destination.

[reviewed by @jabraham17 and @bonachea - thanks!]

@arezaii arezaii requested a review from jabraham17 December 10, 2024 20:10
@arezaii arezaii marked this pull request as ready for review December 10, 2024 20:10
@arezaii arezaii force-pushed the fix-chapel-py-lib-rpath branch 2 times, most recently from 1514187 to e9eb904 Compare December 10, 2024 20:36
Comment on lines +106 to +89
if chpl_install_lib_path is not None:
LDFLAGS += [
"-L{}".format(chpl_install_lib_path),
"-Wl,-rpath,{}".format(chpl_install_lib_path),
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not mean that we are now embedding 2 rpaths? one for the source directory and one for the installed directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and I believe even more are added

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be that way? We are embedding rpaths that we never intend to use, which seems like an issue to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly related, although I have had issues with other binaries besides just chapel-py

Comment on lines +106 to +89
if chpl_install_lib_path is not None:
LDFLAGS += [
"-L{}".format(chpl_install_lib_path),
"-Wl,-rpath,{}".format(chpl_install_lib_path),
]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bonachea bonachea mentioned this pull request Jan 15, 2025
@arezaii arezaii force-pushed the fix-chapel-py-lib-rpath branch from eebf11a to fe6cf54 Compare January 28, 2025 23:01
Signed-off-by: Ahmad Rezaii <[email protected]>
@arezaii arezaii force-pushed the fix-chapel-py-lib-rpath branch from c2238f6 to 061f944 Compare January 29, 2025 17:53
Copy link
Contributor

@bonachea bonachea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arezaii is this ready to be merged? I'd really like to see this in the upcoming Chapel release to simplify our spackaging.

@arezaii arezaii merged commit c944ff9 into chapel-lang:main Feb 4, 2025
9 checks passed
@bradcray
Copy link
Member

bradcray commented Feb 4, 2025

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants