Allow to install any spec #1343

vladshablinsky · 2016-10-21T23:38:34Z

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 tests with your changes locally?

This is a possible fix for install part of #1162 and possible replacement for #1296.
For now it has some complicated if statements that should probably be replaced and some feedback on how it can be achieved is appreciated.

The behaviour for install should be the following:

installing HEAD keg_only should be possible only if:
- Old version installed differs from new one and optlinked
- --force flag is passed
- HEAD is seriously outdated or outdated with --fetch-HEAD
OR
- Old version installed differs from new one and not optlinked
OR
- This formula is not installed
installing keg_only should be possible only if:
- Old version installed differs from new one and optlinked
- --force flag is passed
OR
- Old version installed differs from new one and not optlinked
OR
- This formula is not installed
installing HEAD should be possible only if:
- Old HEAD is seriously outdated or outdated with --fetch-HEAD
OR
- HEAD is not installed
installing stable or devel should be possible only if:
- Old version installed differs from new one

Here is an example of how it looks like:

➜ vlad:Homebrew$ brew info smpeg2                                   explicit-specs ✗
smpeg2: stable 2.0.0 (bottled), HEAD
SDL MPEG Player Library
https://icculus.org/smpeg/
/usr/local/Cellar/smpeg2/2.0.0 (23 files, 613.9K)
  Poured from bottle on 2016-10-22 at 01:45:27
/usr/local/Cellar/smpeg2/HEAD-411 (23 files, 596.4K) *
  Built from source on 2016-07-27 at 18:48:29
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/smpeg2.rb
==> Dependencies
Build: autoconf ✔, automake ✔, libtool ✔, pkg-config ✔
Required: sdl2 ✔
➜ vlad:Homebrew$ brew install smpeg2 --HEAD                         explicit-specs ✗
Warning: smpeg2-HEAD-411 already installed
➜ vlad:Homebrew$ brew install smpeg2                                explicit-specs ✗
Warning: smpeg2-2.0.0 already installed, however linked version is HEAD-411.
You can use `brew switch smpeg2 2.0.0` to link this version.
➜ vlad:Homebrew$ brew install smpeg2 --HEAD --fetch-HEAD            explicit-specs ✗
Error: smpeg2-HEAD-411 already installed
To install this version, first `brew unlink smpeg2`
➜ vlad:Homebrew$ brew unlink smpeg2                                 explicit-specs ✗
Unlinking /usr/local/Cellar/smpeg2/HEAD-411... 8 symlinks removed
➜ vlad:Homebrew$ brew install smpeg2 --HEAD --fetch-HEAD            explicit-specs ✗
==> Using the sandbox
==> Cloning svn://svn.icculus.org/smpeg/trunk
Updating /Users/vlad/Library/Caches/Homebrew/smpeg2--svn-HEAD
==> ./autogen.sh
==> ./configure --prefix=/usr/local/Cellar/smpeg2/HEAD-412 --with-sdl-prefix=/usr/loc
==> make
==> make install
🍺  /usr/local/Cellar/smpeg2/HEAD-412: 24 files, 613.7K, built in 27 seconds

CC @MikeMcQuaid @ilovezfs

ilovezfs · 2016-10-22T05:23:36Z

What is the command line to install stable if devel or HEAD is already installed?

vladshablinsky · 2016-10-22T10:37:42Z

What is the command line to install stable if devel or HEAD is already installed?

Just brew install.

MikeMcQuaid · 2016-10-22T13:13:10Z

Library/Homebrew/cmd/install.rb

-        current = f if f.installed?
-        current ||= f.old_installed_formulae.first
+        if f.keg_only? && !f.installed_prefixes.empty? && f.opt_prefix.symlink? && !ARGV.force?
+          # keg-only install is only possible when no ohter version is


ohter -> other

MikeMcQuaid · 2016-10-22T13:13:34Z

Library/Homebrew/cmd/install.rb

-        current ||= f.old_installed_formulae.first
+        if f.keg_only? && !f.installed_prefixes.empty? && f.opt_prefix.symlink? && !ARGV.force?
+          # keg-only install is only possible when no ohter version is
+          # optlinked, because silent install can break dependencies. Therefore


optlinked -> linked to opt. Not sure I understand what "silent install" means.

Will replace "silent install" with install without any warnings.

MikeMcQuaid · 2016-10-22T13:14:08Z

Library/Homebrew/cmd/install.rb

+          # optlinked, because silent install can break dependencies. Therefore
+          # before performing other checks we need to be sure --force flag is
+          # passed.
+          opoo "#{f.full_name} is a keg-only and another version is optlinked."


optlinked ->linked to opt

MikeMcQuaid · 2016-10-22T13:14:40Z

Library/Homebrew/cmd/install.rb

+          puts "Use `brew install --force` if you want to install this version"
+        elsif (ARGV.build_head? && (version = f.latest_head_version) &&
+            !f.head_version_outdated?(version, fetch_head: ARGV.fetch_head?)) ||
+              (f.prefix.exist? && !f.prefix.children.empty?)


Split some/all of these conditionals into variables. Similarly consider doing so for the if too.

MikeMcQuaid · 2016-10-22T13:15:08Z

Library/Homebrew/cmd/install.rb

+          end
+
+          msg = "#{f.full_name}-#{installed_version} already installed"
+          if f.linked_keg.symlink? && f.linked_keg.resolved_path != f.prefix(installed_version)


f.linked_keg.resolved_path != f.prefix(installed_version) could be a helper method or just variable.

MikeMcQuaid · 2016-10-22T13:15:35Z

Library/Homebrew/download_strategy.rb

+      else
+        version.update_commit(commit)
+        false
+      end


What's this doing, out of interest?

This tries to shrink the commit for the version if it's possible. When we pass the commit to this method, we can fetch a full-length commit and overwrite short one with full-length for the formula. If there is no multiple short commits, there is no need to keep it full-length and we update it with initial one.

commit_outdated? is called here: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/formula.rb#L522

Cool, works for me.

MikeMcQuaid · 2016-10-22T13:16:44Z

Liking where this is going, nice work @vladshablinsky!

vladshablinsky · 2016-11-01T01:05:30Z

@MikeMcQuaid is it getting better? Still needs some fixes in documentation and I guess tests, but will go for it when we're fine with the behaviour and the code.

MikeMcQuaid

A few more changes but looking good 👍

MikeMcQuaid · 2016-11-05T19:33:23Z

Library/Homebrew/cmd/install.rb

+          # sure --force flag is passed.
+          opoo "#{f.full_name} is a keg-only and another version is linked to opt."
+          puts "Use `brew install --force` if you want to install this version"
+        elsif ARGV.build_head? && new_head_installed || prefix_installed


split new_head_installed || prefix_installed into another variable because the parity is ambiguous.

Actually it's (ARGV.build_head? && new_head_installed) || prefix_installed here.

Heh, see! 😀

MikeMcQuaid · 2016-11-05T19:34:37Z

Library/Homebrew/formula.rb

+
+  # Is formula's linked keg points to the prefix.
+  def prefix_linked?(v = pkg_version)
+    linked? && linked_keg.resolved_path == prefix(v)


return false unless linked?

MikeMcQuaid · 2016-11-05T19:35:00Z

Library/Homebrew/cmd/install.rb

+          end
+        elsif !f.any_version_installed? && old_formula = f.old_installed_formulae.first
+          msg = "#{old_formula.full_name}-#{old_formula.installed_version} already installed"
+          unless old_formula.linked? || old_formula.keg_only?


Use if here because unless || hurts my brain

👍 OK. The reason I didn't change it is that unless had been here before any change was made.

Yeh, not your issue but may as well clean it up while you're refactoring the other bits 👍

MikeMcQuaid · 2016-11-05T19:35:19Z

Library/Homebrew/cmd/install.rb

+          end
+
+          msg = "#{f.full_name}-#{installed_version} already installed"
+          if f.linked? && f.linked_version != installed_version


Could make f.linked_version != installed_version another variable for clarity but feel free to ignore that.

MikeMcQuaid · 2016-11-05T19:35:34Z

Library/Homebrew/cmd/install.rb

-          msg = "#{current.full_name}-#{current.installed_version} already installed"
-          unless current.linked_keg.symlink? || current.keg_only?
-            msg << ", it's just not linked"
+        prefix_installed = (f.prefix.exist? && !f.prefix.children.empty?)


Can probably skip parentheses here.

MikeMcQuaid · 2016-11-12T10:46:03Z

@vladshablinsky Any news here? Thanks!

vladshablinsky · 2016-11-12T16:22:36Z

Sorry for absence during last week, left some comments and updated the code.

MikeMcQuaid · 2016-11-13T10:52:11Z

Sorry for absence during last week, left some comments and updated the code.

@vladshablinsky No need to apologise or be Homebrewing every week. Just checking in 😁

vladshablinsky · 2016-11-15T07:56:05Z

It's probably set for adding tests now.

MikeMcQuaid · 2016-11-15T09:18:59Z

@vladshablinsky Agreed, code looks good. If you could add some tests and also detail how you're testing this manually (so I can do so myself too). Thanks!

vladshablinsky · 2016-11-17T22:32:29Z

@MikeMcQuaid

Will add tests later this week.

As for manual testing -- I was patching ack in different places to see how it worked. It can be done easier, I think. I edited version and version under devel. For manual testing HEAD's I changed url so that it pointed to my local ack repo, but just renaming HEAD_<something> to HEAD_<something else> under Cellar seems to do the thing too.

MikeMcQuaid · 2016-11-18T08:29:38Z

As for manual testing -- I was patching ack in different places to see how it worked. It can be done easier, I think. I edited version and version under devel. For manual testing HEAD's I changed url so that it pointed to my local ack repo, but just renaming HEAD_ to HEAD_ under Cellar seems to do the thing too.

Great, thanks.

* `Formula#linked?` returns true if formula linked * `Formula#optlinked?` returns true if formula linked to opt formula installed to the Cellar * `Formula#prefix_linked?` returns true if linked keg points to the prefix passed as an argument * `Formula#linked_version` to get linked version of the formula

* installing HEAD keg_only should be possible only if: 1. - Old version installed differs from new one and optlinked - `--force` flag is passed - HEAD is seriously outdated or outdated with `--fetch-HEAD` or 2. - Old version installed differs from new one and not optlinked or 3. - This formula is not installed * installing keg_only should be possible only if: 1. - Old version installed differs from new one and optlinked - `--force` flag is passed or 2. - Old version installed differs from new one and not optlinked or 3. - This formula is not installed * installing HEAD should be possible only if: 1. - Old HEAD is seriously outdated or outdated with `--fetch-HEAD` or 2. - HEAD is not installed * installing stable or devel should be possible only if: - Old version installed differs from new one

vladshablinsky · 2016-12-03T02:34:58Z

@MikeMcQuaid seems like it's almost ready, however El Capitan caks tests fail, what should I do?
@ilovezfs feel free to try. CC @alyssais.

MikeMcQuaid · 2016-12-03T15:02:31Z

@vladshablinsky I reckon just a random failure. Have requeued.

MikeMcQuaid · 2016-12-03T15:03:08Z

Library/Homebrew/test/install_test.rb

+      end
+    EOS
+
+    # Ignore dependincies, because we'll try to resolve requirements in build.rb


dependincies

vladshablinsky · 2016-12-05T00:57:17Z

Will merge as soon as I am able to be around 4 hours after the merge.

vladshablinsky added do not merge features New features in progress Maintainers are working on this discussion Input solicited from others labels Oct 21, 2016

vladshablinsky mentioned this pull request Oct 21, 2016

Unable to install devel if stable installed. #1162

Closed

2 tasks

vladshablinsky removed the do not merge label Oct 21, 2016

vladshablinsky force-pushed the explicit-specs branch from 6ed9ca4 to a23b700 Compare October 22, 2016 01:08

MikeMcQuaid requested changes Oct 22, 2016

View reviewed changes

vladshablinsky force-pushed the explicit-specs branch 3 times, most recently from 3fd1461 to 9354579 Compare November 1, 2016 01:02

vladshablinsky force-pushed the explicit-specs branch from 9354579 to 1a5c1a8 Compare November 1, 2016 01:16

MikeMcQuaid requested changes Nov 5, 2016

View reviewed changes

vladshablinsky force-pushed the explicit-specs branch from 1a5c1a8 to ca57c2c Compare November 12, 2016 16:20

vladshablinsky mentioned this pull request Nov 12, 2016

cmd/install: improve logic for following aliases #1296

Closed

5 tasks

vladshablinsky force-pushed the explicit-specs branch 2 times, most recently from ae91ccb to d52a36c Compare November 15, 2016 07:41

vladshablinsky force-pushed the explicit-specs branch 2 times, most recently from 57ebda4 to 3c0ebb9 Compare November 27, 2016 02:33

vladshablinsky force-pushed the explicit-specs branch from 3c0ebb9 to a37334a Compare November 27, 2016 02:42

antonakv mentioned this pull request Nov 30, 2016

error: the requested upstream branch 'antonakv/cask-repair_update-deadbeef' does not exist vitorgalvao/tiny-scripts#52

Closed

vladshablinsky added 4 commits December 3, 2016 04:42

download_strategy: keep commit short if possible

7b24d1d

Update tests for new install behaviour

6ef96d0

vladshablinsky force-pushed the explicit-specs branch from a37334a to ae80110 Compare December 3, 2016 01:45

MikeMcQuaid added the CI-requeued label Dec 3, 2016

MikeMcQuaid approved these changes Dec 3, 2016

View reviewed changes

MikeMcQuaid reviewed Dec 3, 2016

View reviewed changes

install_test: add HEAD tests

a24a919

vladshablinsky force-pushed the explicit-specs branch from ae80110 to a24a919 Compare December 3, 2016 15:55

vladshablinsky merged commit 45ef7ea into Homebrew:master Dec 11, 2016

BrewTestBot removed the in progress Maintainers are working on this label Dec 11, 2016

vladshablinsky deleted the explicit-specs branch December 13, 2016 15:18

Homebrew locked and limited conversation to collaborators May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to install any spec #1343

Allow to install any spec #1343

vladshablinsky commented Oct 21, 2016 •

edited

Loading

ilovezfs commented Oct 22, 2016

vladshablinsky commented Oct 22, 2016 •

edited

Loading

MikeMcQuaid Oct 22, 2016

MikeMcQuaid Oct 22, 2016

vladshablinsky Oct 24, 2016

MikeMcQuaid Oct 22, 2016

MikeMcQuaid Oct 22, 2016

MikeMcQuaid Oct 22, 2016

MikeMcQuaid Oct 22, 2016

vladshablinsky Oct 24, 2016 •

edited

Loading

MikeMcQuaid Nov 13, 2016

MikeMcQuaid commented Oct 22, 2016

vladshablinsky commented Nov 1, 2016

MikeMcQuaid left a comment

MikeMcQuaid Nov 5, 2016

vladshablinsky Nov 12, 2016

MikeMcQuaid Nov 13, 2016

MikeMcQuaid Nov 5, 2016

MikeMcQuaid Nov 5, 2016

vladshablinsky Nov 6, 2016

MikeMcQuaid Nov 7, 2016

MikeMcQuaid Nov 5, 2016

MikeMcQuaid Nov 5, 2016

MikeMcQuaid commented Nov 12, 2016

vladshablinsky commented Nov 12, 2016

MikeMcQuaid commented Nov 13, 2016

vladshablinsky commented Nov 15, 2016

MikeMcQuaid commented Nov 15, 2016

vladshablinsky commented Nov 17, 2016 •

edited

Loading

MikeMcQuaid commented Nov 18, 2016

vladshablinsky commented Dec 3, 2016

MikeMcQuaid commented Dec 3, 2016

MikeMcQuaid Dec 3, 2016

vladshablinsky commented Dec 5, 2016 •

edited

Loading

Allow to install any spec #1343

Allow to install any spec #1343

Conversation

vladshablinsky commented Oct 21, 2016 • edited Loading

ilovezfs commented Oct 22, 2016

vladshablinsky commented Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladshablinsky Oct 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Oct 22, 2016

vladshablinsky commented Nov 1, 2016

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Nov 12, 2016

vladshablinsky commented Nov 12, 2016

MikeMcQuaid commented Nov 13, 2016

vladshablinsky commented Nov 15, 2016

MikeMcQuaid commented Nov 15, 2016

vladshablinsky commented Nov 17, 2016 • edited Loading

MikeMcQuaid commented Nov 18, 2016

vladshablinsky commented Dec 3, 2016

MikeMcQuaid commented Dec 3, 2016

Choose a reason for hiding this comment

vladshablinsky commented Dec 5, 2016 • edited Loading

vladshablinsky commented Oct 21, 2016 •

edited

Loading

vladshablinsky commented Oct 22, 2016 •

edited

Loading

vladshablinsky Oct 24, 2016 •

edited

Loading

vladshablinsky commented Nov 17, 2016 •

edited

Loading

vladshablinsky commented Dec 5, 2016 •

edited

Loading