-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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 conflicts_with
stanza
#9319
Conversation
:x86_64, | ||
:ppc_7400, | ||
:ppc_64, | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we support machines running PPC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this so far @jawshooah - thank you! Left you plenty of review notes - happy to clarify and discuss any of 'em. 👍 |
Thanks for the speedy response! I love a good code review. As noted in the opening comment, most of the changes here (including much of what you've commented on 😉) were taken directly from |
If you're willing, that'd be great! Though I'd probably approach that as a separate "refactor depends_on" PR to keep things from getting too hefty. |
I recommend against this degree of duplication of code. |
@rolandwalker Agreed, I'm just not entirely sure of the best way to go about deduplicating it. It seems like every class in |
One obvious thing that would help would be to move |
Ah yeah I didn't see the amount of similarity this had with depends_on in my initial review. I think |
@phinze I've moved all shared logic to The |
Turns out it wasn't so unrelated; the |
Procedurally: While we are pretty darn good about writing roadmaps in this project, it is not practical to get 100% of our private branches, thoughts, and IRC discussions into GitHub issues. So, there are some things about implementing When the idea of implementing Substantively:
Roughly, the path leading to
Currently, the folks interested in dependencies are discussing whether to add interface to support pure meta-Casks (#8492). It is worth noting that there is a divergence of opinions about whether or not to support meta-Casks, and what the interface might be. The maintainers are responding to that by taking their time, being consultative, going to extra effort to collect opposite views. It's a tautology, but: the only way we can work together is by coordinating, asking questions, and listening. Scanning this PR, conceptual flaws are apparent, such as the notion that one can serially Furthermore, it is too large. |
These changes are almost entirely taken from depends_on.rb. Should probably consider moving common methods to a superclass or utility class.
Test conflicts_with :cask, :macos, :arch, and :x11. Add todo for testing :formula.
Create class Hbc::DSL::Dependency to serve as superclass for ConflictsWith and DependsOn. Move dependency and conflict validation and resolution logic to Hbc::Dependencies and its subclasses.
Eradicate duplicate logic in Hbc::DSL::DependsOn and make it a subclass of Hbc::DSL::Dependency.
Ahhhh ok! Thanks for laying out the full context so clearly @rolandwalker. I was running behind on the overall conversation here, and I dove into line-by-line review before I should have. Sorry to steer you in the wrong direction @jawshooah! Sounds like we're going to need to back up a little bit on this feature: resolve the direction in @rolandwalker's linked conversations, then break down the pieces so we can avoid any big-bang PRs. Thanks for diving into this @jawshooah - once I'm a little more caught back up I promise I won't lead you waste work like this. 😕 We may not merge this work as is, but it definitely helps to inform the progression of the feature as a whole, so thanks for that! 👍 |
Thanks to @rolandwalker for context, clarification, and critique. @phinze, don't worry about it. I should have asked around more before diving headfirst into the implementation. Is there currently an open discussion around implementing a more generalized dependency graph? It seems like resolution would proceed as follows:
Obviously this is going to take some deeper thinking, but that's the gist as I understand it. We will need to interface with Homebrew to get Homebrew also has a much longer list of dependency types. We may need to either restore some of We'll also need to hardcode in relationships between This is going to be quite an endeavor. |
Very nice catch! I intentionally deleted that bullet point, in order not to muddy the waters, and added the weasel-word "roughly" to cover my behind. It is probably best not to try to resolve Formula dependencies, and just pretend that it works until we encounter breakage. It's too much work!
And then, if Almost nothing is "expressed well" at the moment, because of the transition phase of #4688. In large part, git history would show that I took @phinze's designs and subjected them to … bending and denting. It's a design lesson! Good design will survive the next guy. |
Ignoring Formula dependencies makes me feel queasy, but it probably won't cause many problems in practice, and I agree that the effort required to handle them properly is probably too much, at least for an initial implementation. Though this first attempt was a bust, I'm still very eager to contribute! Is there any way that I can help to bring proper dependency resolution to fruition? I don't want to step on any toes, but there must be something I can hack away at. I care a lot about this project, and I'm happy to lend a hand wherever needed. |
@rolandwalker Is anyone currently working on a more comprehensive solution for dependency resolution as you described? I wasn't sure whether your response to this PR was an invitation to give it another, better-conceived shot or a polite request to let the big boys handle this one 😉 |
Closes #4497.
As mentioned in the commit message for 46738c9, most of
conflicts_with.rb
was taken directly fromdepends_on.rb
and modified accordingly. It would be likely be better to outsource common methods to a shared superclass or utility class.