-
Notifications
You must be signed in to change notification settings - Fork 368
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
Environments for PowerShell and Windows command line #4816
Conversation
Question: Somewhat related to this PR ... OPAM_SWITCH_PREFIX='C:\Users\beckf\AppData\Local\.opam\diskuv-boot-DO-NOT-DELETE'; export OPAM_SWITCH_PREFIX;
MANPATH=';C:\Users\beckf\AppData\Local\.opam\diskuv-boot-DO-NOT-DELETE\man'; export MANPATH;
PATH='C:\Users\beckf\AppData\Local\.opam\diskuv-boot-DO-NOT-DELETE\bin;Z:\ProgramFiles\VMware\VMware Player\bin\;...;Z:\ProgramFiles\CMake\bin'; export PATH; However, these Windows paths can only work in native Windows (ex. PowerShell) but not in MSYS2. Among other things that means Should |
Indeed, the check here is not quite correct: even on Windows, opam needs to be looking at its environment to work out the shell. To remain compatible with running native opam inside MSYS2, we should definitely inspect the diff --git a/src/core/opamStubs.dummy.ml b/src/core/opamStubs.dummy.ml
index a7afdee28..6dbb477c5 100644
--- a/src/core/opamStubs.dummy.ml
+++ b/src/core/opamStubs.dummy.ml
@@ -34,5 +34,6 @@ let process_putenv _ = that's_a_no_no
let shGetFolderPath _ = that's_a_no_no
let sendMessageTimeout _ _ _ _ _ = that's_a_no_no
let getParentProcessID = that's_a_no_no
+let getParentProcessName = that's_a_no_no
let getConsoleAlias _ = that's_a_no_no
let win_create_process _ _ _ _ _ = that's_a_no_no
diff --git a/src/core/opamStubs.mli b/src/core/opamStubs.mli
index cbbcd67a5..891264164 100644
--- a/src/core/opamStubs.mli
+++ b/src/core/opamStubs.mli
@@ -124,6 +124,12 @@ val getParentProcessID : int32 -> int32
@raise Failure If walking the process tree fails to find the process. *)
+val getParentProcessName : int32 -> string
+(** Windows only. [getParentProcessName pid] returns the executable name of the
+ parent of [pid].
+
+ @raise Failure If walking the process tree fails to find the process. *)
+
val getConsoleAlias : string -> string -> string
(** Windows only. [getConsoleAlias alias exeName] retrieves the value for a
given executable or [""] if the alias is not defined. See
diff --git a/src/stubs/win32/opamWin32Stubs.ml b/src/stubs/win32/opamWin32Stubs.ml
index 9fe73f738..fdd1e32f3 100644
--- a/src/stubs/win32/opamWin32Stubs.ml
+++ b/src/stubs/win32/opamWin32Stubs.ml
@@ -33,4 +33,5 @@ external process_putenv : int32 -> string -> string -> bool = "OPAMW_process_put
external shGetFolderPath : int -> 'a -> string = "OPAMW_SHGetFolderPath"
external sendMessageTimeout : nativeint -> int -> int -> 'a -> 'b -> 'c -> int * 'd = "OPAMW_SendMessageTimeout_byte" "OPAMW_SendMessageTimeout"
external getParentProcessID : int32 -> int32 = "OPAMW_GetParentProcessID"
+external getParentProcessName : int32 -> string = "OPAMW_GetParentProcessName"
external getConsoleAlias : string -> string -> string = "OPAMW_GetConsoleAlias"
diff --git a/src/stubs/win32/opamWindows.c b/src/stubs/win32/opamWindows.c
index 8e357dad1..39b82c6a8 100644
--- a/src/stubs/win32/opamWindows.c
+++ b/src/stubs/win32/opamWindows.c
@@ -726,6 +726,26 @@ CAMLprim value OPAMW_GetParentProcessID(value processId)
OPAMreturn(caml_copy_int32(entry.th32ParentProcessID));
}
+CAMLprim value OPAMW_GetParentProcessName(value processId)
+{
+ CAMLparam1(processId);
+
+ PROCESSENTRY32 entry;
+ DWORD parent_pid;
+ char* msg;
+ HANDLE hProcessSnapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0);
+
+ if ((msg = getProcessInfo(hProcessSnapshot, Int32_val(processId), &entry)))
+ caml_failwith(msg);
+
+ if ((msg = getProcessInfo(hProcessSnapshot, entry.th32ParentProcessID, &entry)))
+ caml_failwith(msg);
+
+ CloseHandle(hProcessSnapshot);
+
+ CAMLreturn(caml_copy_string(entry.szExeFile));
+}
+
CAMLprim value OPAMW_GetConsoleAlias(value alias, value exeName)
{
CAMLparam2(alias, exeName); The story - as with everything on Windows - is complicated (!!). In principle, one should be using an opam matching the compiler you're working with. So with a native Windows opam, you should be using that in native Windows to create native Windows switches. From that, for example, one shouldn't expect to use Ubuntu's |
1ed2674
to
732de68
Compare
Okay, I overhauled the PR. What is missing is the contents of the environment (ex. PATH should be cygpath'd within Cygwin and MSYS2), but I think that belongs in a separate PR. It prints the following ... MSYS2 and Cygwin
/z/source/opam$ env _build/default/src/client/opamMain.exe env --root $LOCALAPPDATA/.opam
OPAM_SWITCH_PREFIX='Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam'; export OPAM_SWITCH_PREFIX;
OCAMLLIB='Z:\source\diskuv-ocaml-starter\build\dev\Debug\_opam/lib/ocaml'; export OCAMLLIB;
CC='cl.exe'; export CC;
... Command Prompt
PowerShell
|
I can't push on your repo @jonahbeckford . I pushed in rjbou/feature-eval-on-pwsh, if you can port last commits, and feel free to squash commits (there is one that is just style related) |
902e802
to
c5de584
Compare
Thanks @rjbou ; I merged, squashed and pushed your changes. (And my future opam PRs will be pushable) |
c5de584
to
cacf650
Compare
cacf650
to
1e07ed5
Compare
Rebased. Can see CI problems with |
Rebased and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry this has taken me such a ridiculously long time to review, but thank you very much for this! This goes very nicely with one of my oldest still-to-be-merged commits for opam (from 2015!) for the Command Prompt but I hadn't tackled PowerShell at all, and several parts of your cmd implementation here are really very nice (I particularly like having a clear - if, of course, arcane - equivalent for eval $(opam env)
).
I have a few questions for small changes, but I propose otherwise that we get this merged and then I'll get on with pulling in the rest of my opam init
stuff (which covers the shell script generation, and so forth).
| SH_pwsh | SH_win_powershell -> | ||
if Sys.win32 then win_my_powershell "Microsoft.Powershell_profile.ps1" else | ||
List.fold_left Filename.concat (home ".config") ["powershell"; "Microsoft.Powershell_profile.ps1"] | ||
| SH_sh -> home ".profile" | ||
| SH_win_cmd -> | ||
(* cmd.exe does not have a concept of profiles *) | ||
home ".profile" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some related work on this which I propose coming to also in a subsequent PR - IIUC Diskuv doesn't use opam init
to do this at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the question whether Diskuv uses opam init
to populate entries (PATH, auto-complete) in the user's profile? No, Diskuv does not use opam init
for that.
The installer takes care of putting Opam and with-dkml.exe (the wrapper) and a system dune + utop in the PATH via the registry in the source code https://github.com/diskuv/dkml-component-ocamlcompiler/blob/1e1990447a2fafdca0b2a18e845821952dc0cce2/assets/staging-files/win32/setup-userprofile.ps1#L1538-L1541 .
The uninstaller (on my TODO) will do the reverse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent - the main thing I wanted to be sure was that me messing around with this in a subsequent PR wouldn't risk breaking things for you! I've got a patch which takes advantage of clink (if the user has it installed) to do the ".profile" trick for cmd, as it provides start-up scripts in Lua.
Thanks for the updates! There's the CLI versioning question above and also the question as to whether the guessing of the shell for |
fdd8172
to
2a9cc2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to go, thank you!
It's virtually impossible to define a general escaping function for the Command Prompt, because the expansion is context-specific. However, as set is the _only_ command, it's possible to use its double-quote escaped form which then requires no further escaping of the strings.
2a9cc2a
to
49a7450
Compare
Splitting up #4813:
For continuity this initial PR is as-is from the original PR.