Remove broken symlinks when updating deps #2842
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,26 @@ set -o pipefail | |
|
|
||
| source $(dirname $0)/../vendor/github.com/knative/test-infra/scripts/library.sh | ||
|
|
||
| # Remove symlinks in /vendor that are broken or lead outside the repo. | ||
| function remove_broken_symlinks() { | ||
| for link in $(find ./vendor -type l); do | ||
| local target="$(ls -l ${link})" | ||
| target="${target##* -> }" | ||
| # Remove broken symlinks | ||
| if [[ ! -e ${link} ]]; then | ||
erain marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| unlink ${link} | ||
| continue | ||
| fi | ||
| # Get canonical path to target, remove if outside the repo | ||
| [[ ${target} == /* ]] || target="./${target}" | ||
|
There was a problem hiding this comment. I don't understand what's going on here, and why this is needed to find a canonical path. I'd have this follow the links recursively. I understand we want this to work with MacOS X/BSD, so we only have a simple function remove_external_links() { # Removes a chain of links pointing out of tree
if ! [[ -e $1 ]]; then
return 1 # This link doesn't point anywhere.
fi
if ! [[ -L $1 ]]; then
return 0 # This is a real file/directory which is not a link.
fi
# This determines if it points outside the tree, either absolutely, or outside vendor.
target="$(readlink "$1")"
if [[ $target == /* ]] || [[ "$(cd $(dirname "$target"; pwd)/" == ${PWD}/vendor/* ]]; then
rm "$1" # Remove the errant link, but not its target (which is outside our tree)
return 1
fi
if ! remove_external_links "$target"; then
rm "$1" # Recursive examination found a bad target, remove this link, too.
return 1
fi
}
remove_external_links "${link}"There was a problem hiding this comment. FWIW, I tested this as a set of one-liners, so you may want to re-test if you decide to use it. Note that the current script does not handle any of the following:
Unfortuantely, for the third item, I think you have to process each segment of the There was a problem hiding this comment. a) There was a problem hiding this comment. For b) If you don't check beyond the first target, there may still be dangling symlinks after running N passes of this, depending on the order of directory entries returned by the OS. Consider the following: $ cd /tmp/x
$ ls -l
b -> ../a
c -> b
d -> c
$ for x in $(find . -type l ); do [[ -L $x ]] || continue; t=$(readlink $x); [[ $t == /* ]] || t="./$t"; t2="$(cd $(dirname $x) && cd ${t%/*} && echo $PWD/${t##*/})"; echo $x $t $t2; if [[ ${t2}/ != $PWD/* ]]; then echo remove $x; continue; fi; done
./d ./c /tmp/x/c
./c ./b /tmp/x/b
./b ./../a /tmp/a
remove ./bAt the end of this, Unfortunately, I just discovered a different problem with my solution, which is that a symlink loop will cause it to recurse until stack space is exhausted. 🤷♂️ I'm okay with this solution, but wanted to point out that it might miss symlink chains where the last element in the chain points outside the tree (and that order will depend on the order that directory entries are added and returned, which is OS-dependent). There was a problem hiding this comment. Actually "following the symlink all the way by simple And that approach can solve the problem you mentioned, since each symlink will be solved to the canonical path of a final target (and be deleted if it is outside of the source tree). Let me know if you want me to revert to that approach, or do the minor improvement (as your first comment) on this approach. There was a problem hiding this comment. The version in 1adb644 doesn't check that the intermediate symlinks are inside the tree, allowing for e.g. vendor/foo --> /home/evan/foo --> /home/evan/go/src/github.com/knative/serving/vendor/bar With that said, I'm happy with a simpler solution for now and then having There was a problem hiding this comment. (And I've approved, so if people want this version, they can just |
||
| target="$(cd `dirname ${link}` && cd ${target%/*} && echo $PWD/${target##*/})" | ||
| if [[ ${target} != *github.com/knative/* ]]; then | ||
| unlink ${link} | ||
| continue | ||
| fi | ||
| done | ||
| } | ||
|
|
||
| cd ${REPO_ROOT_DIR} | ||
|
|
||
| # Ensure we have everything we need under vendor/ | ||
|
|
@@ -35,3 +55,6 @@ update_licenses third_party/VENDOR-LICENSE "./cmd/*" | |
| # cherrypick of #66078. Remove this once that reaches a client version | ||
| # we have pulled in. | ||
| git apply ${REPO_ROOT_DIR}/hack/66078.patch | ||
|
|
||
| # Remove all invalid symlinks under ./vendor | ||
| remove_broken_symlinks | ||
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'd suggest using
readlinkfor this: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, it doesn't work on Macs. That was the major reason for this PR taking so long to be finished.
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 think vanilla
readlinkwithout-fworks on Macs. At least one test on Dan's laptop suggested this was installed in/bin, which is pretty likely to be a core utility.