nest: migrate to homebrew/core #21561

tammoippen · 2017-12-11T12:31:08Z

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>)?

Migrating because of: https://github.com/Homebrew/homebrew-science/issues/6331

Companion PR: https://github.com/Homebrew/homebrew-science/pull/6489

tammoippen · 2017-12-11T13:03:01Z

Failing because of dependency on matplotlib, which is also scheduled for migration (see top of list https://github.com/Homebrew/homebrew-science/issues/6331).

ilovezfs · 2017-12-11T13:17:03Z

It's unlikely matplotlib will be accepted into homebrew/core. You should vendor as is done with most python resources.

tammoippen · 2017-12-11T13:20:15Z

👍 will do so.

ilovezfs · 2017-12-11T13:20:26Z

Formula/nest.rb

+
+  requires_py3 = []
+  requires_py3 << "with-python3" if build.with? "python3"
+  depends_on "numpy" => requires_py3


depends_on "foo" => "with-bar" is an audit failure, which this use of a variable manages to circumvent. It's also unnecessary here since numpy builds with both python2 and python3 by default. Unless we're building both python2 and python3 bindings by default for nest, numpy will need to be vendored.

In Python for Formula Authors the matplotlib formula is given as an example for python dependencies and I followed their lead - and they use the variable to circumvent audit failure. Vendoring scipy and numpy now. matplotlib actually is not a necessary requirement - only used in examples, hence removing it from dependencies.

That documentation is dated.

ilovezfs · 2017-12-11T13:21:03Z

Formula/nest.rb

+  requires_py3 = []
+  requires_py3 << "with-python3" if build.with? "python3"
+  depends_on "numpy" => requires_py3
+  depends_on "scipy" => requires_py3


same comment as above

ilovezfs · 2017-12-11T13:22:20Z

Formula/nest.rb

+  end
+
+  def install
+    ENV.delete("CFLAGS")


This should never be needed. Ditto CXXFLAGS below.

ilovezfs · 2017-12-11T13:23:22Z

Formula/nest.rb

+  option "without-openmp", "Build without OpenMP support."
+  needs :openmp if build.with? "openmp"
+
+  depends_on "gsl" => :recommended


just require it

ilovezfs · 2017-12-11T13:23:30Z

Formula/nest.rb

+
+  option "with-python3", "Build Python3 bindings (PyNEST) instead of Python2 bindings."
+  option "without-openmp", "Build without OpenMP support."
+  needs :openmp if build.with? "openmp"


don't need this option

I do not understand, why we don't need this option? nest can be build with or without OpenMP. Are you suggesting, we build with OpenMP by default?

I'm saying we should have no openmp option.

ilovezfs · 2017-12-11T13:23:54Z

Formula/nest.rb

+  needs :openmp if build.with? "openmp"
+
+  depends_on "gsl" => :recommended
+  depends_on :mpi => [:optional, :cc, :cxx]


don't need this option

Same here. Are you suggesting we build with MPI by default?

I'm saying we should have no mpi option.

ilovezfs · 2017-12-11T13:24:39Z

Formula/nest.rb

+    ENV.prepend_create_path "PATH", buildpath/"cython/bin"
+    ENV.prepend_create_path "PYTHONPATH", buildpath/"cython/lib/python#{python_version}/site-packages"
+
+    # "out of source" build


comment isn't needed

ilovezfs · 2017-12-11T13:25:42Z

Formula/nest.rb

+    inreplace bin/"nest-config",
+        %r{#{HOMEBREW_REPOSITORY}/Library/Homebrew/shims.*/super},
+        "#{HOMEBREW_PREFIX}/bin",
+        FALSE


Not sure what the deal is with this inreplace but any use of the false argument is essentially a bug by definition.

See https://github.com/Homebrew/homebrew-science/pull/6479 why we introduced the inreplace and https://github.com/Homebrew/homebrew-science/pull/6488 for the FALSE part.

ilovezfs · 2017-12-11T13:26:07Z

Formula/nest.rb

+  homepage "http://www.nest-simulator.org/"
+  url "https://github.com/nest/nest-simulator/archive/v2.14.0.tar.gz"
+  sha256 "afaf7d53c2d5305fac1257759cc0ea6d62c3cebf7d5cc4a07d4739af4dbb9caf"
+  head "https://github.com/nest/nest-simulator.git"


don't need this

Without this, the --HEAD option is gone. Is this intended?

ilovezfs · 2017-12-11T13:26:47Z

Formula/nest.rb

+  depends_on "matplotlib" => requires_py3
+
+  depends_on "libtool" => :run
+  depends_on "readline" => :run


the => :run isn't needed. readline would never be => :build

ilovezfs · 2017-12-11T13:27:03Z

Formula/nest.rb

+
+  depends_on "libtool" => :run
+  depends_on "readline" => :run
+  depends_on "cmake" => :build


=> :build should go above the rest

ilovezfs · 2017-12-11T13:27:22Z

Formula/nest.rb

+
+  fails_with :clang do
+    cause <<-EOS.undent
+      Building NEST with clang is not stable. See https://github.com/nest/nest-simulator/issues/74 .


If this is true, it doesn't belong in homebrew/core.

Seems fixed, see nest/nest-simulator#732. On the other hand, clang from macOS does not support OpenMP (last time i checked) and we would have to rely on gcc anyway. Removing fails_with part.

ilovezfs · 2017-12-12T11:12:14Z

Formula/nest.rb

+  depends_on "libtool" => :run
+  depends_on "readline"
+
+  patch :DATA


New formulae must build without patching.

sjackman · 2017-12-13T06:49:37Z

It's unlikely matplotlib will be accepted into homebrew/core. You should vendor as is done with most python resources.

numpy and scipy are both in Homebrew/core. What's the reason that matplotlib should not be migrated to core?

ilovezfs · 2017-12-13T06:52:34Z

It takes under a minute to build and we don't usually package python libraries. I'm considering removing numpy and scipy too.

DomT4 · 2017-12-13T07:29:30Z

Formula/nest.rb

+         ] {join} Fold
+        } Function def
+      EOS
+      File.open(ENV["HOME"]+"/.nestrc", "w") { |file| file.write(nestrc) }


(testpath/".nestrc").write nestrc, no?

nest will look in HOME for .nestrc... if testpath is equal to HOME in the testing-sandbox, then this would be ok. But since there will not be a MPI build, .nestrc is not needed and those lines will go anyway.

Yeah, testpath is HOME here, as is buildpath during def install, although the latter may one day change as it has some side-effects that aren't completely desirable.

tammoippen · 2017-12-13T09:41:23Z

I will prepare a PR upstream with changes making the patch unnecessary, but I doubt there will be release in the near future (2.14 is relative fresh for nest ~20. Oct.). Also, I am a bit reluctant to vendor matplotlib: I found no other formula in core vendoring matplotlib; and besides the dead code in the tests (in the patch), matplotlib is actually not necessary for nest; also looking at matplotlib formula, there is some complexity I do not want to mirror in nest's formula just for vendoring.

I am ok with the other requests, but this might be a deal breaker until a new release of nest? I guess it is not possible to use a git-commit instead of a stable release?

ilovezfs · 2017-12-13T09:42:59Z

If the patch has been accepted upstream, we can apply it with a patch do block and a url.

Due to `brew audit --strict homebrew/core/nest`: * removing `revision 1` * removing `depends_on :python unless OS.mac?`

* vendor python packages * make OpenMP and MPI default * some restructuring

ilovezfs · 2017-12-25T13:52:19Z

@BrewTestBot test this please

ilovezfs · 2017-12-25T13:53:18Z

Formula/nest.rb

+    end
+
+    # add nosetest executable to path
+    ENV.prepend_create_path "PATH", libexec/"bin"


should just be prepend_path

stale · 2018-01-15T14:43:52Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

tammoippen · 2018-01-15T16:32:58Z

I am still working on an upstream PR. Please do not close.

queengooborg · 2018-01-28T00:37:18Z

@tammoippen How's it going?

tammoippen · 2018-01-31T15:48:11Z

@vinyldarkscratch Still waiting for the upstream PR...

queengooborg · 2018-01-31T22:51:01Z

I see, thanks for the update, @tammoippen!

stale · 2018-02-21T23:32:55Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

ilovezfs reviewed Dec 11, 2017

View reviewed changes

ilovezfs added the science Formula was originally in Homebrew/homebrew-science label Dec 11, 2017

ilovezfs reviewed Dec 12, 2017

View reviewed changes

commitay mentioned this pull request Dec 13, 2017

Remove qgis, gdal-framework: matplotlib Homebrew/homebrew-cask#41883

Merged

DomT4 reviewed Dec 13, 2017

View reviewed changes

tammoippen mentioned this pull request Dec 13, 2017

Remove strong dependency on matplotlib nest/nest-simulator#872

Merged

tammoippen added 3 commits December 13, 2017 10:56

nest: migrate to homebrew/core

840999e

Due to `brew audit --strict homebrew/core/nest`: * removing `revision 1` * removing `depends_on :python unless OS.mac?`

PR requests

8601c8c

* vendor python packages * make OpenMP and MPI default * some restructuring

rm mpi, openmp, head, patch

a95fd74

ilovezfs reviewed Dec 25, 2017

View reviewed changes

tammoippen mentioned this pull request Jan 11, 2018

homebrew-science is deprecated nest/nest-simulator#879

Closed

stale bot added the stale No recent activity label Jan 15, 2018

stale bot removed the stale No recent activity label Jan 15, 2018

stale bot added the stale No recent activity label Feb 21, 2018

stale bot closed this Mar 1, 2018

This was referenced Mar 6, 2018

nest: migrate to homebrew/core #24908

Closed

Update NEST Homebrew installation instructions to changes in Homebrew… nest/nest-simulator#897

Merged

Homebrew locked and limited conversation to collaborators May 4, 2018

nest: migrate to homebrew/core #21561

nest: migrate to homebrew/core #21561

Conversation

tammoippen commented Dec 11, 2017

tammoippen commented Dec 11, 2017

ilovezfs commented Dec 11, 2017

tammoippen commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjackman commented Dec 13, 2017

ilovezfs commented Dec 13, 2017

DomT4 Dec 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tammoippen commented Dec 13, 2017

ilovezfs commented Dec 13, 2017

ilovezfs commented Dec 25, 2017

Choose a reason for hiding this comment

stale bot commented Jan 15, 2018

tammoippen commented Jan 15, 2018

queengooborg commented Jan 28, 2018

tammoippen commented Jan 31, 2018

queengooborg commented Jan 31, 2018

stale bot commented Feb 21, 2018

DomT4 Dec 13, 2017 •

edited