Fix issues for LD_LIBRARY_PATH via EESSI module#114
Open
ocaisa wants to merge 9 commits intoEESSI:mainfrom
Open
Fix issues for LD_LIBRARY_PATH via EESSI module#114ocaisa wants to merge 9 commits intoEESSI:mainfrom
LD_LIBRARY_PATH via EESSI module#114ocaisa wants to merge 9 commits intoEESSI:mainfrom
Conversation
Member
Author
|
bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws for:arch=x86_64/amd/zen2 |
|
New job on instance
|
|
New job on instance
|
Member
Author
|
bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws for:arch=x86_64/amd/zen2 |
|
New job on instance
|
|
New job on instance
|
ocaisa
changed the title
LD_LIBRARY_PATH via EESSI moduleLD_LIBRARY_PATH via EESSI module
Member
Author
|
bot: build repo:eessi.io-2023.06-software instance:eessi-bot-mc-aws for:arch=x86_64/amd/zen2 |
|
New job on instance
|
|
New job on instance
|
casparvl
reviewed
Oct 28, 2025
Comment on lines
+218
to
+224
| -- remove any standard paths that can interfere with the compat layer | ||
| remove_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", "/lib") | ||
| remove_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", "/lib64") | ||
| remove_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", "/usr/lib") | ||
| remove_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", "/usr/lib64") | ||
| remove_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", "/usr/local/lib") | ||
| remove_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", "/usr/local/lib64") |
Contributor
There was a problem hiding this comment.
I'm confused, didn't you intend to remove from LD_LIBRARY_PATH here? I.e. now, you are essentially leaving LD_LIBRARY_PATH unsanitized, while you're cleaning EESSI_DEFAULT_HOST_LD_LIBRARY_PATH (which I think you meant to keep as a the original value so you can restore later)
Contributor
There was a problem hiding this comment.
I mean, I see that your test passes, I just don't understand how... I don't think it should pass with the current code...
Member
Author
There was a problem hiding this comment.
We make a copy of LD_LIBRARY_PATH, clean it up, and then use push_env to update LD_LIBRARY_PATH. This means after loading the module the LD_LIBRARY_PATH has the sanitised value. On unload, the original value get's restored
casparvl
reviewed
Oct 28, 2025
|
|
||
| -- Filter system paths from LD_LIBRARY_PATH | ||
| -- Needs to be reversible so first make a copy | ||
| append_path ("EESSI_DEFAULT_HOST_LD_LIBRARY_PATH", os.getenv("LD_LIBRARY_PATH") or "") |
Contributor
There was a problem hiding this comment.
Should this be an append_path? Should't it just overwrite? What if at the start EESSI_DEFAULT_HOST_LD_LIBRARY_PATH=/foo and LD_LIBRARY_PATH=/bar. I think what you want is that after unloading LD_LIBRARY_PATH=/bar, but with the current setup, it would be LD_LIBRARY_PATH=/foo:/bar no?
Member
Author
There was a problem hiding this comment.
In the Lmod documentation it explicitly says to use append_path when initialising a path-like variable (which we are doing here), and indeed if you don't it doesn't work properly (I tried).
I think you are not quite following what is happening here:
- We make a copy of the initial value of the original
LD_LIBRARY_PATH - We sanitize the copy
- We use
pushenvto setLD_LIBRARY_PATHto the sanitized value. - Using
pushenvmeans the approach is reversible. If we had worked directly onLD_LIBRARY_PATHunloading would not restore the original environment (the path would remain sanitized)
Member
Author
There was a problem hiding this comment.
Would it help to do more extensive commenting?
Member
Author
There was a problem hiding this comment.
I see your point about EESSI_DEFAULT_HOST_LD_LIBRARY_PATH, but it is not expected that it exists. Maybe the right way to imply that is by naming it_EESSI_DEFAULT_HOST_LD_LIBRARY_PATH_ and verifying it is not set (os.getenv(_EESSI_DEFAULT_HOST_LD_LIBRARY_PATH_)should be nil)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Compat layer binaries are not rpathed, so if host locations like
/usr/libare inLD_LIBRARY_PATHthings will break. Fix this in theEESSImodule.See https://gitlab.com/eessi/support/-/issues/198 for a lot of context.