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.

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

Fixed LD_LIBRARY_PATH squashing #867

Merged
merged 4 commits into from
Oct 4, 2024
Merged

Conversation

sternj
Copy link
Collaborator

@sternj sternj commented Oct 4, 2024

Because of how the current environment is checked, LD_LIBRARY_PATH and LD_PRELOAD are squashed if present, rather than prepended to. This commit prevents this.

Also preemptively fixed a potential issue where PYTHONMALLOC would not be properly squashed if present.

@sternj
Copy link
Collaborator Author

sternj commented Oct 4, 2024

I think that what we'll need to do (rather soon) is escape all of the unescaped spaces in environment variables in redirect_python, if there are spaces in any environment variable that we don't mess with then they'll cause issues if the wrapper script is ever invoked.

@emeryberger
Copy link
Member

Any reason not to do this in the current PR?

@sternj
Copy link
Collaborator Author

sternj commented Oct 4, 2024

It's potentially a more far-reaching change, I can't think of any unintended effects but they may exist, especially given that it's happening at the invocation site for any subprocesses. What I have here is the minimal change to fix the issue at hand, but I'll see what the test suite says about the further-reaching solution.

@sternj
Copy link
Collaborator Author

sternj commented Oct 4, 2024

Note that this doesn't help with Windows environment variables, which I do not know the semantics of

@emeryberger
Copy link
Member

Scalene doesn't make specific use environment variables in Windows, should be fine, but should also be tested. Can we get Windows testing into the CI?

@emeryberger emeryberger merged commit 856ba1a into master Oct 4, 2024
22 checks passed
@sternj
Copy link
Collaborator Author

sternj commented Oct 4, 2024

I don't see why it isn't possible! I'll add it to the list.

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.

2 participants