-
Notifications
You must be signed in to change notification settings - Fork 68
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 fixes #156
Multiple fixes #156
Conversation
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.
See my comment, otherwise looks good.
src/choosenim/builder.nim
Outdated
if kind == pcFile: | ||
# Set to 755 = rwxrw-rw- in octal | ||
# = 493 in decimal | ||
discard chmod(path, 493) |
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 shouldn't need posix for this, can you just use the os
module?
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.
Fixed.
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.
Thanks!
when defined(posix): | ||
proc setPermissions() = | ||
## Assumes that CWD contains the compiler | ||
let binDir = getCurrentDir() / "bin" | ||
for kind, path in walkDir(binDir): | ||
if kind == pcFile: | ||
setFilePermissions(path, | ||
{fpUserRead, fpUserWrite, fpUserExec, | ||
fpGroupRead, fpGroupExec, | ||
fpOthersRead, fpOthersExec} | ||
) | ||
|
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.
Another thing that would be nice would be a debug
output to say that these file permission changes are taking place. But I'll leave that to another PR if you agree that it's a good idea.
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.
To do that, we will have to first check the extracted permissions. Do you want to add that additional code?
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? Just show the message always, even if the permissions aren't being changed. It'll be hidden anyway most of the time.
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.
Done in PR #158.
Fix #147 - workaround rather since we don't have root cause yet
Fix #151 - correct binary names in CI
Add testament to list of proxies