Ensure proper path traversals for linked commands;

Ensure proper path traversals for linked commands;

These changes involve the bulk of scripted commands in sdk/bin.

Example:
consider:   $ ls -s $DART/dart-sdk/bin/dart2js  /usr/local/bin/dart2js

Problem:
* old: code would cd to the dirname of $0, and get the linked path to that dir.
  - e.g.:  BIN_DIR=/usr/local/bin  # wrong;  want  $DART/dart-sdk/bin

* Solution:  follow links on $0 (not it's dir);  return it's dir, and resolve any remaining links on that;
  - additional commentary:  (from original code):
     -# Unlike $0, $BASH_SOURCE points to the absolute path of this file.
  - a simple test shows this to be false (at least on OS/X; no bash documentation supports this claim either);
  - while $(source some_script) will return the shell in $0 (not "some_script), these commands end in exec,
    and will never be called.  $0 is clearer, more direct about intent (as well as more general across shells).

* Problem (e.g. dartanalyzer, dartdoc, pub)
  - old code would resolve links to path of command link (but not command itself first);

* Solution:  add function / idiom, similar to dart2js;

* Problem (general):
   - function follow_links()  used illegal variable name on left-hand-side
      - e.g. 1=$2   # positional vars cannot be assigned to
   - function returned a file-name path, where everywhere a dirname path required
* Solution:
   - assign positional parameter to a valid variable name;
   - return dir of linked-to-file rather than linked file;
   - rename function

BUG=dart:10806

[ahe, 2013-05-29 05:29:10]

I don't think this handles the case where someone has made a symlink to sdk/bin.

And as I mentioned in the bug, you should use $BASH_SOURCE, not $0.

As far as I have been able to determine, $BASH_SOURCE is best for when sdk/bin
has been added to the path. I'm that case, $0 may be "dart2js", whereas
$BASH_SOURCE seems to be a relative path, IIRC.  I haven't found any
documentation supporting this claim, but I found several other sources on the
web who confirmed this.

[yarkot1, 2013-05-29 22:22:27]

On 2013/05/29 05:29:10, ahe wrote:
> I don't think this handles the case where someone has made a symlink to
sdk/bin.
> 
> And as I mentioned in the bug, you should use $BASH_SOURCE, not $0.

I have, as yet, seen no technical reason to support this.  I can see it as a
stylistic preference.

In the case of direct shell script invocation, 
$_, $0, $BASH_SOURCE, and $BASH_SOURCE[0] return the same expanded values in my
tests
(so far, just on OS/X - might check on debian, possibly cygwin).  But this isn't
worth a lot of attention.
Anything will do.

> 
> As far as I have been able to determine, $BASH_SOURCE is best for when sdk/bin
> has been added to the path. I'm that case, $0 may be "dart2js", whereas
> $BASH_SOURCE seems to be a relative path, IIRC.  I haven't found any
> documentation supporting this claim, but I found several other sources on the
> web who confirmed this.

You are correct about linking a directory instead of the command.   SHall I
remove this patch (still some work to do)?
Upload an updated patch?

Not sure what the preferred process / protocol is here.

[yarkot1, 2013-05-30 00:09:03]

On 2013/05/29 22:22:27, yarkot1 wrote:
> On 2013/05/29 05:29:10, ahe wrote:
> > I don't think this handles the case where someone has made a symlink to
> sdk/bin.
> > 
> > And as I mentioned in the bug, you should use $BASH_SOURCE, not $0.
> 
> I have, as yet, seen no technical reason to support this.  I can see it as a
> stylistic preference.
> 
> In the case of direct shell script invocation, 
> $_, $0, $BASH_SOURCE, and $BASH_SOURCE[0] return the same expanded values in
my
> tests
> (so far, just on OS/X - might check on debian, possibly cygwin).  But this
isn't
> worth a lot of attention.
> Anything will do.
> 
> > 
> > As far as I have been able to determine, $BASH_SOURCE is best for when
sdk/bin
> > has been added to the path. I'm that case, $0 may be "dart2js", whereas
> > $BASH_SOURCE seems to be a relative path, IIRC.  I haven't found any
> > documentation supporting this claim, but I found several other sources on
the
> > web who confirmed this.
> 
> You are correct about linking a directory instead of the command.   SHall I
> remove this patch (still some work to do)?
> Upload an updated patch?
> 
> Not sure what the preferred process / protocol is here.

sorry, rushing out for the bus:   
I mean $0 vs. $BASH_SOURCE values appear same regardless of sdk/bin added to
path (or commands linked from /usr/local/bin).

The problem with this, my patch is when there is a link to sdk/bin  (that is
directory level, rather than command level links).

This is what I'll look at tonight after dinner, as it looks like that was the
issue: these two cases (cmd vs. dir linked).

[yarkot1, 2013-05-30 05:05:16]

On 2013/05/30 00:09:03, yarkot1 wrote:
> On 2013/05/29 22:22:27, yarkot1 wrote:
> > On 2013/05/29 05:29:10, ahe wrote:
> > > I don't think this handles the case where someone has made a symlink to
> > sdk/bin.
> > > 
> > > And as I mentioned in the bug, you should use $BASH_SOURCE, not $0.
> > 
> > I have, as yet, seen no technical reason to support this.  I can see it as a
> > stylistic preference.
> > 
> > In the case of direct shell script invocation, 
> > $_, $0, $BASH_SOURCE, and $BASH_SOURCE[0] return the same expanded values in
> my
> > tests
> > (so far, just on OS/X - might check on debian, possibly cygwin).  But this
> isn't
> > worth a lot of attention.
> > Anything will do.
> > 
> > > 
> > > As far as I have been able to determine, $BASH_SOURCE is best for when
> sdk/bin
> > > has been added to the path. I'm that case, $0 may be "dart2js", whereas
> > > $BASH_SOURCE seems to be a relative path, IIRC.  I haven't found any
> > > documentation supporting this claim, but I found several other sources on
> the
> > > web who confirmed this.
> > 
> > You are correct about linking a directory instead of the command.   SHall I
> > remove this patch (still some work to do)?
> > Upload an updated patch?
> > 
> > Not sure what the preferred process / protocol is here.
> 
> sorry, rushing out for the bus:   
> I mean $0 vs. $BASH_SOURCE values appear same regardless of sdk/bin added to
> path (or commands linked from /usr/local/bin).
> 
> The problem with this, my patch is when there is a link to sdk/bin  (that is
> directory level, rather than command level links).
> 
> This is what I'll look at tonight after dinner, as it looks like that was the
> issue: these two cases (cmd vs. dir linked).

Please direct your comments to the second patch set.

[ahe, 2013-05-30 10:00:19]

I don't understand most of the changes made to the dart2js script.  As far as I
can tell, all problems you have reported with that script are fixed by changing
follow_links to:

function follow_links() {
  file="$1"
  while [ -h "$file" ]; do
    # On Mac OS, readlink -f doesn't work.
    file="$(readlink "$file")"
  done
  echo "$file"
}

As for $0 versus $BASH_SOURCE, they are different, and we care about situations
where the difference matter. So the only viable option is to use $BASH_SOURCE.

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js
File sdk/bin/dart2js (right):

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode11
sdk/bin/dart2js:11: # if dir linked, not file, need to follow dir path:
This comment does not make sense to me.

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode12
sdk/bin/dart2js:12: echo "$(cd $(dirname "$a"); pwd -P)"
This invokes the external command dirname.  That adds overhead that I have tried
to avoid by using "${a%/*}".

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode15
sdk/bin/dart2js:15: # Handle the case where dart-sdk/bin has been symlinked to.
You're still not handling this case.

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode16
sdk/bin/dart2js:16: BIN_DIR="$(link_dir "$0")"
$0 is not the right thing to use.  See the difference:

$ mkdir subdir
$ cat > subdir/script.bash
#!/bin/bash

echo "$0"
echo "$BASH_SOURCE"
$ cat > super.bash
#!/bin/bash

. subdir/script.bash
$ bash super.bash 
super.bash
subdir/script.bash
$

[yarkot1, 2013-05-31 04:39:50]

On 2013/05/30 10:00:19, ahe wrote:
> I don't understand most of the changes made to the dart2js script.  As far as
I
> can tell, all problems you have reported with that script are fixed by
changing
> follow_links to:
> 
> function follow_links() {
>   file="$1"
>   while [ -h "$file" ]; do
>     # On Mac OS, readlink -f doesn't work.
>     file="$(readlink "$file")"
>   done
>   echo "$file"
> }
> 
> As for $0 versus $BASH_SOURCE, they are different, and we care about
situations
> where the difference matter. So the only viable option is to use $BASH_SOURCE.
> 
> https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js
> File sdk/bin/dart2js (right):
> 
> https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode11
> sdk/bin/dart2js:11: # if dir linked, not file, need to follow dir path:
> This comment does not make sense to me.
> 
> https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode12
> sdk/bin/dart2js:12: echo "$(cd $(dirname "$a"); pwd -P)"
> This invokes the external command dirname.  That adds overhead that I have
tried
> to avoid by using "${a%/*}".
> 
> https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode15
> sdk/bin/dart2js:15: # Handle the case where dart-sdk/bin has been symlinked
to.
> You're still not handling this case.
> 
> https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode16
> sdk/bin/dart2js:16: BIN_DIR="$(link_dir "$0")"
> $0 is not the right thing to use.  See the difference:
> 
> $ mkdir subdir
> $ cat > subdir/script.bash
> #!/bin/bash
> 
> echo "$0"
> echo "$BASH_SOURCE"
> $ cat > super.bash
> #!/bin/bash
> 
> . subdir/script.bash
> $ bash super.bash 
> super.bash
> subdir/script.bash
> $

To help this along, I am in the process of writing shunit2 tests to both
demonstrate, and in the future collect and check the issues.  Unless given other
direction, I'll initially put this under tests/shutils (new test directory).

As for $0 vs. $BASH_SOURCE, on OSX I am seeing no difference - if I have time, I
will also test under ubuntu and cygwin.  As long as I'm heading in this
direction, are there any other platforms you are concerned with, w.r.t. shell
scripts?   If I don't have time, I'll just put in BASH_SOURCE, as you would like
(but I'd like to have / show definitive reason for this, if possible).

[ahe, 2013-05-31 07:57:36]

On 2013/05/31 04:39:50, yarkot1 wrote:
> To help this along, I am in the process of writing shunit2 tests to both
> demonstrate, and in the future collect and check the issues.  Unless given
other
> direction, I'll initially put this under tests/shutils (new test directory).

Awesome!

> As for $0 vs. $BASH_SOURCE, on OSX I am seeing no difference - if I have time,
I
> will also test under ubuntu and cygwin.  As long as I'm heading in this
> direction, are there any other platforms you are concerned with, w.r.t. shell
> scripts?   If I don't have time, I'll just put in BASH_SOURCE, as you would
like
> (but I'd like to have / show definitive reason for this, if possible).

When you source a file, there is a difference between $0 and $BASH_SOURCE.  We
plan to move dart2js_developer to a different location, so it is important that
we use $BASH_SOURCE, not $0.

[ahe, 2013-05-31 07:58:50]

Turns out that Nathan is working on something similar, see:
https://codereview.chromium.org/15362003/

It would be great if you can coordinate efforts.

[nweiz, 2013-05-31 19:35:21]

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js
File sdk/bin/dart2js (right):

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode15
sdk/bin/dart2js:15: # Handle the case where dart-sdk/bin has been symlinked to.
On 2013/05/30 10:00:19, ahe wrote:
> You're still not handling this case.

This case is handled by the "pwd -P" call in link_dir.

[yarkot1, 2013-05-31 22:22:17]

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js
File sdk/bin/dart2js (right):

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode11
sdk/bin/dart2js:11: # if dir linked, not file, need to follow dir path:
On 2013/05/30 10:00:19, ahe wrote:
> This comment does not make sense to me.

if $a (filename) is a file, readlink handles following to the linked target;
If, however, $(dirname a) was the link, we readlink will miss it;  then we
need the $(cd ...; pwd -P) to follow to the linked target.

i.e. if the dirname (not the filename) was what was linked, we need this next
line.

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode12
sdk/bin/dart2js:12: echo "$(cd $(dirname "$a"); pwd -P)"
On 2013/05/30 10:00:19, ahe wrote:
> This invokes the external command dirname.  That adds overhead that I have
tried
> to avoid by using "${a%/*}".

yes - you are correct.  Timing this confirms.

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode16
sdk/bin/dart2js:16: BIN_DIR="$(link_dir "$0")"
On 2013/05/30 10:00:19, ahe wrote:
> $0 is not the right thing to use.  See the difference:
> 
> $ mkdir subdir
> $ cat > subdir/script.bash
> #!/bin/bash
> 
> echo "$0"
> echo "$BASH_SOURCE"
> $ cat > super.bash
> #!/bin/bash
> 
> . subdir/script.bash
> $ bash super.bash 
> super.bash
> subdir/script.bash
> $

yes - this is true in the case of sourced scripts;  will change.

[ahe, 2013-06-03 10:29:01]

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js
File sdk/bin/dart2js (right):

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode11
sdk/bin/dart2js:11: # if dir linked, not file, need to follow dir path:
On 2013/05/31 22:22:17, yarkot1 wrote:
> On 2013/05/30 10:00:19, ahe wrote:
> > This comment does not make sense to me.
> 
> if $a (filename) is a file, readlink handles following to the linked target;
> If, however, $(dirname a) was the link, we readlink will miss it;  then we
> need the $(cd ...; pwd -P) to follow to the linked target.
> 
> i.e. if the dirname (not the filename) was what was linked, we need this next
> line.
> 

Could you change the comment to:

# If the parent directory of $a is a symlink, we need to follow it.  pwd -P does
that:

https://codereview.chromium.org/15954011/diff/5001/sdk/bin/dart2js#newcode15
sdk/bin/dart2js:15: # Handle the case where dart-sdk/bin has been symlinked to.
On 2013/05/31 19:35:21, nweiz wrote:
> On 2013/05/30 10:00:19, ahe wrote:
> > You're still not handling this case.
> 
> This case is handled by the "pwd -P" call in link_dir.

Thank you.  I just studied the man page, and you're absolutely right.

[kasperl, 2013-11-28 10:02:03]

Just found this review still sitting around. How far are we from being able to
land this?

[kasperl, 2013-11-28 10:31:22]

Looks like the main problem this is addressing may have been fixed in
https://codereview.chromium.org/68503004/. Are there any of the changes in this
"issue" that we should still try to get submitted?