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

depends_on :formula should take install flags #8784

Closed
vitorgalvao opened this issue Jan 9, 2015 · 8 comments
Closed

depends_on :formula should take install flags #8784

vitorgalvao opened this issue Jan 9, 2015 · 8 comments

Comments

@vitorgalvao
Copy link
Member

Refs lisamelton/video-transcoding-scripts#8 (comment).

There is currently at least one case where it’d be useful (the referenced issue). Homebrew does it; could we perhaps leverage that?

Looking now at the relevant code. From what I gather, we’d need to both add the option to supply install flags and a way to check if the relevant formula was compiled with the relevant flags (since it currently only checks if it was installed at all).

@jawshooah
Copy link
Contributor

Checking whether it was compiled with the relevant flags is simple enough; we'd just need to parse the output of brew info formula or brew info --json=v1 formula.

A trickier question, I think, is how to support this in the DSL. At the moment, depends_on :formula can be given a string or an array. From here, there are a couple of ways we could incorporate install flags.

We could permit only one formula per depends_on clause, with an optional :options key that takes an array of strings. This would open the floor to further extension, such as minimum version requirements. For example:

depends_on :formula => 'mkvtoolnix'
depends_on :formula => 'ffmpeg',
           :version => '>= 2.5.2',
           :options => %w{ --with-faac --with-fdk-aac }

This method seems the most straightforward and readable to me. It is also the most consistent with Homebrew's depends_on syntax.

Alternatively, we could allow :formula to be an array of strings and/or hashes...

depends_on :formula => [
  { :name => 'mkvtoolnix' }, # or just 'mkvtoolnix'
  { :name => 'ffmpeg', :version => '>= 2.5.2', :options => %w{ --with-faac --with-fdk-aac } }
]

...or a hash of hashes...

depends_on :formula => {
  'mkvtoolnix' => {},
  'ffmpeg'     => { :version => '>= 2.5.2', :options => %w{ --with-faac --with-fdk-aac } }
}

I find the latter two options pretty clunky and unattractive. Are there any other alternatives you guys can think of? This was just a quick brainstorm.

@vitorgalvao
Copy link
Member Author

I agree the first one looks better. We’ve never been shy about repeating stanzas (app, binary, name), and most dependencies should be easy enough to declare (just the name — extra options being the exception). I’m definitely for it.

@jawshooah
Copy link
Contributor

Alright, so how about a stricter requirement, then? Each depends_on stanza may only contain one of:

  • :formula with optional :options (and any future args such as :version)
  • :cask with optional :options (and any future args such as :version)
  • :macos
  • :arch
  • :x11
  • :java

This reduces complexity by allowing us to assume exactly one dependency per depends_on stanza.

@vitorgalvao
Copy link
Member Author

I also see no problem with that. Most casks have only one depends_on anyway.

@rolandwalker
Copy link
Contributor

@jawshooah the purpose of defining DSL 1.0 (#4688) is to have a standard from which we will not remove anything. New functionality should be added in ways that add to the existing DSL, or leave the existing DSL unchanged.

Also, please note that this issue has already been assigned. We use assignments as a way to avoid duplicating efforts.

@jawshooah
Copy link
Contributor

@rolandwalker Right, but I can't find anything in the roadmap or docs specifying that a depends_on stanza should be allowed to contain multiple dependencies. Restricting it to one dependency per stanza would simplify the backend and better mirror Homebrew functionality. It also helps that only a few casks (<10 in the main repo, by my count) would need to change as a result.

And sorry about that, I didn't notice that @ndr-qef had already self-assigned this.

@tapeinosyne
Copy link
Contributor

We appreciate contributions. My self-assignment was largely meant as a warning not to start an implementation before I had the chance to comment, in the interest of avoiding preventable mistakes.

First, as @rolandwalker noted, it is unadvisable to break forms defined in the DSL v1; any change should be incremental. There is room for argument, partly because depends_on is recent and uncommon, but backward compatibility is a better baseline for discussion.

With regard to syntax, we should avoid nested forms. @rolandwalker in particular expressed concern over “creeping braces”, as exemplified by uninstall :script. That means that adding key-value pairs, as in @jawshooah's first proposal, is preferrable to more contrived alternatives.

More relevantly to the issue at hand, we cannot be cavalier about preexisting installations. Aborting is the most we can do if an installed formula fails to meet our requirements. Given that Homebrew formulae are latest-only, I would focus on supporting option flags, and postpone version boundaries.

@tapeinosyne tapeinosyne removed their assignment Jan 14, 2015
@rolandwalker rolandwalker self-assigned this Jan 14, 2015
@vitorgalvao vitorgalvao added the core Issue with Homebrew itself rather than with a specific cask. label Aug 12, 2015
@vitorgalvao
Copy link
Member Author

Closing in favour of #22825.

@miccal miccal removed core Issue with Homebrew itself rather than with a specific cask. enhancement labels 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

No branches or pull requests

5 participants