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

Multiple features #63

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/choosenim.nim
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,29 @@ proc parseVersion(versionStr: string): Version =
result = newVersion(versionStr)

proc installVersion(version: Version, params: CliParams) =
when defined(windows):
# Add MingW bin dir to PATH so script can find gcc.
if findExe("gcc") == "" and dirExists(params.getMingwBin()):
let pathEnv = getEnv("PATH")
putEnv("PATH", params.getMingwBin() & PathSep & pathEnv)
defer:
putEnv("PATH", pathEnv)

# Install the requested version.
let path = download(version, params)
# Extract the downloaded file.
let extractDir = params.getInstallationDir(version)
# Make sure no stale files from previous installation exist.
removeDir(extractDir)
extract(path, extractDir)

when defined(windows):
# 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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just use moveDir here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moveDir() raises an exception since directory already exists.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

removeDir(winFolder)

# A "special" version is downloaded from GitHub and thus needs a `.git`
# directory in order to let `koch` know that it should download a "devel"
# Nimble.
Expand Down Expand Up @@ -241,4 +257,4 @@ when isMainModule:
handleTelemetry(params)
quit(QuitFailure)

handleTelemetry(params)
handleTelemetry(params)
20 changes: 8 additions & 12 deletions src/choosenim/builder.nim
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@ import nimblepkg/common as nimble_common
import cliparams, download, utils, common, telemetry

proc buildFromCSources() =
let arch = getGccArch()
when defined(windows):
doCmdRaw("build.bat")
# TODO: How should we handle x86 vs amd64?
if arch == 32:
doCmdRaw("build.bat")
elif arch == 64:
doCmdRaw("build64.bat")
else:
doCmdRaw("sh build.sh")

proc buildCompiler(params: CliParams) =
proc buildCompiler(params: CliParams, version: Version) =
## Assumes that CWD contains the compiler (``build`` should have changed it).
let binDir = getCurrentDir() / "bin"
if fileExists(binDir / "nim".addFileExt(ExeExt)):
Expand All @@ -22,10 +25,7 @@ proc buildCompiler(params: CliParams) =
if fileExists(getCurrentDir() / "build.sh"):
buildFromCSources()
else:
display("Warning:", "Building from latest C sources. They may not be " &
"compatible with the Nim version you have chosen to " &
"install.", Warning, HighPriority)
let path = downloadCSources(params)
let path = downloadCSources(params, version)
let extractDir = getCurrentDir() / "csources"
extract(path, extractDir)

Expand Down Expand Up @@ -77,18 +77,14 @@ proc build*(extractDir: string, version: Version, params: CliParams) =

let currentDir = getCurrentDir()
setCurrentDir(extractDir)
# Add MingW bin dir to PATH so that `build.bat` script can find gcc.
let pathEnv = getEnv("PATH")
putEnv("PATH", params.getMingwBin() & PathSep & pathEnv)
defer:
setCurrentDir(currentDir)
putEnv("PATH", pathEnv)

display("Building", "Nim " & $version, priority = HighPriority)

var success = false
try:
buildCompiler(params)
buildCompiler(params, version)
buildTools()
success = true
except NimbleError as exc:
Expand Down
78 changes: 47 additions & 31 deletions src/choosenim/download.nim
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ const
githubUrl = "https://github.com/nim-lang/Nim/archive/$1.tar.gz"
websiteUrl = "http://nim-lang.org/download/nim-$1.tar" &
getArchiveFormat()
csourcesUrl = "https://github.com/nim-lang/csources/archive/master.tar.gz"
csourcesUrl = "https://github.com/nim-lang/csources/archive/$1.tar.gz"
winBinaryZipUrl = "http://nim-lang.org/download/nim-$1_x$2.zip"

const # Windows-only
mingwUrl = "http://nim-lang.org/download/mingw32.tar.gz"
Expand Down Expand Up @@ -198,7 +199,21 @@ proc needsDownload(params: CliParams, downloadUrl: string,
priority=HighPriority)
return false

proc downloadCheck(params: CliParams, url: string): string =
result = ""
if not needsDownload(params, url, result): return

downloadFile(url, result, params)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe hashedDownload?

# Special check since we have to rename to disambiguate
result = getDownloadDir(params) / filename
if not fileExists(result):
let origfile = downloadCheck(params, url)
moveFile(origfile, result)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


proc downloadImpl(version: Version, params: CliParams): string =
let arch = getGccArch()
if version.isSpecial():
let reference =
case normalize($version)
Expand All @@ -208,21 +223,28 @@ proc downloadImpl(version: Version, params: CliParams): string =
($version)[1 .. ^1]
display("Downloading", "Nim $1 from $2" % [reference, "GitHub"],
priority = HighPriority)
let url = githubUrl % reference
var outputPath: string
if not needsDownload(params, url, outputPath): return outputPath

downloadFile(url, outputPath, params)
result = outputPath
result = downloadCheck(params, githubUrl % reference)
else:
display("Downloading", "Nim $1 from $2" % [$version, "nim-lang.org"],
priority = HighPriority)
let url = websiteUrl % $version
var outputPath: string
if not needsDownload(params, url, outputPath): return outputPath

downloadFile(url, outputPath, params)
result = outputPath
# Check if binary build available
when defined(windows):
try:
result = downloadCheck(params, winBinaryZipUrl % [$version, $arch])
return
except HttpRequestError:
display("Info:", "Binary build unavailable, building from source",
priority = HighPriority)

# Check if source tar.gz posted on nim-lang.org
try:
result = downloadCheck(params, websiteUrl % $version)
except HttpRequestError:
# Fallback to downloading from Github
# Rename to nim-VERSION.tar.gz to disambiguate
result = downloadCheckRenamed(params, githubUrl % ("v" & $version),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"nim-$1.tar.gz" % $version)

proc download*(version: Version, params: CliParams): string =
## Returns the path of the downloaded .tar.(gz|xz) file.
Expand All @@ -232,32 +254,26 @@ proc download*(version: Version, params: CliParams): string =
raise newException(ChooseNimError, "Version $1 does not exist." %
$version)

proc downloadCSources*(params: CliParams): string =
var outputPath: string
if not needsDownload(params, csourcesUrl, outputPath):
return outputPath

proc downloadCSources*(params: CliParams, version: Version): string =
display("Downloading", "Nim C sources from GitHub", priority = HighPriority)
downloadFile(csourcesUrl, outputPath, params)
return outputPath
try:
# Download csources tagged version from Github
# Rename to csources-VERSION.tar.gz to disambiguate
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick:

*Downloading the latest C sources

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"compatible with the Nim version you have chosen to " &
"install.", Warning, HighPriority)

proc downloadMingw32*(params: CliParams): string =
var outputPath: string
if not needsDownload(params, mingwUrl, outputPath):
return outputPath

display("Downloading", "C compiler (Mingw32)", priority = HighPriority)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

downloadFile(mingwUrl, outputPath, params)
return outputPath
result = downloadCheck(params, mingwUrl)

proc downloadDLLs*(params: CliParams): string =
var outputPath: string
if not needsDownload(params, dllsUrl, outputPath):
return outputPath

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above.


proc retrieveUrl*(url: string): string =
when defined(curl):
Expand Down
33 changes: 33 additions & 0 deletions src/choosenim/utils.nim
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,24 @@ proc doCmdRaw*(cmd: string) =
"Execution failed with exit code $1\nCommand: $2\nOutput: $3" %
[$exitCode, cmd, output])

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'); }\""
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


let (outp, errC) = execCmdEx(cmd % [path, extractDir])
if errC != 0:
raise newException(ChooseNimError, "Unable to extract ZIP. Error was $1" % outp)

proc extract*(path: string, extractDir: string) =
display("Extracting", path.extractFilename(), priority = HighPriority)

let ext = path.splitFile().ext
var newPath = path
case ext
of ".zip":
extractZip(path, extractDir)
return
of ".xz":
# We need to decompress manually.
let unxzPath = findExe("unxz")
Expand All @@ -51,6 +63,27 @@ proc extract*(path: string, extractDir: string) =
raise newException(ChooseNimError, "Unable to extract. Error was '$1'." %
exc.msg)

proc moveDirContents*(srcDir, dstDir: string) =
for kind, entry in walkDir(srcDir):
if kind in [pcFile, pcLinkToFile]:
moveFile(entry, dstDir/entry.extractFilename())
else:
moveDir(entry, dstDir/entry.extractFilename())

proc getGccArch*(): int =
# gcc should be in PATH
var
outp = ""
errC = 0

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not POSIX Shell syntax?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

else:
(outp, errC) = execCmdEx("sh echo \"int main() { return sizeof(void *); }\" | gcc -xc - -o archtest && archtest")

removeFile("archtest".addFileExt(ExeExt))
return errC * 8

proc getProxy*(): Proxy =
## Returns ``nil`` if no proxy is specified.
var url = ""
Expand Down
4 changes: 4 additions & 0 deletions tests/tester.nim
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ test "can choose v0.16.0":

check inLines(output.processOutput, "building")
check inLines(output.processOutput, "downloading")
when defined(windows):
check inLines(output.processOutput, "already built")
else:
check inLines(output.processOutput, "building tools")
check hasLine(output.processOutput, "switched to nim 0.16.0")

block:
Expand Down