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

Add missing env to GetUnixArchitecture #7152

Merged
merged 2 commits into from
May 31, 2024

Conversation

Elias-Graf
Copy link
Contributor

@Elias-Graf Elias-Graf commented May 20, 2024

Add missing environment when acquiring the UNIX architecture. Not providing the environment, breaks systems that don't have uname in the $PATH by default, such as NixOS. Demonstrated by this issue: #5575.

Personally, I would also recommend removing the default value for the env parameter, forcing the programmer to think about their intention, instead of just leaving it out.

env: NodeJS.ProcessEnv = {}

If that is something you are interested in, let me know, and I will adjust the function, and necessary call sites. There don't appear to be too many.

Fixes #5575.

@Elias-Graf Elias-Graf requested a review from a team as a code owner May 20, 2024 16:17
@Elias-Graf Elias-Graf changed the title Add missing env Add missing env to GetUnixArchitecture May 20, 2024
Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay here. Generally I think I'm good with the change. I do worry a little that we might break some edge scenario where an incorrect env var breaks this, but I think the answer there is to fix the env var.

Personally, I would also recommend removing the default value for the env parameter, forcing the programmer to think about their intention, instead of just leaving it out.

Yup, I think that would be a good idea. Would be happy to accept that change. I'm good to merge as-is as well if you'd prefer to not do it now.

@Elias-Graf
Copy link
Contributor Author

@dibarbet Thank you for looking at the pull request.
I have now removed the default value for the env variable. I have provided process.env in all cases, as it seemed appropriate.
Due to the second to last variable also being optional, I had to remove the default value of the workingDirectory as well. I had also considered reordering the parameters, but given that the workingDirectory is always provided anyway, I've decided against it. In addition, the order seems more natural in the current state. Feel free to request further changes.

I do worry a little that we might break some edge scenario where an incorrect env var breaks this, but I think the answer there is to fix the env var.

That is always a risk with changes considering "user input". Although given that not passing along the environment in most cases is probably wrong, and most definitely problematic in the affected cases, it is also not fixable by the user in any obvious way - while a broken environment definitely is.

@dotnet-policy-service agree

@dibarbet
Copy link
Member

Thanks for the contribution!

@dibarbet dibarbet merged commit d610683 into dotnet:main May 31, 2024
11 of 13 checks passed
@Elias-Graf Elias-Graf deleted the fix_get_unix_arch_env branch June 1, 2024 08:52
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.

Extension no longer loads in NixOS (uname in non-standard location)
2 participants