Conversation
01603a8 to
2b7c15f
Compare
8a1fcbf to
77e7f22
Compare
9cd3f38 to
27b186e
Compare
bd517f1 to
c32b23b
Compare
pkg/dl/dl_darwin.go
Outdated
| // Path is NOT supported on Darwin. | ||
| // TODO: dlinfo is not supported on darwin and dladdr should be used. | ||
| // See for example: https://github.com/Manu343726/siplasplas/issues/82 | ||
| // This requires the resolution of an expected symbol. | ||
| // For now we return an error. | ||
| func (dl *DynamicLibrary) Path() (string, error) { | ||
| if dl.handle == nil { | ||
| return "", fmt.Errorf("%v not opened", dl.Name) | ||
| } | ||
|
|
||
| return "", fmt.Errorf("not implemented") | ||
| } |
There was a problem hiding this comment.
Why do we care about darwin at all?
There was a problem hiding this comment.
I care becase I develop on a mac and having these stubs defined there allows me to run unit tests locally. (Even if we don't expect to trigger the error there).
There was a problem hiding this comment.
This can be made more general though. I think dl_other.go may be better than keeping this Darwin specific.
Signed-off-by: Evan Lezar <elezar@nvidia.com>
8c7a1da to
7882a5b
Compare
pkg/dl/dl_linux.go
Outdated
| resolved, err := filepath.EvalSymlinks(path) | ||
| if err != nil { | ||
| // Ignore errors resolving the symlink.s | ||
| resolved = path | ||
| } |
There was a problem hiding this comment.
Why is it ok to simply ignore the error here?
There was a problem hiding this comment.
I was trying to avoid unknown unknowns here. I have a resonable understanding on how this should behave with the libraries we actually call this with (libnvidia-ml.so.1), but don't know if there's edge cases I'm missing. With that said, maybe it's the wrong thing to do the Symlink evaluation here. Let me rather simplify this so that the resolution of links (and handling the subsequent errors) is left to the caller.
There was a problem hiding this comment.
Given your comment, did you mean to remove the call to filepath.EvalSymlinks(path)?
There was a problem hiding this comment.
Yes, I did. (Let me double-check the diff).
08e2b74 to
bf61cb2
Compare
pkg/dl/dl_linux.go
Outdated
| dl.path = filepath.Join(C.GoString((*C.char)(libParentPathBuffer)), dl.Name) | ||
| libPath = dl.path |
There was a problem hiding this comment.
nit: I would assign the local variable libPath first and then set dl.path second. Non-blocking though,
There was a problem hiding this comment.
Or maybe not, to be consistent with the other assignment.
This change adds a a function to get the path for a DynamicLibrary. This is currently only supported on Linux. Signed-off-by: Evan Lezar <elezar@nvidia.com>
bf61cb2 to
a522afe
Compare
This change adds an
Pathfunction to thedl.DynamicLibrarytype so as to allow for the path of a loaded library to be determined.This is inspired by: https://github.com/NixOS/nixpkgs/blob/953355a9568f324d232bd920ad09791009f2c4c0/pkgs/by-name/nv/nvidia-container-toolkit/0001-Add-dlopen-discoverer.patch#L65
and allows a
dlopen-based library locator to be added to the NVIDIA Container Toolkit: NVIDIA/nvidia-container-toolkit#1646Using the included example:
And also: