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

this fixes subprogs missing keys #1086

Merged
merged 2 commits into from
Jan 17, 2025
Merged

this fixes subprogs missing keys #1086

merged 2 commits into from
Jan 17, 2025

Conversation

jhheider
Copy link
Contributor

closes #1085

@jhheider jhheider requested a review from mxcl January 17, 2025 20:56
@mxcl
Copy link
Member

mxcl commented Jan 17, 2025

Nice, I had a similar patch coming.

@mxcl
Copy link
Member

mxcl commented Jan 17, 2025

Again: unbelievable I didn’t spot this before! How on earth so many things I tested worked fine I have no idea.

@jhheider
Copy link
Contributor Author

i hate the two for-loops, but didn't feel like messing around with maps and zips and so forth.

@jhheider
Copy link
Contributor Author

my guess is because it's a script running under an execve that running in an interpreter... etc. levels on levels.

@mxcl
Copy link
Member

mxcl commented Jan 17, 2025

Hilariously a couple hours ago I introduced a new test that exposes it: https://github.com/pkgxdev/pkgx/actions/runs/12835540088/job/35795376923?pr=1084#step:19:9

@mxcl
Copy link
Member

mxcl commented Jan 17, 2025

i hate the two for-loops, but didn't feel like messing around with maps and zips and so forth.

Seems fine. We're iterating over two different things. Not sure there's a easy way to zip them together and do it clearly.

@jhheider
Copy link
Contributor Author

joyous. i guess the "right" way to do it is to clone std::env::vars(), then loop through input and insert_or_join each key in a single loop.

@jhheider
Copy link
Contributor Author

couldn't live with two loops. clone env, prepend needed keys.

@mxcl
Copy link
Member

mxcl commented Jan 17, 2025

Definitely need some unit tests for these pieces

@jhheider
Copy link
Contributor Author

that'd be an interesting one to spot regardless. needs a smart test. but preventing regressions should definitely be possible. an integration test stands a better chance.

@mxcl mxcl merged commit 31ba978 into main Jan 17, 2025
13 checks passed
@mxcl mxcl deleted the fix-sub-env branch January 17, 2025 21:16
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.

getting libcrypto.so.1.1 ... No such file or directory for pkgx npx ...
2 participants