Fix shell detection on Windows when opam is called via Cygwin's /usr/bin/env even though cmd/powershell is used #5797
Conversation
There was a problem hiding this comment.
This line was there for Diskuv (I don't know why Diskuv invokes opam using MSYS2's env, rather than via Unix.create_process_env et al, but I expect there's a reason @jonahbeckford?)
The idea behind the original line was that if we see "env" in the process chain, opam assumes that its in Unix-land, and guesses sh. That said, it turns out the only situation where this really matters is when env is used from a non-login terminal. In a login shell, Cygwin exports SHELL by default, so with no special handling for env (i.e. your change here), it still correctly identifies the shell. So with this PR, the time that it would be "wrong" is these steps:
- From Powershell, or Cmd, run MSYS2/Cygwin's bash without
--login(e.g.C:\cygwin64\bin\bash) echo $SHELLshould give/bin/bash, butcmd /c echo %SHELL%should give%SHELL%(from a login shell, you'd get/bin/bashfor both)opam envshould give sh-output/usr/bin/env opam envwill now give cmd-style outputexport SHELL/usr/bin/env opam envwill now give sh-style output
Given that this line has caused the wrong shell for actual users (and while using the system for which the line was originally there anyway) coupled with what I think is a fairly contrived failure mode, I think we should delete the line 🙂
I think because |
Does this mean upgrading to cmdliner 1.2.0 together with this PR in the current state would fix both issues? If so I'll upgrade the lower-bounds and vendor of cmdliner in this PR. |
|
There were at least two separate problems and only one has been fixed. (That linked issue expects "less" and "groff" to be in the PATH, which it is not in native Windows. That might be a third and fourth problem.)
Sorry but I didn't do a full investigation to discover the extent of the problems with the pager support on Windows, and I didn't file issues, and do all the other time-consuming steps needed for a full solution. I just know that using "env" as a hack is sufficient for a working pager, and that cmdliner.1.2.0 (which is used in DkML) is insufficient.
Vendoring to 1.2.0 won't solve the full problem but is better than nothing!
…________________________________
From: Kate ***@***.***>
Sent: Wednesday, January 31, 2024 2:04:34 AM
To: ocaml/opam ***@***.***>
Cc: Jonah Beckford ***@***.***>; Mention ***@***.***>
Subject: Re: [ocaml/opam] Fix shell detection on Windows when opam is called via Cygwin's /usr/bin/env even though cmd/powershell is used (PR #5797)
I think because opam --help does not work on native Windows (no PAGER environment variable if I remember correctly; there may be more). I hate that hack, but --help was kinda important. And I also think that hack predated the dbuenzli/cmdliner#166<dbuenzli/cmdliner#166> hang issue.
Does this mean upgrading to cmdliner 1.2.0 should fix together with this PR in the current state would fix both issues?
If so I'll upgrade the lower-bounds and vendor of cmdliner in this PR.
—
Reply to this email directly, view it on GitHub<#5797 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AREG4PJ5MAJZPCKVWGRCXULYRIJLFAVCNFSM6AAAAABCGGMKOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJYG43TSMZTGA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
cc @dbuenzli |
|
I don't think it does. If it can't find a pager it outputs a plain text version (which does not need groff). |
kit-ty-kate
force-pushed
the
fix-win-terminal-detect
branch
from
2bf268f
to
034c97f
Compare
|
I've updated the vendored cmdliner but in any case, with or without, I was unable to reproduce your issue @jonahbeckford. |
|
It works with a pager? How did a pager get into your path?
(Normal users should not have the “/usr/bin/less” pager in their PATH, nor “groff”, and it would be scrolling 10 pages of help plaintext all at once.)
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Kate ***@***.***>
Sent: Monday, February 5, 2024 1:14:11 PM
To: ocaml/opam ***@***.***>
Cc: Jonah Beckford ***@***.***>; Mention ***@***.***>
Subject: Re: [ocaml/opam] Fix shell detection on Windows when opam is called via Cygwin's /usr/bin/env even though cmd/powershell is used (PR #5797)
I've updated the vendored cmdliner but in any case, with or without, I was unable to reproduce your issue @jonahbeckford<https://github.com/jonahbeckford>. --help works just fine for me in both cmd and powershell
—
Reply to this email directly, view it on GitHub<#5797 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AREG4PMW5MQMQUZ2ADSOWJDYSFDSHAVCNFSM6AAAAABCGGMKOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYGEYDINBZGE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
It does not. As far as I can see cmdliner does not support But I don't think it's too much of a problem in the short term. In the medium term however, is there a bug report somewhere on cmdliner for this? |
|
Okay. I don't know of any feature request for pager support on native Windows for cmdliner.
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Kate ***@***.***>
Sent: Monday, February 5, 2024 1:45:16 PM
To: ocaml/opam ***@***.***>
Cc: Jonah Beckford ***@***.***>; Mention ***@***.***>
Subject: Re: [ocaml/opam] Fix shell detection on Windows when opam is called via Cygwin's /usr/bin/env even though cmd/powershell is used (PR #5797)
It works with a pager? How did a pager get into your path? (Normal users should not have the “/usr/bin/less” pager in their PATH, nor “groff”, and it would be scrolling 10 pages of help plaintext all at once.)
It does not. As far as I can see cmdliner does not support PAGER to be a path on Windows as it uses where which doesn't seem to support full-paths (whereas command -v does)
But I don't think it's too much of a problem in the short term. Is there a bug report somewhere on cmdliner for this?
—
Reply to this email directly, view it on GitHub<#5797 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AREG4PI7JD34CN5IYQ2A663YSFHGZAVCNFSM6AAAAABCGGMKOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRYGE2DSNZTGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Then I opened one here: dbuenzli/cmdliner#185 |
|
Shouldn't we update dependencies to cmdliner >= 1.2.0 ? |
I don't personally think it's necessary |
…'s /usr/bin/env even though the shell is cmd/powershell
kit-ty-kate
force-pushed
the
fix-win-terminal-detect
branch
from
034c97f
to
676365d
Compare
Fixes #5644
This line was added in #4816 but looking at the relevant discussion (#4816 (comment)) I’m not sure if the change makes much sense to me (but I’m not a Windows user so most of the discussions around Windows doesn’t make sense to me anyway :D )