Multiple features #63

genotrance · 2018-05-28T05:11:29Z

Support for installation of binary builds on Windows with detection of gcc arch
Support for 64-bit detection - issue Windows 64 bit builds? #16 and Improve gcc detection logic #27
Download sources and csources from Github if not available combined on nim-lang.org - issue Serve all downloads from GitHub? #42

…om Github, extract zips

timotheecour · 2018-07-03T09:07:39Z

see also #65 (as mentioned by @genotrance)

Araq · 2018-10-09T16:19:13Z

dom96 · 2018-10-13T21:58:48Z

I'm going to remove your comments because they are incredibly distracting to me as I'm reviewing this, sorry. (I backed them up in this screenshot)

genotrance · 2018-10-13T22:20:24Z

No worries.

dom96 · 2018-10-13T22:51:15Z

src/choosenim/download.nim

+
+  downloadFile(url, result, params)
+
+proc downloadCheckRenamed(params: CliParams, url, filename: string): string =


I don't think this code should be in a procedure. Also the downloadCheck proc above should be renamed to something else or restructured. Even something long like downloadIfNecessary would be better, the current name is too vague.

or maybe hashedDownload?

dom96 · 2018-10-13T22:56:29Z

src/choosenim/download.nim

+    except HttpRequestError:
+      # Fallback to downloading from Github
+      # Rename to nim-VERSION.tar.gz to disambiguate
+      result = downloadCheckRenamed(params, githubUrl % ("v" & $version),


Why would GitHub have it, but not nim-lang.org? This seems redundant.

The idea was to fallback to github.com if nim-lang.org was down. It should fall back to tar.gz + csources on Linux as well since github.com won't have an xz file with built-in csources.

nim-lang.org has so far a better uptime than github.com. It's a nice thought but I doubt this complexity is worth it.

dom96 · 2018-10-13T23:03:36Z

src/choosenim/download.nim

+  result = getDownloadDir(params) / filename
+  if not fileExists(result):
+    let origfile = downloadCheck(params, url)
+    moveFile(origfile, result)


After looking at this in more detail this seems unnecessary. The downloadFile proc takes an outputPath, also the params.getDownloadPath used in needsDownload should already use the right path. You shouldn't need to move the file for cases where you're downloading "nim-0.19.0.tar.gz", from what I can tell your code just moves it to the same place...

I'll look into cleaning up the proc names and avoid renaming.

Actually now that I looked at this further, the reason its this way is because needsDownload() sets the outputPath, you cannot pass a value down if you want a rename, it will get overwritten.

There are two reasons to rename:

Line 269 - downloading Nim sources from github - you get a master.tgz, no version in name

Line 285 - downloading csources from github - you again get a master.tgz, no version in name

Okay, what I would do is overload needsDownload:

proc needsDownload(params: CliParams, downloadUrl: string, outputPath: string): bool =

Also I would get rid of these procs, putting two lines into a proc isn't worth it.

dom96 · 2018-10-13T23:03:56Z

src/choosenim/download.nim

-  return outputPath
+  try:
+    # Download csources tagged version from Github
+    # Rename to csources-VERSION.tar.gz to disambiguate


See my remark above, renaming shouldn't be necessary.

dom96 · 2018-10-13T23:08:02Z

src/choosenim/download.nim

+                                  "csources-$1.tar.gz" % $version)
+  except HttpRequestError:
+    result = downloadCheck(params, csourcesUrl % "master")
+    display("Warning:", "Building from latest C sources. They may not be " &


Nitpick:

*Downloading the latest C sources

Did you want to change the message? Csources are already downloaded by now though.

In that case "Downloaded", the point is that this procedure isn't building the C sources. You don't know what context it might be called from, it's better to make the messages reflect what the procedure is doing.

dom96 · 2018-10-13T23:08:34Z

src/choosenim/download.nim

-  if not needsDownload(params, mingwUrl, outputPath):
-    return outputPath
-
  display("Downloading", "C compiler (Mingw32)", priority = HighPriority)


This will be misleading since it will be shown even if the download doesn't happen.

dom96 · 2018-10-13T23:08:42Z

src/choosenim/download.nim

  display("Downloading", "DLLs (openssl, pcre, ...)", priority = HighPriority)
-  downloadFile(dllsUrl, outputPath, params)
-  return outputPath
+  result = downloadCheck(params, dllsUrl)


Same as above.

dom96 · 2018-10-13T23:10:11Z

src/choosenim/utils.nim

+proc extractZip(path: string, extractDir: string) =
+  var cmd = "unzip -o $1 -d $2"
+  if defined(windows):
+      cmd = "powershell -nologo -noprofile -command \"& { Add-Type -A 'System.IO.Compression.FileSystem'; [IO.Compression.ZipFile]::ExtractToDirectory('$1', '$2'); }\""


It's awesome that powershell supports this.

Sadly it will fail on older Windows versions. I test choosenim on Win XP, any chance we could get a good fallback for those?

No good answer for this. Options are to use nimarchive or some other capable extraction lib that supports gz and xz as well. Alternative is to disallow binary install if powershell isn't available.

Alternative is to disallow binary install if powershell isn't available.

I would like this, but I'm not going to block you for it.

dom96 · 2018-10-13T23:12:04Z

src/choosenim.nim

+    # Fix win binary folder to avoid /nim-0.xx.0/nim-0.xx.0
+    let winFolder = extractDir/"nim-$1" % $version
+    if dirExists(winFolder):
+      moveDirContents(winFolder, extractDir)


Can't you just use moveDir here?

moveDir() raises an exception since directory already exists.

Error: unhandled exception: Cannot create a file when that file already exists. Good old Windows.

Why would that directory already exist? Even if it exists, can't we just overwrite it?

dom96 · 2018-10-13T23:18:39Z

src/choosenim/utils.nim

+    errC = 0
+
+  when defined(windows):
+    (outp, errC) = execCmdEx("cmd /c echo int main^(^) { return sizeof^(void *^); } | gcc -xc - -o archtest && archtest")


Is this not POSIX Shell syntax?

This is what was added to build.bat to allow gcc arch detection.

https://github.com/nim-lang/csources/blob/master/build.bat#L14

genotrance · 2019-01-03T05:28:10Z

I'm closing this PR in favor of simpler PRs that can be reviewed and accepted quickly.

fix(switcher.nim): fix CC for FreeBSD

genotrance added 3 commits May 24, 2018 23:03

Support for 64-bit and binary installs on Windows, download tar.gz fr…

6cc2d24

…om Github, extract zips

Merge remote-tracking branch 'upstream/master' into bin64-1

3f0662d

Add binary test, cleanup

0a05f27

genotrance mentioned this pull request Jul 3, 2018

BUG: choosenim saves to downloads/master.tar.gz which causes clashes when master gets updated; should use git hashes or tags instead #65

Closed

dom96 mentioned this pull request Sep 1, 2018

added windows 64bit support #83

Closed

Repository owner deleted a comment from genotrance Oct 13, 2018

dom96 requested changes Oct 13, 2018

View reviewed changes

This was referenced Oct 23, 2018

Clean up after building a version to save space #98

Closed

[Feature Request] Some sort of --force argument for choosenim update ... #88

Closed

Merge upstream

538c43e

genotrance mentioned this pull request Jan 3, 2019

Serve all downloads from GitHub? #42

Closed

genotrance closed this Jan 3, 2019

iffy pushed a commit to iffy/choosenim that referenced this pull request Jul 11, 2025

Merge pull request dom96#63 from lbartoletti/patch-1

e71f941

fix(switcher.nim): fix CC for FreeBSD


		downloadFile(url, result, params)

		proc downloadCheckRenamed(params: CliParams, url, filename: string): string =

Multiple features #63

Multiple features #63

Uh oh!

Conversation

genotrance commented May 28, 2018

Uh oh!

timotheecour commented Jul 3, 2018

Uh oh!

Araq commented Oct 9, 2018

Uh oh!

dom96 commented Oct 13, 2018

Uh oh!

genotrance commented Oct 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genotrance commented Jan 3, 2019

Uh oh!

Uh oh!