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

keg: create symlinks in opt for formula aliases #1192

Merged
merged 4 commits into from Feb 20, 2017

Conversation

Projects
None yet
6 participants
@ilovezfs
Copy link
Contributor

ilovezfs commented Sep 30, 2016

No description provided.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 30, 2016

based on that test run, it sure looks like it will need to be bootstrapped with the existing ones rebuilt against the versioned name

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 30, 2016

CC @Homebrew/maintainers for thoughts on this change and @penman too.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 30, 2016

Could someone write up the rationale behind this change? I see discussions in team chat but not everyone read transcripts after the fact, and @alyssais (looks like the handle changed) doesn't have access to that.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 30, 2016

The existing bottles are linked against opt/boost not opt/boost@1.61. So renaming boost to boost@1.61 breaks them. The alternatives are not to rename boost to boost@1.61 and just to go from boost to boost@1.62 (that pattern is already underway with openssl), or have duplicate boost and boost@1.61 formulae alongside boost@1.62, until dependents are transferred from boost to boost@1.61 or boost@1.62, and then delete the duplicate boost formula, and create a boost alias instead.

@zmwangx

This comment has been minimized.

Copy link
Contributor

zmwangx commented Sep 30, 2016

Do we want to limit opt linking for aliases to @ formulae only? Having both cowthink and cowsay in opt is pointless (but harmless either).

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Sep 30, 2016

I like the idea that there's nothing special about @ formulae (that they're just normal aliases), but that could definitely end up not being practical.

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 30, 2016

@alyssais I agree with you. I think we'll want a full-on DSL for this stuff at some point relatively soon hehe

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Sep 30, 2016

I think this looks good. Is it cool if I make a couple of comments on the code?

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Sep 30, 2016

of course!

based on that brew tests run, I'm 👎 on my own PR though :)

@alyssais

This comment has been minimized.

Copy link
Contributor

alyssais commented Sep 30, 2016

based on that brew tests run, I'm 👎 on my own PR though :)

Ah, sure. I'll wait a bit until that's resolved then. :)

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 30, 2016

I like the idea that there's nothing special about @ formulae (that they're just normal aliases), but that could definitely end up not being practical.

I agree with this.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 30, 2016

The alternatives are not to rename boost to boost@1.61 and just to go from boost to boost@1.62 (that pattern is already underway with openssl)

This would be my recommendation, FWIW.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 30, 2016

Although we need to resolve the issues around keg_only and so on either way with Boost, especially if my comments on boost and boost-python last night prove accurately remembered. It didn't matter for openssl and openssl@1.1 because both are always going to be keg_only when Homebrew is located in /usr/local.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Sep 30, 2016

The alternatives are not to rename boost to boost@1.61 and just to go from boost to boost@1.62 (that pattern is already underway with openssl)

This would be my recommendation, FWIW.

Even if that's the case I think this alias change would be a good one.

@DomT4

This comment has been minimized.

Copy link
Contributor

DomT4 commented Sep 30, 2016

Yes, I have no issues with aliases also being in opt pointing to the "true" source of the alias, even for non-@ formulae. Might be a good idea to add some tests, if possible, for this change though to ensure we don't bork it in future somehow.

@@ -197,6 +197,11 @@ def optlinked?

def remove_opt_record
opt_record.unlink
unless aliases.empty?
aliases.each do |a|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 1, 2016

Member

Can do the each even if empty? as it'll be a no-op.

result = aliases_path.children.select do |c|
c.symlink? && c.readlink.basename(".rb").to_s == formula_name
end
result.map(&:basename).map(&:to_s)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 1, 2016

Member

Can get this from Formula#aliases (which also handles the no-tap case)

def optlink(mode = OpenStruct.new)
opt_record.delete if opt_record.symlink? || opt_record.exist?
make_relative_symlink(opt_record, path, mode)
unless aliases.empty?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 1, 2016

Member

Again, aliases.each is a no-op if this is empty so you can just use .each without the check.

unless aliases.empty?
aliases.each do |a|
alias_opt_record = opt_record.parent/a
alias_opt_record.delete if alias_opt_record.symlink? || alias_opt_record.exist?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 1, 2016

Member

Will a symlink? not always exist??

This comment has been minimized.

@ilovezfs

ilovezfs Oct 3, 2016

Contributor

Broken symlinks return false for exist?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Oct 3, 2016

Member

TIL.

@ilovezfs ilovezfs closed this Oct 1, 2016

@BrewTestBot BrewTestBot removed the in progress label Oct 1, 2016

@MikeMcQuaid MikeMcQuaid reopened this Oct 1, 2016

@MikeMcQuaid MikeMcQuaid referenced this pull request Oct 3, 2016

Merged

Migrate a Formula to Multiple Versions: add docs. #1204

5 of 5 tasks complete
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Oct 3, 2016

@MikeMcQuaid I'm not sure it matters but do you prefer opt/boost -> boost@1.62 or opt/boost -> ../Cellar/boost@1.62?

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 3, 2016

@ilovezfs Then latter just in case people are relying on being able to follow one layer of symlinks to find the Cellar 👍

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 3, 2016

There's some test failures but code and local testing look good to me.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Oct 15, 2016

@ilovezfs Thoughts on this? Shout if you want help with test failures.

ilovezfs and others added some commits Oct 3, 2016

have opt alias symlink point into the Cellar
in case people expect to be able to find the prefix by only resolving
the symlink once (e.g., if they're using readlink not realpath)

@MikeMcQuaid MikeMcQuaid force-pushed the ilovezfs:optlink_aliases branch from 38e525e to dfa2c24 Feb 20, 2017

@MikeMcQuaid MikeMcQuaid merged commit 60ba0e4 into Homebrew:master Feb 20, 2017

3 checks passed

codecov/patch 62.5% of diff hit (target 55.31%)
Details
codecov/project 55.38% (+0.06%) compared to afb66d0
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Feb 21, 2017

@MikeMcQuaid this has introduced a bug for anything that has an alias but was installed before this PR shipped:

iMac-TMP:~ joe$ brew uninstall ag
Uninstalling /usr/local/Cellar/the_silver_searcher/1.0.2... (10 files, 113.1K)
Error: No such file or directory - /usr/local/opt/ag
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 21, 2017

@ilovezfs Working on it.

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Feb 21, 2017

keg: handle missing alias opt link on uninstall.
Fixes an issue introduced in Homebrew#1192 where there would be a failure if the
alias link didn't exist on removal (which would be the case for anything
with an alias installed before Homebrew#1192 was merged).
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 21, 2017

@ilovezfs Opened #2085 to address this.

@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.