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

Roll back forceful assignment of PATH when invoking a local process #708

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

motus
Copy link
Member

@motus motus commented Mar 8, 2024

Roll back one of the updates from #705
It breaks tests on both Windows and WSL DevContainer on my PC.

  • On Windows, environ may not have PATH at all
  • On my WSL DevContainer, passing non-null env to subprocess.run prevents it from finding the right python interpreter

@motus motus added bug Something isn't working ready for review Ready for review labels Mar 8, 2024
@motus motus self-assigned this Mar 8, 2024
@motus motus requested a review from a team as a code owner March 8, 2024 01:03
@motus motus mentioned this pull request Mar 11, 2024
4 tasks
Copy link
Contributor

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

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

Approving to unblock, but I think we'll need to revisit (see comments in the PR).

@motus motus enabled auto-merge (squash) March 13, 2024 21:05
@motus motus merged commit d7c4334 into microsoft:main Mar 13, 2024
12 checks passed
@bpkroth
Copy link
Contributor

bpkroth commented Mar 14, 2024

On Windows, environ may not have PATH at all

That seems like a bug. PATH is used to resolve command locations on both platforms.

On my WSL DevContainer, passing non-null env to subprocess.run prevents it from finding the right python interpreter

Oh that's interesting. That's exactly the situation I was trying to fix, but in the opposite direction. Do you have an example?

@motus motus deleted the sergiym/local_exec/env branch March 15, 2024 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants