-
Notifications
You must be signed in to change notification settings - Fork 109
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
Implement engine_getClientVersionV1 #2233
Conversation
nimbus/version.nim
Outdated
# [0..7] : remove trailing chars(e.g. on Github Windows CI) | ||
GitRevision* = strip(staticExec("git rev-parse --short=8 HEAD")) | ||
# -C sourcePath: get the correct git hash no matter where the current dir is. | ||
GitRevision* = strip(staticExec("git -C " & sourcePath & " rev-parse --short=8 HEAD")) |
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.
Might this need something similar to strutils.escape(...) if paths contain spaces or other non-alphanumeric characters?
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.
no, I've found the problem. To add more spaces, the .git folder is deleted in Windows CI.
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.
The .git folder being deleted in the Windows CI evidently indeed was the key issue here, but this still won't work when the sourcePath
contains spaces or similar characters.
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.
The current approach in 8773258 isn't sufficient either:
$ ls -l /tmp/staging/
total 4
drwxrwxr-x 3 user user 4096 May 28 18:23 'path with " in it'
$ git -C /tmp/staging/path\ with\ \"\ in\ it/ rev-parse --short=8 HEAD
8dc8fb68
$ cat a.nim
const sourcePath = "/tmp/staging/path with \" in it"
from std/os import dirExists
doAssert dirExists(sourcePath)
echo "directory exists named: " & sourcePath
const foo = staticExec("git -C \"" & sourcePath & "\" rev-parse --short=8 HEAD")
echo "commithash is " & foo
$ ./env nim c -r a
...
directory exists named: /tmp/staging/path with " in it
commithash is /bin/sh: 1: Syntax error: Unterminated quoted string
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.
ha ha forget to escape
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.
7e284dd seems to add extra quotation marks now? strutils.escape
already adds them:
import std/strutils
echo strutils.escape("some string")
echo strutils.escape("some")
echo strutils.escape("embedded \" character")
results in
"some string"
"some"
"embedded \" character"
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.
huh?, I thought strutils.escape
only escape chars, turn out it also add prefix and suffix.
@@ -267,7 +267,7 @@ jobs: | |||
mingw32-make ${DEFAULT_MAKE_FLAGS} | |||
build/nimbus.exe --help | |||
# give us more space | |||
find . -type d -name ".git" -exec rm -rf {} + | |||
# find . -type d -name ".git" -exec rm -rf {} + |
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.
You introduced this last year: 4893097
What were the conditions then such that it made sense, and have they persisted?
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.
iirc at that time windows runner have less than 14GB of disk space, although the documentation says all OS are given the same disk space. Don't know today.
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.
As long as it the CI builds still work, however much disk space the CI runners have, not an issue, then. If reverting this change triggers disk exhaustion in Windows CI runners, can try other approaches.
General question, if someone has a non- const foo = staticExec("git -C /tmp rev-parse --short=8 HEAD") # an extant directory, but not a git repo directory
echo "commithash is: " & foo produces:
This seems less than optimal. |
Now they will get zero git revision if they compile from source tarball or download github zip. And we can configure the build system to give some number to replace git commit hash. |
Sure, as reasonable as anything |
fix #2230