Skip to content

install_symlink_p: Fix when dest includes a symlink#5793

Merged
MikeMcQuaid merged 1 commit intoHomebrew:masterfrom
sjackman:install_symlink_p
Mar 17, 2019
Merged

install_symlink_p: Fix when dest includes a symlink#5793
MikeMcQuaid merged 1 commit intoHomebrew:masterfrom
sjackman:install_symlink_p

Conversation

@sjackman
Copy link
Copy Markdown
Contributor

@sjackman sjackman commented Feb 22, 2019

install_symlink_p does not work as intended when dst includes a symlink in its path.
relative_path_from requires that both src and dst be real paths without symlinks.

From https://ruby-doc.org/stdlib-2.3.7/libdoc/pathname/rdoc/Pathname.html#method-i-relative_path_from

This method doesn't access the filesystem. It assumes no symlinks.

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

@sjackman sjackman self-assigned this Feb 22, 2019
@sjackman sjackman requested a review from MikeMcQuaid February 22, 2019 21:22
@sjackman sjackman force-pushed the install_symlink_p branch 2 times, most recently from dfe3d5a to 39c6ec1 Compare February 22, 2019 22:31
@sjackman
Copy link
Copy Markdown
Contributor Author

I've marked this as do not merge because I'd like to test it some more first.

@MikeMcQuaid
Copy link
Copy Markdown
Member

Suggestion: write a test to reproduce the issue you saw.

@stale
Copy link
Copy Markdown

stale bot commented Mar 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Mar 16, 2019
@sjackman
Copy link
Copy Markdown
Contributor Author

I've been using this PR myself for a while, and I'm happy with it. I'll write a test.

@stale stale bot removed the stale No recent activity label Mar 16, 2019
install_symlink_p does not work as intended when dst includes a symlink in its path.
relative_path_from requires that both src and dst be real paths without symlinks.

From https://ruby-doc.org/stdlib-2.3.7/libdoc/pathname/rdoc/Pathname.html#method-i-relative_path_from
This method doesn't access the filesystem. It assumes no symlinks.
@sjackman sjackman force-pushed the install_symlink_p branch from 39c6ec1 to 5cb458d Compare March 17, 2019 06:12
@sjackman
Copy link
Copy Markdown
Contributor Author

I've added a test.

@MikeMcQuaid
Copy link
Copy Markdown
Member

Thanks again @sjackman!

@MikeMcQuaid MikeMcQuaid merged commit 587c8f0 into Homebrew:master Mar 17, 2019
@sjackman sjackman deleted the install_symlink_p branch March 17, 2019 17:45
@sjackman
Copy link
Copy Markdown
Contributor Author

Thanks for merging, Mike!
This PR fixed an issue for me when /usr/local/Cellar was a symlink to a directory found in my home directory. A non-standard unsupported setup to be sure, but revealed this bug.

@lock lock bot added the outdated PR was locked due to age label Apr 16, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants