Skip to content
This repository has been archived by the owner on Jan 16, 2025. It is now read-only.

Collect and expose all collected dependencies (core change) #49361

Closed
wants to merge 1 commit into from
Closed

Collect and expose all collected dependencies (core change) #49361

wants to merge 1 commit into from

Conversation

arnested
Copy link
Contributor

This adds a new class method @@collected_dependencies to
DependencyCollector and exposes them via a class method
collected_dependencies.

We need this change for the Homebrew PHP tap. See Homebrew/homebrew-php#2889.

The PHP tap has a PhpMetaRequirement that works as a "just depend on any of the php versions"-dependency.

Unfortunately it currently can only work flawlessly if one of the php versions has already been installed and linked. If no PHP version has been installed yet it will fallback to default_package php56.

This is a problem for packages like phan which depends on php70. Since no php version is installed PhpMetaRequirement will fallback to php56. And since two different php versions can't be linked at the same time the install will fail. This happens very when @BrewTestBot is testing formulas in the PHP tap.

To work around this we need to know all the dependencies collected by DependencyCollector.

See Homebrew/homebrew-php#2889 for the change in Homebrew-PHP that will take advantage of this feature and the discussion around it.

All Submissions:

  • Have you followed the guidelines in our Contributing document?

Changes to Homebrew's Core:

  • 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 core changes, as applicable? Here's an example if you'd like one.
  • Have you successfully ran brew tests with your changes locally?

This adds a new class method `@@collected_dependencies` to
`DependencyCollector` and exposes them via a class method
`collected_dependencies`.
@MikeMcQuaid
Copy link
Member

I'm not opposed to making core changes for requirements to work better but this is exposing an internal implementation detail as a public API so we can't accept it as-is.

@arnested
Copy link
Contributor Author

Exposing internal implementation details is not desirable. I understand and agree.

But to understand this a bit better could you please elaborate on whether you consider the concept of exposing all dependencies during an install an implementation detail? Or is it the way I implemented it you consider an implementation detail?

In other words could we work out a way to expose the list of dependencies that would be abstracted away from the implementation details?

I don't think it would be unreasonable for a Requirement class to know which packages are going to be installed. But my view might be biased 😄

I am willing to keep working on this if a solution can be found. I will be offline for the next 2.5 weeks though.

@MikeMcQuaid
Copy link
Member

But to understand this a bit better could you please elaborate on whether you consider the concept of exposing all dependencies during an install an implementation detail?

I consider that an implementation detail.

I don't think it would be unreasonable for a Requirement class to know which packages are going to be installed. But my view might be biased

I don't think it should know outside of its own Requirement. Let's talk more generally instead: can you describe what you're trying to do and what the problems are with Homebrew's current code?

@ablyler
Copy link
Contributor

ablyler commented Feb 24, 2016

@MikeMcQuaid the situation we are trying to solve is:

We have a formula in homebrew-php named phan. This formula depends on the composer module, which includes a class PhpMetaRequirement which is a child of Requirement. This class ensures that one of the phpXX formulas are installed. In this case, if none are installed it will pick php56 by default. The issue is, that the phan formula then also adds php70 as a dependency, since it only works with that version.

We are looking for a way to have homebrew resolve this dependency tree conflict, since php56 and php70 can't be linked at the same time.

One solution might be to have a global variable that is the default PHP version that can be modified by the formula. So that the conflict doesn't happen in the first place.

What are your thoughts?

@ablyler
Copy link
Contributor

ablyler commented Feb 24, 2016

@MikeMcQuaid I took a stab at solving this purely in homebrew-php land: https://github.com/Homebrew/homebrew-php/pull/2896 let me know what you think of the approach.

@MikeMcQuaid
Copy link
Member

@ablyler Your approach seems fine. It seems to me the solution might be something like making the requirement :recommended and depending on without-requirement. As it seems to be workable in PHP-land: closing this.

@arnested arnested deleted the dependency_collector_collect_all_dependencies branch March 8, 2016 21:20
@arnested
Copy link
Contributor Author

arnested commented Mar 8, 2016

I am back from vacation and happy to see you found a good solution! 👍

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
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