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

fluid-synth 2.1.1 #48863

Closed
wants to merge 1 commit into from
Closed

fluid-synth 2.1.1 #48863

wants to merge 1 commit into from

Conversation

lilyinstarlight
Copy link
Contributor

  • 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?
  • Is your test running fine brew test <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>)?

Following from #46793 by @Bo98, this bumps the formula to 2.1.0 (released 2019-12-01) and implements a reasonably useful test that FluidSynth was installed and succesfully runs. I additionally added a make check step before make install since that seems to be common for packages that have it and FluidSynth recommends doing so after make when compiling from source.

See #46793 and #46793 (comment)

Thanks to @Bo98 for the idea of (and some code for) synthesizing a simple midi as a test.

I think the only potentially breaking change with 2.1.0 is that the fluid_synth_set_sample_rate function in libfluidsynth has been deprecated. I'm not sure if it's commonly used, however, because the documentation says the function is broken by design and the deprecation notice says:

Changing the sample-rate is generally not considered to be a real-time use-case, as it always produces some audible artifact ("click", "pop") on the dry sound and effects (because LFOs for chorus and reverb need to be reinitialized).

Let me know if any changes are necessary but everything works and passes on my system. I opened a new pull request despite #46793 already being open because this bumps the version to 2.1.0 (instead of 2.0.9) and supercedes that pull request. If that isn't the right thing to do, let me know the appropriate way to get the update merged.

Thanks!

Copy link
Member

@fxcoudert fxcoudert left a comment

Choose a reason for hiding this comment

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

Thank you for the pull request! Almost there…

@@ -29,11 +34,18 @@ def install

mkdir "build" do
system "cmake", "..", *args
system "make", "check"
Copy link
Member

Choose a reason for hiding this comment

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

Everything else is OK, but please remove make check: we don't generally run compile-time tests, except for security-related formulas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright; I'm pushing the change now. I saw it in other formulae (probably the security-related ones you are talking about) and was curious but didn't get a response in the other PR. Thanks!

@alebcay alebcay added the almost there PR is nearly ready to merge label Jan 11, 2020
@lilyinstarlight
Copy link
Contributor Author

lilyinstarlight commented Jan 12, 2020

It looks like the build failed this time on High Sierra on the csound package when testing reverse dependencies (judging by https://jenkins.brew.sh/job/Homebrew%20Core%20Pull%20Requests/version=highsierra/55116/console). I did rebase this on master when making the requested changes but I'm not entirely certain why it would fail now given the successfully installed versions of fluid-synth and csound should be the same for both packages and for the dependencies for csound (unless I'm measuring that wrong).

Do you have any ideas why it might be failing?

Current (failing) highsierra build: https://jenkins.brew.sh/job/Homebrew%20Core%20Pull%20Requests/version=highsierra/55116/console
Previous (successful) highsierra build: https://jenkins.brew.sh/job/Homebrew%20Core%20Pull%20Requests/version=highsierra/54986/console

Diff between initial PR and current:

diff --git a/Users/foster/Downloads/fluid-synth.rb b/Formula/fluid-synth.rb
index eceed476..96fdb266 100644
--- a/Users/foster/Downloads/fluid-synth.rb
+++ b/Formula/fluid-synth.rb
@@ -34,7 +34,6 @@ class FluidSynth < Formula
 
     mkdir "build" do
       system "cmake", "..", *args
-      system "make", "check"
       system "make", "install"
     end
 

Thanks for the help!

@lilyinstarlight
Copy link
Contributor Author

So I rebased against master and the build seems to have passed this time. I'm not entirely sure what was up with that.

Is there anything else I need to do to get this merged?

Thanks again!

Copy link
Member

@zbeekman zbeekman left a comment

Choose a reason for hiding this comment

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

LGTM

@zbeekman
Copy link
Member

@BrewTestBot test this please

@zbeekman zbeekman added ready to merge PR can be merged once CI is green and removed almost there PR is nearly ready to merge labels Feb 24, 2020
@alebcay
Copy link
Member

alebcay commented Feb 24, 2020

fluidsynth 2.1.1 is now available.

@lilyinstarlight
Copy link
Contributor Author

So as of last Monday FluidSynth 2.1.1 has been released (see link below). Since this PR includes more than a version bump, you can either merge this one then I can do a bump-formula-pr for 2.1.1 and test or I can open a new one/edit this one for the version bump and test or whatever else you would like to do.

https://github.com/FluidSynth/fluidsynth/releases/tag/v2.1.1

What would you prefer?

Thanks!

@zbeekman
Copy link
Member

If you could update this PR (title, update url, sha256 commit message) and amend the commit (or squash) and force push that would be ideal.

@lilyinstarlight lilyinstarlight changed the title fluid-synth 2.1.0 fluid-synth 2.1.1 Feb 24, 2020
@lilyinstarlight
Copy link
Contributor Author

I don't think I can give the PR a new branch name but I updated the PR title, commit message, and url and sha256 in the formula. The automated test and outputting a MIDI to CoreAudio both worked just fine with 2.1.1.

Copy link
Member

@zbeekman zbeekman 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 your contribution to Homebrew! 🎉 🥇

Without awesome contributors like you, it would be impossible to maintain Homebrew to the high level of quality users have come to expect. Thank you @fkmclane!

@zbeekman zbeekman closed this in 09b5989 Feb 25, 2020
@lilyinstarlight lilyinstarlight deleted the fluid-synth-2.1.0 branch February 25, 2020 13:20
@lock lock bot added the outdated PR was locked due to age label Mar 27, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age ready to merge PR can be merged once CI is green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants