Conversation
spackbot-app
bot
added
build-systems
core
This comment was marked as off-topic.
This comment was marked as off-topic.
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Preliminary review. Feel free to ignore lower-level criticisms (only 2-3 of the comments are high-level).
johnwparent
force-pushed
the
windows-rpath-support
branch
from
2f7bf60 to
b75ec51
Compare
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Additional questions/requests.
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
This follow up is mostly questions/responses (although depending on the answers, some changes may be useful)
johnwparent
force-pushed
the
windows-rpath-support
branch
from
3a5d002 to
4e1337f
Compare
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
Additional requests, all minor (hopefully - let me know if you disagree).
johnwparent
force-pushed
the
windows-rpath-support
branch
2 times, most recently
from
dbd44a7 to
a21c54e
Compare
Logic misstep fixup
johnwparent
force-pushed
the
windows-rpath-support
branch
from
577694c to
fd6c431
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have a couple of notes for future refactoring but I think this is in a good state to merge as is.
| for path in self.pkg.rpath: | ||
| dependent_libs.extend(list(find_all_shared_libraries(path, recursive=True))) | ||
| for extra_path in self._addl_rpaths: | ||
| dependent_libs.extend(list(find_all_shared_libraries(extra_path, recursive=True))) |
There was a problem hiding this comment.
[4] Ultimately I think it will be most idiomatic to iterate through dependencies (transitively) and ask for .libs: as it is, this essentially replicates that but excludes special cases (e.g. a specific package might know where to find libraries and def libs accordingly, but this would miss that). That can be refactored later.
| """ | ||
|
|
||
| dependent_libs = [] | ||
| for path in self.pkg.rpath: |
There was a problem hiding this comment.
We spoke about this needing to be transitive at https://github.com/spack/spack/pull/31930/files#r957678049 - I should correct myself and say that Package.rpath only includes direct dependencies (not that this needs to change now given that this would likely change as part of [4]).
|
@spackbot run pipeline |
|
I've started that pipeline for you! |
| if is_windows: | ||
| static = "lib" | ||
| shared = "dll" | ||
| else: | ||
| # Used on both Linux and macOS | ||
| static = "a" | ||
| shared = "so" |
There was a problem hiding this comment.
@mwkrentel @scheibelp @johnwparent That's the error. Here the shared argument is overridden, so we'll always hit the True condition in the conditional below.
There was a problem hiding this comment.
Oh just found my way here looking into the armadillo build failure in the tutorial stack from today. It can't find libraries in the relocated superlu binary. I guess this is the source of the problem?
There was a problem hiding this comment.
@zackgalbreath I think this ☝️ could the problem we've been looking into.
There was a problem hiding this comment.
No I didn't work on a solution, just getting to the point of figuring out the cause.
4 participants
This PR vendors initial pass Windows rpath analog support. Windows does not understand the concept of an RPATH, so Spack must implement similar functionality and perform such actions manually. This is accomplished via symlinking shared libraries, later to be replaced with a more sophisticated method. This PR is intended as a temporary stop gap support for the concept of RPATHs on Windows.
PR is currently in progress and pending review.