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

Eliminate the separate constructors for powershell #5203

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jul 26, 2022

Record whether the shell was detected as powershell or pwsh, although this information isn't actually used at the moment. Decreases the chance of accidentally only matching on one constructor.

@jonahbeckford - I'm not sufficiently familiar with PowerShell: are there situations where we might need to care whether it's pwsh (PowerShell 6.0) or classic PowerShell Desktop/Core (5.x and earlier)? I'm wondering whether we could risk dropping the --shell=powershell option completely?

@dra27 dra27 added this to PR in progress in Opam 2.2.0 via automation Jul 26, 2022
@dra27 dra27 added this to the 2.2.0~alpha milestone Jul 26, 2022
@jonahbeckford
Copy link
Contributor

There is no pwsh for the bundled PowerShell 3.0 and 5.0 in Windows; only powershell is present.

@dra27
Copy link
Member Author

dra27 commented Jul 28, 2022

What I mean is that I don't think opam will ever do anything different based on whether you specify --shell=pwsh or --shell=powershell?

@jonahbeckford
Copy link
Contributor

Oh I see. I thought the code actually invoked powershell.exe or pwsh.exe somewhere, but I'm mistaken. And even if it did, could just probe the PATH to find pwsh.exe or powershell.exe (whichever was found first).

+1 LTGM

@dra27
Copy link
Member Author

dra27 commented Jul 29, 2022

And even if it did, could just probe the PATH to find pwsh.exe or powershell.exe (whichever was found first).

I’ve got some follow-up code for the parent process detection where it gets the full path of the parent process instead of just the name (I wanted this to be able to reliably ask PowerShell where profile files should go) so we should be able to have both the shell and the exact path to it. I’m hoping this may also help with disambiguating running under Cygwin’s PATH and being able to avoid WSL’s bash in the Windows system32, too

+1 LTGM

Thanks!

Record whether the shell was detected as powershell or pwsh, although
this information isn't actually used at the moment.
@kit-ty-kate
Copy link
Member

Thanks!

@kit-ty-kate kit-ty-kate merged commit cf93e24 into ocaml:master Jul 29, 2022
Opam 2.2.0 automation moved this from PR in progress to Done Jul 29, 2022
Leonidas-from-XIV pushed a commit to Leonidas-from-XIV/opam that referenced this pull request Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Opam 2.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants