handle tilde expansion for OS home directory #249
Conversation
|
I tried this from a shell script (one script calls another script) and it resolves ~ in the paths. So I think adding support for this probably makes sense. At first I was confused at the motivation - I originally assumed it was primarily only supported interactively by the bash shell. |
node/internal.ts
Outdated
| @@ -426,7 +427,7 @@ export function _which(tool: string, check?: boolean): string { | |||
| function _tryGetExecutablePath(filePath: string, extensions: string[]): string { | |||
| try { | |||
| // test file exists | |||
| let stats: fs.Stats = fs.statSync(filePath); | |||
| let stats: fs.Stats = fs.statSync(_unTildify(filePath)); | |||
There was a problem hiding this comment.
Since untildify is only run on that path that is stat'd and not the path that is returned, wouldn't which ultimately return the "wrong" tildified path?
There was a problem hiding this comment.
filePath in tryGetExe is the segment of the PATH env variable which is what would be prefixed with a tilde.
It's fs.statSync that fails if there is a tilde. See the Node issue where they won't fix
So untildify just expands tilde only when it's a prefix as that should only be the case
All the Tests pass which include tests for running ls, grep, etc
There was a problem hiding this comment.
If someone calls tl.which('sometool') and it returns a tildified-file-path, won't that cause problem depending on what the path is then used for? For instance, if they stat or lstat it will blow up. I would imagine other things may error as well.
I think it would make more sense to do this in the which function when iterating over the directories. And probably only when home is truthy and process.platform != 'win32' and the dir is ~ or starts with ~/. Also I noticed the regex below also handles ~\ which doesn't seem correct to me.
Also I'm curious if you have insight into how common this problem is? The stance node took is interesting, and I think makes sense for node. However, since the task lib exposes a which() function, it makes me feel like we have a better argument to add it (pragmatic reasoning to avoid common confusion).
There was a problem hiding this comment.
let me add some tests that deal with these conditions and take it from there. The regex was taken directly from that library referenced by many in the Node issue.
Please stand by....
There was a problem hiding this comment.
also, within the past week 2 other people using the AZ CLI ran into this problem on macOS. It's fairly routine in .bash_profile to rely on bash support of ~ instead of something like $HOME
There was a problem hiding this comment.
This is going to take longer as ultimately it's let stats: fs.Stats = fs.statSync(filePath); that is failing, yet in each block the ENOENT exception is being eaten, so it passes. Some of these might valid; that's what I have to investigate. e.g.. https://github.com/Microsoft/vsts-task-lib/blob/master/node/internal.ts#L453 and https://github.com/Microsoft/vsts-task-lib/blob/master/node/internal.ts#L495
There was a problem hiding this comment.
thats why i dont want which() to return the tildified path :). the dir should be untildified before it's joined with the file name to construct the candidate full path.
this is related to #4302 microsoft/azure-pipelines-tasks#4302
|
Is there anything new with that? |
|
I'm going to close this since there hasn't been progress for a year and a half, please reopen if appropriate. If people are still experiencing issues from this, lets log an issue and move forward from there. @cicorias if you're planning on continuing work on this, feel free to reopen here or start a new PR. |
this is related to microsoft/azure-pipelines-tasks#4302
This handles tilde
~expansion for bash shells where thePATHhas a tilde such as~/bin.Since node's
fs.statSyncdoesn't expand tilde's, See this conversation as well as to why node team didn't fix. nodejs/node#684 (comment)