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

Implement depends_on :cask #8491

Merged
merged 5 commits into from
Dec 30, 2014
Merged

Implement depends_on :cask #8491

merged 5 commits into from
Dec 30, 2014

Conversation

tapeinosyne
Copy link
Contributor

This PR includes library and (some) test code for hard Cask dependencies, i.e. Casks which must always be installed prior to the depender.

There are some warts. Most notably:

  • An invasive optional argument, skip_cask_deps, is passed down from Cask::Installer.install to satisfy_dependencies, in order to avoid redundant loading and (re)sorting of Cask subdependencies.

Secondarily:

  • TopologicalHash, a helper class for dependency sorting, should not be in the top namespace. We do have extend, but it is reserved for monkeypatches.
  • CaskDependencies should also not be in the top namespace, being a specialized class.

ndr added 4 commits December 29, 2014 16:31
In the interest of `depends_on :cask`, we introduce a topologically
sortable hashmap, implemented as a small subclass of Hash.
as a class concerned with recursively loading, graphing and sorting all
dependencies of a given Cask.
Includes an unsavory hack to bypass redundant checks for Cask
subdependencies, in the form of an optional argument passed down from
`install` to `satisfy_dependencies`.
@tapeinosyne tapeinosyne mentioned this pull request Dec 29, 2014
5 tasks
@tapeinosyne tapeinosyne added the core Issue with Homebrew itself rather than with a specific cask. label Dec 29, 2014
@tapeinosyne tapeinosyne changed the title Implement depends_on :casks Implement depends_on :cask Dec 29, 2014
@rolandwalker
Copy link
Contributor

Warts or not, it is a much-wanted feature, and looks mergeable.

My only question after reading is why everything happens in the constructor. (Not that it matters in practice, because you read the dependencies in the next line after instantiation.)

@tapeinosyne
Copy link
Contributor Author

No reason other than overlap and utility. A naked array of first-level dependencies is already available at the DSL level as depends_on.cask, and CaskDependencies serves no purpose beyond sorting. (Perhaps the name is insufficiently descriptive of the current implementation.)

If desired, I have no qualm with refactoring the class into a more conventional structure.

@rolandwalker
Copy link
Contributor

Just curious, no need to bother with refactoring.

The name is also perfectly descriptive, if you think of a CaskDependencies instance as a collection. Purely for entertainment value, it is worth noting that one can make it act more like a collection by include Enumerable or just providing an each method, after which

deps.sorted.each do |dep_token|

could be spelled without the sorted

deps.each do |dep_token|

rolandwalker added a commit that referenced this pull request Dec 30, 2014
@rolandwalker rolandwalker merged commit 2c847c5 into Homebrew:master Dec 30, 2014
@miccal miccal removed the core Issue with Homebrew itself rather than with a specific cask. label Dec 23, 2016
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants