pathname: convert Cellar to opt paths before symlinking#11351
pathname: convert Cellar to opt paths before symlinking#11351carlocab wants to merge 1 commit intoHomebrew:masterfrom carlocab:opt-symlinks
Conversation
|
Review period will end on 2021-05-11 at 00:00:00 UTC. |
|
For After: |
Some formulae (e.g. `include-what-you-use`) install symlinks to a descendant of another formula's prefix in their install methods. Currently, `install_symlink` fully resolves symlinks, so using it to install such a symlink creates a reference to a Cellar path. This reference is invalidated with version or revision bumps to the referenced formula. Let's fix that by converting Cellar references into equivalent opt references before constructing the relative path. I'm hoping that the Cellar-to-opt conversion will prove useful elsewhere (e.g. `keg_relocate`), so I've created a separate method for it.
| sig { returns(Pathname) } | ||
| def to_opt | ||
| return self unless descend.include? HOMEBREW_CELLAR | ||
| return self if relative? |
There was a problem hiding this comment.
This line doesn't actually do anything (descend.include? is false whenever relative? is true). However, I think it makes what's happening clearer.
|
Review period skipped due to |
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Great work! Optional feedback.
| # No-op if {Pathname} is not a descendant of HOMEBREW_CELLAR | ||
| # or if {Pathname} is a relative path. | ||
| sig { returns(Pathname) } | ||
| def to_opt |
There was a problem hiding this comment.
Do you plan to use this in formulae? If so, I'd prefer not to have this extend Pathname. If not, can you make it private (and perhaps consider pulling it into utils)?
There was a problem hiding this comment.
No, there's no reason to use it in formulae, I think. Let me look at moving it/making it private.
|
@carlocab I'm good for this to be merged if/when you are? |
|
There are still two issues:
As an illustration of the second issue, suppose I have the following in an install method: The existing behaviour would be to create the following symlink in With this change, we would have that symlink look something like It doesn't hurt, but it's bound to confuse some people looking at it. |
Given we generally have decent support for side-by-side versioned kegs (although this has somewhat faded over the years on the CLI side), I think it is indeed ideal to keep versioned Cellar references when used in the same keg. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?Some formulae (e.g.
include-what-you-use) install symlinks to adescendant of another formula's prefix in their install methods.
Currently,
install_symlinkfully resolves symlinks, so using it toinstall such a symlink creates a reference to a Cellar path. This
reference is invalidated with version or revision bumps to the
referenced formula.
Let's fix that by converting Cellar references into equivalent opt
references before constructing the relative path.
I'm hoping that the Cellar-to-opt conversion will prove useful elsewhere
(e.g.
keg_relocate), so I've created a separate method for it.CC @sjackman (Ref. #5793)