nim-lang / choosenim

Official tool for easily installing and managing multiple versions of the Nim programming language.
BSD 3-Clause "New" or "Revised" License
36 stars 8 forks source link

fix: use execv in proxyexe #38

Open SpotlightKid opened 2 months ago

SpotlightKid commented 2 months ago

ref https://forum.nim-lang.org/t/12524

Potential fix for #13 on POSIX and Windows systems.

On non-posix/windows systems still uses the old behaviour using startProcess. Use --undef:useExec to switch back to old behaviour on posix too.

PMunch commented 1 month ago

Great work! You do mention however on the forum that some manual steps are required to replace the proxy executables. Maybe it would be better to add something in updateSelf so that a simple choosenim update self would also rebuild these? Not sure why that's not already done, the way it is now the choosenim version only pertains to the actual choosenim binary and the proxy exes are just whichever version you happened to start your choosenim install on.

SpotlightKid commented 1 month ago

Maybe it would be better to add something in updateSelf so that a simple choosenim update self would also rebuild these?

The proxyexe binary is built into the choosenim executable. It gets written to ~/.nimble/bin/{nim,...}, when choosenim switches to a different Nim channel/version.

So actually, contrary to what I wrote on the forum, currently it isn't necessary to remove the old proxyexe binaries, just to switch to a new (or the same) version of Nim with choosenim after updating choosenim. See the code here:

https://github.com/nim-lang/choosenim/blob/fc240acbcfaff5fe8f3eda4bcf5fbafe84a86add/src/choosenimpkg/switcher.nim#L270

(areProxiesInstalled also checks whether the proxyexe file is up-to-date.)

Updating the proxyexe binaries when choosenim updates itself, can be easily done (see below), but belongs in a different PR, imho, and also doesn't help when somebody just updates choosenim using nimble install choosenim or via a Linux distro package.

diff --git a/src/choosenim.nim b/src/choosenim.nim
index 3306e01..c1b628a 100644
--- a/src/choosenim.nim
+++ b/src/choosenim.nim
@@ -167,6 +167,27 @@ proc updateSelf(params: CliParams) =
   display("Info:", "Updated choosenim to version " & $version,
           Success, HighPriority)

+  # Any Nim installation currently activated by choosenim?
+  if $getCurrentVersion(params) != "":
+    var proxiesToInstall = @proxies
+
+    # Handle MingW proxies.
+    when defined(windows):
+      if not isDefaultCCInPath(params):
+        let mingwBin = getMingwBin(params)
+        if not fileExists(mingwBin / "gcc".addFileExt(ExeExt)):
+          let msg = "No 'gcc' binary found in '$1'." % mingwBin
+          raise newException(ChooseNimError, msg)
+
+        proxiesToInstall.add(mingwProxies)
+
+    if not params.areProxiesInstalled(proxiesToInstall):
+      # Create the proxy executables.
+      for proxy in proxiesToInstall:
+        writeProxy(proxy, params)
+
+
+
 proc update(params: CliParams) =
   if params.commands.len != 2:
     raise newException(ChooseNimError,
diff --git a/src/choosenimpkg/switcher.nim b/src/choosenimpkg/switcher.nim
index 2965859..c195e13 100644
--- a/src/choosenimpkg/switcher.nim
+++ b/src/choosenimpkg/switcher.nim
@@ -107,7 +107,7 @@ proc getSelectedPath*(params: CliParams): string =
 proc getProxyPath(params: CliParams, bin: string): string =
   return params.getBinDir() / bin.addFileExt(ExeExt)

-proc areProxiesInstalled(params: CliParams, proxies: openarray[string]): bool =
+proc areProxiesInstalled*(params: CliParams, proxies: openarray[string]): bool =
   result = true
   let proxyExe = proxyToUse()
   for proxy in proxies:
@@ -173,7 +173,7 @@ proc getNimbleVersion(toolchainPath: string): Version =
     display("Warning:", "Could not find toolchain's Nimble version.",
             Warning, MediumPriority)

-proc writeProxy(bin: string, params: CliParams) =
+proc writeProxy*(bin: string, params: CliParams) =
   # Create the ~/.nimble/bin dir in case it doesn't exist.
   createDir(params.getBinDir())
SpotlightKid commented 1 month ago

Now that there is an implementation of this PR for Windows as well, I will try to add sensible tests. Not sure how yet, but I will come up with something.