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

Fix octave sundials dep #29387

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@apjanke
Contributor

apjanke commented Jun 25, 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>)?

Per https://savannah.gnu.org/bugs/?52475, Octave 4.4.0 is not actually compatible with SUNDIALS 3.x, even though there's no check to prevent it from building against the incompatible 3.x series.

This PR changes Octave to build against SUNDIALS 2.x, and introduces a versioned sundials@2 formula to provide it.

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Jun 25, 2018

Can the dependency be removed entirely? Sundials 2.x doesn't appear to be maintained upstream as a separate release branch so I'm reluctant to add an @ formula for it.

Also note that the new audit against conditional dependencies is broken on --new-formula, which is why CI is failing.

@apjanke

This comment has been minimized.

Contributor

apjanke commented Jun 25, 2018

Well, if the sundials dependency is removed, then the ode5i and ode5s solvers will not work. (Which is not a regression from the current state of things, since they're broken under SUNDIALS 3.x.) I don't know how much demand there is in the Octave user community for these solvers; that's a user question. I can ask around. Ubuntu and Debian include it in their Octaves.

Amended the commit to make the open-mpi dependency use the :recommended mechanism instead of a conditional dep.

Looks like there's a bug in the audit code; it's throwing an error instead of an audit complaint.

        Error: Error running `rubocop --format json --force-exclusion --parallel --config /usr/local/Homebrew/Library/.rubocop_audit.yml /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/sundials@2.rb`
An error occurred while NewFormulaAudit/DependencyOrder cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/sundials@2.rb:1:0.
To see the complete backtrace run rubocop -d.
An error occurred while NewFormulaAudit/DependencyOrder cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/sundials@2.rb:1:0.
To see the complete backtrace run rubocop -d.

1 error occurred:
An error occurred while NewFormulaAudit/DependencyOrder cop was inspecting /usr/local/Homebrew/Library/Taps/homebrew/homebrew-core/Formula/sundials@2.rb:1:0.
undefined method `metadata' for nil:NilClass
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/gems/rubocop-0.55.0/lib/rubocop/cli.rb:265:in `display_error_summary'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/gems/rubocop-0.55.0/lib/rubocop/cli.rb:158:in `execute_runner'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/gems/rubocop-0.55.0/lib/rubocop/cli.rb:84:in `execute_runners'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/gems/rubocop-0.55.0/lib/rubocop/cli.rb:41:in `run'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/gems/rubocop-0.55.0/bin/rubocop:13:in `block in <top (required)>'
/System/Library/Frameworks/Ruby.framework/Versions/2.3/usr/lib/ruby/2.3.0/benchmark.rb:308:in `realtime'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/gems/rubocop-0.55.0/bin/rubocop:12:in `<top (required)>'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/bin/rubocop:22:in `load'
/usr/local/Homebrew/Library/Homebrew/vendor/bundle/ruby/2.3.0/bin/rubocop:22:in `<main>'

@apjanke apjanke referenced this pull request Jun 25, 2018

Merged

Octave and Sundials27 #41

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Jun 25, 2018

Looks like there's a bug in the audit code; it's throwing an error instead of an audit complaint.

Right. That's what I meant by "broken" :)

@apjanke

This comment has been minimized.

Contributor

apjanke commented Jun 25, 2018

Ah. Gotcha.

@apjanke

This comment has been minimized.

Contributor

apjanke commented Jun 25, 2018

There seems to be a decent amount of sundials-related activity on the GNU Octave mailing lists, which indicates some interest:

I've also asked over at dpo/homebrew-openblas since they are using SUNDIALS 2.7 themselves. Let's see what they have to say.

@apjanke

This comment has been minimized.

Contributor

apjanke commented Jun 25, 2018

From @schoeps:

I would say that sundials is important. ode15s and ode15i are the only Marlab-compatible stiff ODE solvers in Octave. There has been an effort to move to the newer version (http://savannah.gnu.org/bugs/?52475) but it's obviously not stable yet.

@ilovezfs

This comment has been minimized.

Contributor

ilovezfs commented Jul 4, 2018

Thanks for the PR @apjanke. We're going to pass on this since SUNDIALS 2.x isn't maintained as a separate release branch upstream.

@ilovezfs ilovezfs closed this Jul 4, 2018

@apjanke

This comment has been minimized.

Contributor

apjanke commented Jul 5, 2018

That's fine. I'll watch upstream activity and come back when there's SUNDIALS 3.x support.

@apjanke apjanke deleted the apjanke:fix-octave-sundials-dep branch Jul 5, 2018

@lock lock bot added the outdated label Aug 4, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.