Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions tools/chapel-py/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,41 @@

host_bin_subdir = str(chpl_variables.get("CHPL_HOST_BIN_SUBDIR"))
chpl_lib_path = os.path.join(chpl_home, "lib", "compiler", host_bin_subdir)
# For installations using --prefix, the lib final lib path are going to differ
# figure it out now and write it to the rpath
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",
)
Comment on lines +52 to +84
Copy link
Member

Choose a reason for hiding this comment

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

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


CXXFLAGS = []
if have_llvm and have_llvm != "none":
Expand All @@ -68,6 +103,12 @@
"-lChplFrontendShared",
]

if chpl_install_lib_path is not None:
LDFLAGS += [
"-L{}".format(chpl_install_lib_path),
"-Wl,-rpath,{}".format(chpl_install_lib_path),
]

Comment on lines +106 to +111
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
Collaborator 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
Collaborator 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
Collaborator 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

if str(chpl_variables.get("CHPL_SANITIZE")) == "address":
if str(chpl_variables.get("CHPL_HOST_PLATFORM")) == "darwin":
sys.exit(
Expand Down
Loading