Skip to content

Conversation

@tresf
Copy link
Contributor

@tresf tresf commented Aug 29, 2018

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Adds Carla, an audio plugin host for several audio plugin formats (LV2, VST, etc).

Also adds pkg-config support, libraries and headers for projects building against the carla library.

Edit: Despite the similarities to a cask, this package is intended to provide as a pkg-config driven build library for C++ projects, etc.

@BrewTestBot
Copy link
Contributor

  • C: 10: col 3: dependency "pkg-config" (line 10) should be put before dependency "macos" (line 9)
  • C: 12: col 3: dependency "pyqt" (line 12) should be put before dependency "python" (line 11)
  • C: 13: col 3: dependency "libmagic" (line 13) should be put before dependency "python" (line 11)
  • C: 14: col 3: dependency "liblo" (line 14) should be put before dependency "python" (line 11)
  • C: 15: col 3: dependency "fluid-synth" (line 15) should be put before dependency "python" (line 11)
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies

@lembacon lembacon added the new formula PR adds a new formula to Homebrew/homebrew-core label Aug 29, 2018
@tresf
Copy link
Contributor Author

tresf commented Aug 29, 2018

Formulae should not have optional or recommended dependencies

I'm going on https://docs.brew.sh/Formula-Cookbook, which clearly says:

:recommended generates an implicit without-foo option, meaning that the dependency is enabled by default and the user must pass --without-foo to disable this dependency. The default description can be overridden using the normal option syntax (in this case, the option declaration must precede the dependency):

... which is desired for Carla. Please offer guidance.

@BrewTestBot
Copy link
Contributor

  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies
  • Formulae should not have optional or recommended dependencies

@commitay
Copy link
Contributor

@commitay commitay closed this Aug 29, 2018
@tresf
Copy link
Contributor Author

tresf commented Aug 29, 2018

Upstream distributes this as an app

The purpose of this formula is to provide it as a build dependency offering pkg-config, headers and linking for the building of 3rd party applications, not to be run as an interactive application.

It is not meant to replace the app, it's meant for developers to build applications against so that they can use the Carla application in a project. Please advise.

Also, please help answer the question about "optional or recommended dependencies", it is still valid regardless of the target repository.

@commitay
Copy link
Contributor

Reopening so other maintainers can comment on it.

Also, please help answer the question about "optional or recommended dependencies", it is still valid regardless of the target repository.

See #31510. The formula should have not have optional or recommended dependencies, choose which ones are popular/useful and require them.

@commitay commitay reopened this Aug 29, 2018
@commitay commitay added the maintainer feedback Additional maintainers' opinions may be needed label Aug 29, 2018
@tresf
Copy link
Contributor Author

tresf commented Aug 29, 2018

choose which ones are popular/useful and require them.

Done, thanks.

Reopening so other maintainers can comment on it.

Thank you. For context, if useful: LMMS/lmms#2689 (comment)

@BrewTestBot
Copy link
Contributor

  • C: 9: col 3: dependency "fluid-synth" (line 9) should be put before dependency "macos" (line 8)
  • C: 10: col 3: dependency "liblo" (line 10) should be put before dependency "macos" (line 8)
  • C: 11: col 3: dependency "libmagic" (line 11) should be put before dependency "macos" (line 8)

@tresf tresf mentioned this pull request Aug 30, 2018
3 tasks
DomT4
DomT4 previously requested changes Sep 1, 2018
Formula/carla.rb Outdated
depends_on "fluid-synth"
depends_on "liblo"
depends_on "libmagic"
depends_on :macos => :mavericks
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

@tresf tresf Sep 4, 2018

Choose a reason for hiding this comment

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

He's patched this in master, but not in the stable. Should I include the patch now and remove the depends_on :macos => :mavericks flag so that the next version works without formula changes? Please advise.

The update will look something like this:

  def install
    args = []

    # Fix C++11 compilation
    if :macos < :mavericks?
      args << "MACOS_OLD=true"
    end

    system "make", *args
    system "make", "install", "PREFIX=#{prefix}"
  end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per 35a2e74, I've gone ahead and removed this restriction in favor of the MACOS_OLD flag. The developer says this flag will remain for the foreseeable future.

end

test do
system bin/"carla", "--version"
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance of a test that exercises a deeper level of functionality? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The library author confirms carla --version is adequate (it uses pyqt, so this will show errors if compilation failed). He's provided me a subsequent test... system bin/"carla-discovery-native", internal, :all. It will be tested locally and added to this PR.

@tresf
Copy link
Contributor Author

tresf commented Sep 5, 2018

All recent conversation points have been addressed in 35a2e74. Please let me know if there are any further improvements required.

@tresf
Copy link
Contributor Author

tresf commented Sep 5, 2018

The brew test carla is failing specifically system bin/"carla-discovery-native", "internal", ":all". I need to work out the reasons with upstream.

@tresf
Copy link
Contributor Author

tresf commented Sep 5, 2018

I need to work out the reasons with upstream.

Done via 24cf812.

@DomT4 DomT4 dismissed their stale review September 11, 2018 12:01

Changes requested were addressed.

@fxcoudert
Copy link
Member

@BrewTestBot test this please

@fxcoudert
Copy link
Member

Thanks @tresf for the pull request!

@fxcoudert fxcoudert closed this in 87849b8 Sep 14, 2018
@lock lock bot added the outdated PR was locked due to age label Oct 14, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

maintainer feedback Additional maintainers' opinions may be needed new formula PR adds a new formula to Homebrew/homebrew-core outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants