-
Notifications
You must be signed in to change notification settings - Fork 170
Discard output from the cd command when followed by pwd #16238
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
base: master
Are you sure you want to change the base?
Conversation
This commits fixes the issue when `cd` is followed by the `pwd` in a bash one-liner which should get the directory in which the executed script is located. The problem with such approach is that the `cd` command can print the destination directory in case when one has the CDPATH environmental variable set. From the `man bash`: > cd [-L|...] [dir] > Change the current directory to dir. [...] If a non-empty directory > name from CDPATH is used the absolute pathname of the new working > directory is written to the standard output. ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy <[email protected]>
seanshpark
left a comment
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.
plz split to separate PR for circle-mlir/* ,
and for each compiler/* to each PRs,
and infra/*
| ''''test -f ${PY_PATH} && exec ${PY_PATH} "$0" "$@" # ''' | ||
| ''''echo "Error: Virtual environment not found. Please run 'one-prepare-venv' command." # ''' | ||
| ''''exit 255 # ''' | ||
| ''''export SCRIPT_PATH="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" >/dev/null && pwd)" # ''' |
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.
is this really needed?
what is the problem?
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.
I've just checked this particular one-liner and you are right, everything is OK here!
So, here is the detailed explanation what is going on, and why I've raised this PR:
Create some directory in your $HOME, e.g. mkdir -p $HOME/dir/subdir, then create a bash script in it (and make it executable), like this:
$ cat $HOME/dir/subdir/test.sh
#!/bin/bash
SOURCEDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
echo "Source dir: $SOURCEDIR"Then run this script:
$ ./dir/subdir/test.sh
Source dir: /home/arkq/dir/subdir
$ dir/subdir/test.sh
Source dir: /home/arkq/dir/subdirNow set the CDPATH env variable and run the script again:
$ export CDPATH=$HOME:$HOME/dir
$ ./dir/subdir/test.sh
Source dir: /home/arkq/dir/subdir
$ dir/subdir/test.sh
Source dir: /home/arkq/dir/subdir
/home/a.bokowy/dir/subdirIn the second test, the cd "$(dirname "${BASH_SOURCE[0]}")" prints the destination directory /home/arkq/dir/subdir, so the entire one-liner gets duplicated output. HOWEVER, if the one-liner uses readlink -f, e.g.: SOURCEDIR="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)" the cd gets the absolute path, and in such case the destination directory is not printed!
$ export CDPATH=$HOME:$HOME/dir/subdir
$ cd dir/subdir
/home/arkq/dir/subdir
$ cd $HOME
$ cd $HOME/dir/subdir
$ cd $HOMESo, now the question which approach is better, discarding output or using readlink -f? I think that discarding output is safer, because resolving symlinks will cd you into different directory structure if somewhere along the path symlink was used. Then, relative paths (ones with ../../../) will no longer work.
I will update this PR to account work this finding. Also I will split this PR info separate ones for different parts of this repo.
Many thanks for the review! :)
You should test all your changes. That said, posting 110 files in single PR is not good practice. |
This commits fixes the issue when
cdis followed by thepwdin a bash one-liner which should get the directory in which the executed script is located. The problem with such approach is that thecdcommand can print the destination directory in case when one has the CDPATH environmental variable set.From the
man bash:Testing
I've only tested the
runtime/tests/scripts/onert-testscript, so I hope that the rest will be verified by CI. Now, in my setup I'm able to run it. Before that change the outcome was like this:ONE-DCO-1.0-Signed-off-by: Arkadiusz Bokowy [email protected]