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

fix: use execv in proxyexe #38

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SpotlightKid
Copy link

@SpotlightKid SpotlightKid commented Sep 22, 2024

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
Copy link
Collaborator

PMunch commented Sep 24, 2024

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
Copy link
Author

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:

let proxiesInstalled = params.areProxiesInstalled(proxiesToInstall)

(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())
 

Signed-off-by: Christopher Arndt <chris@chrisarndt.de>
Signed-off-by: Christopher Arndt <chris@chrisarndt.de>
@SpotlightKid SpotlightKid changed the title fix: use execv in proxyexe on posix fix: use execv in proxyexe Sep 26, 2024
@SpotlightKid
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants