Skip to content
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

Fix #1118: follow only dir-symlinks #1120

Conversation

webknjaz
Copy link

@webknjaz webknjaz commented Mar 15, 2018

This corrects the deffect where pyenv tries to determine an absolute
path of the directory containing a file and follows symlinks
manually. Because of missing check it behaved correctly, but a
misleading error message was shown in the terminal output.
Resolves #1118.

Thanks to @HQJaTu for the initial patch.

This corrects the deffect where pyenv tries to determine an absolute
path of the directory containing a file and follows symlinks
manually. Because of missing check it behaved correctly, but a
misleading error message was shown in the terminal output.

Thanks to @HQJaTu for the patch
@webknjaz
Copy link
Author

@joshfriend @yyuu @timsavage what is the policy for bugfix releases? can we have this out asap plz?

@webknjaz
Copy link
Author

Ping @joshfriend @yyuu @timsavage

@timsavage
Copy link
Contributor

Please stop referencing me, I'm not part of the pyenv team.

@webknjaz
Copy link
Author

Oops, sorry. I just saw your commits in master and missed they they're coming from PR :)

@@ -40,7 +40,7 @@ else

# Use a subshell to avoid changing the current path
(
while [ -n "$path" ]; do
while [[ -n "$path" && $(echo "$path" | grep /) ]]; do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will return "pwd" if "$path" is /. At least it must not be the correct return value of the function.

Copy link
Contributor

@yyuu yyuu Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some fix like following will work sufficiently. Please consider adding tests for it.

while [[ "$path" == *"/"* ]]; do
  cd "${path%/*}"
  local name="${path##*/}"
  [ -n "$name" ] || break
  path="$(resolve_link "$name" || true)"
done

pwd

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll check this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked it: it doesn't work your way at all. Will need to investigate some better way.

@blueyed
Copy link
Contributor

blueyed commented Sep 19, 2018

I've started a while back to fix this in rbenv (upstream), but it needs updating: rbenv/rbenv#868.

@blueyed
Copy link
Contributor

blueyed commented Sep 19, 2018

btw: src/configure && make -C src in pyenv's checkout should fix / work around this, and improve performance as a side-effect.

@webknjaz
Copy link
Author

Wow, why?

/me needs to get back to fixing this

@blueyed
Copy link
Contributor

blueyed commented Sep 19, 2018

Closing in favor of #1216.

@blueyed blueyed closed this Sep 19, 2018
@blueyed
Copy link
Contributor

blueyed commented Sep 19, 2018

@webknjaz
The code that causes this error is only a fallback, and the realpath extension is preferred in general.
I've created #1216 now to fix this finally hopefully, please review/test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants