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.

Sign up for GitHub

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

Jump to bottom

Implement conflicts_with :formula for casks #16374

Closed
wants to merge 1 commit into from
Closed

Implement conflicts_with :formula for casks #16374

wants to merge 1 commit into from

Conversation

Rylan12
Copy link
Member

@Rylan12 Rylan12 commented Dec 21, 2023

  • 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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Closes Homebrew/homebrew-cask#12822

This PR implements the conflicts_with formula: "..." DSL for casks by extracting the logic for handling formula conflicts and applying it to casks as well.

Interestingly, after I started working on this, I noticed some discussion in #16309 about this kind of thing. My reading of that conversation was that there might be interest in a more advanced type of conflict where instead of refusing to install, we simply don't link the conflicting files. For now, my implementation works the same way inter-casks and inter-formulae conflicts work (meaning it simply refuses to install). I figured I'd open this PR so we can discuss what the best way to handle this is.

CC @MikeMcQuaid, @cho-m, and @Homebrew/cask

MikeMcQuaid
MikeMcQuaid requested changes Dec 21, 2023
View reviewed changes
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Rylan12!

My reading of that conversation was that there might be interest in a more advanced type of conflict where instead of refusing to install, we simply don't link the conflicting files. For now, my implementation works the same way inter-casks and inter-formulae conflicts work (meaning it simply refuses to install).

Yes. conflicts_with is a terrible formula DSL. If I could: I'd get rid of it entirely except for somehow miraculous cases where two keg-only formulae conflict.

Instead: we should have the idea of a linking conflict (which we could/should bring to formualae too): these two packages cannot both be linked at the same time so when you install the latter: don't try to link it, output a warning on how to "flip" the linking and ensure that an upgrade of either doesn't cause the same issue again.

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 21, 2023

Gotcha, I think that makes sense. Is that something we'd want in addition to conflicts_with as it currently exists? Meaning, are there situations where even without linking we shouldn't install the formula? If not, should we just change the logic of conflicts_with or add a new DSL for that?

@MikeMcQuaid
Copy link
Member

Is that something we'd want in addition to conflicts_with as it currently exists?

Personally: I don't think so. I think if there's any conflicts_with that cannot be resolved by automatically handling linking: we should add something like conflicts_with :cannot_be_installed_side_by_side or similar for the current/existing behaviour. I'm open to opinions from other @Homebrew/maintainers though because I realise this change to conflicts_with would be a large one.

It may be less confusing for end-users to move to a new DSL entirely, though, like link_conflicts or something and plan on moving all of homebrew/core over to that (where possible) and only having link_conflicts for casks.

I'm open to doing things either ways. The only bits I feel strongly about:

  • casks conflict behaviours should avoid the pitfalls we have for formulae
  • ideally, we do this in a way that opens the door to doing this for formulae in future
  • if the implementation is not overly involved: let's add support to do this for formulae and casks at the same time

Thanks again @Rylan12!

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 22, 2023

I'd love to get more maintainer opinions on this. My current thinking is that sticking with conflicts_with is probably the easiest. Any estimates on the number of formulae for which installing but linking would not work? If that number isn't too high I feel like we shouldn't bother with a new DSL if we can avoid it.

@apainintheneck
Copy link
Contributor

I'm not super familiar with the linker code but after reading this thread it seems like 1) adding conflicts_with would be the simplest option here short-term and 2) we should open a new general issue to discuss moving to link_conflicts since it's not a trivial change.

The original issue has been open since 2015 so this doesn't seem super urgent. Maybe we could wait a bit after opening the link_conflicts thread to see if that would be a better option going forward.

@MikeMcQuaid
Copy link
Member

I do feel very strongly that:

  • to add conflicts_with behaviour that behaves like it does with formulae (i.e. blocking side-by-side installation) would be a huge mistake: we should not do this
  • to have conflicts_with behave differently in formulae and casks would be overly confusing
  • that this PR doesn't close any doors for future formula implementations

What is completely reasonable to me and not-at-all-blocking:

  • that this PR just deals with casks and doesn't touch formulae at all right now

I hope that helps! Thanks again @Rylan12 and sorry you've stumbled onto a contentious topic 😅

@Rylan12
Copy link
Member Author

Rylan12 commented Dec 25, 2023

Sorry, I wasn't clear. I planned to close this PR anyway but I wanted to wait to do so to see if anyone had any more thoughts. It sounds like probably not, so I'm going to close it now. I'll open a new issue that people might be more likely to look at with some options

@Rylan12 Rylan12 closed this Dec 25, 2023
@Rylan12 Rylan12 deleted the cask-conflicts-with-formula branch December 25, 2023 18:24
@Rylan12 Rylan12 mentioned this pull request Dec 25, 2023
Open
1 task
@MikeMcQuaid
Copy link
Member

@Rylan12 Ok, sorry and thanks!

@github-actions github-actions bot added the outdated PR was locked due to age label Jan 26, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@MikeMcQuaid MikeMcQuaid MikeMcQuaid requested changes

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implementing conflicts_with formula:
3 participants
@Rylan12 @MikeMcQuaid @apainintheneck