espressopp: package for the ESPResSo++ software #2602

junghans · 2016-12-15T21:25:38Z

This commit add a package for the ESPResSo++
simulation software.

davydden · 2016-12-16T00:31:08Z

var/spack/repos/builtin/packages/espressopp/package.py

+    homepage = "https://espressopp.github.io"
+    url      = "https://github.com/espressopp/espressopp/archive/v1.9.3.zip"
+
+    version('master', git='https://github.com/espressopp/espressopp.git', branch='master')


rename master to develop, otherwise master < 1.9.3 in version comparisons

davydden · 2016-12-16T00:32:18Z

var/spack/repos/builtin/packages/espressopp/package.py

+    depends_on("cmake@2.8:", type='build')
+    depends_on("mpi", type=('build', 'link', 'run'))
+    depends_on("boost@:1.61.0+serialization+filesystem+system+python+mpi", when='@master', type=('build', 'link', 'run'))
+    depends_on("boost@1.56.0+python+mpi+filesystem+system+serialization", when='@1.9.3', type=('build', 'link', 'run'))


other versions don't depend on boost? Why is it specific 1.56.0?

davydden · 2016-12-16T00:33:11Z

var/spack/repos/builtin/packages/espressopp/package.py

+
+    depends_on("cmake@2.8:", type='build')
+    depends_on("mpi", type=('build', 'link', 'run'))
+    depends_on("boost@:1.61.0+serialization+filesystem+system+python+mpi", when='@master', type=('build', 'link', 'run'))


something wrong with 1.62.0? if you support it, then don't constrain the version at all.

Because at the time I wrote it wasn't tested with 1.62; will check now!

1.9.4 works with boost-1.62 on OpenSuse: https://build.opensuse.org/public/build/science/openSUSE_Tumbleweed/i586/python-espressopp/_log

@junghans I guess for the master branch I can remove the version restriction right? Can you confirm that with 1.62 works fine?

1.9.3 is two years old! So I would say, just drop all versions below 1.9.4 and all boost restrictions with it.

Done! I just put a restriction on the latest cmake, as I remember the system FindBoost.cmake in earlier versions wasn't updated to cover the case boost 1.62.

junghans · 2016-12-16T00:47:34Z

var/spack/repos/builtin/packages/espressopp/package.py

+    url      = "https://github.com/espressopp/espressopp/archive/v1.9.3.zip"
+
+    version('master', git='https://github.com/espressopp/espressopp.git', branch='master')
+    version('1.9.3', git='https://github.com/espressopp/espressopp.git', tag='v1.9.3')


Actually we could add 1.9.4, too.

fedepad · 2016-12-16T02:54:51Z

I rebased, squashed the commits and pushed. Hope it's ok.

junghans · 2016-12-16T05:26:37Z

var/spack/repos/builtin/packages/espressopp/package.py

+    variant('dg', default=False, description='Build developer guide')
+    variant('tests', default=False, description='Run the tests')
+
+    depends_on("cmake@3.7.1:", type='build')


I think cmake>=2.8 is enough.

junghans · 2016-12-16T05:28:03Z

var/spack/repos/builtin/packages/espressopp/package.py

+    depends_on("boost+serialization+filesystem+system+python+mpi", when='@1.9.4:', type=('build', 'link', 'run'))
+    extends("python")
+    depends_on("python@2:")
+    depends_on("py-mpi4py@2.0.0", when='@1.9.4:', type=('build', 'link', 'run'))


For the develop version the dependency is back to mpi4py>=1.3.1.

davydden · 2016-12-16T07:57:29Z

var/spack/repos/builtin/packages/espressopp/package.py

+# License along with this program; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+##############################################################################
+


i don't think you need those two lines, i would suggest removing both

It would be three lines actually, as the whole sentence starts one line above these two...
BTW, I see those lines in other packages, e.g. yaml-cpp, adios, just to mention two.
Just let me know what to do in this case, I can throw away those three lines or keep them ;)

What do we do with this?

davydden · 2016-12-16T07:59:59Z

var/spack/repos/builtin/packages/espressopp/package.py

+            options.append('-DCMAKE_BUILD_TYPE:STRING=Debug')
+        else:
+            options.append('-DCMAKE_BUILD_TYPE:STRING=Release')
+        options.extend(std_cmake_args)


i think you need to put std_cmake_args in front, otherwise IIRC cmake will pick up CMAKE_BUILD_TYPE from there instead of those you are setting above.

@adamjstewart @alalazo This package should be subclassing CMakePackage and not writing its own install() method. Please grep for CMakePackage in the repository for other examples.

I know about CMakePackage , but at the time I wrote it (a while ago) CMakePackage was kind of broken and almost no package using Cmake had implemented that way. I could turn into that ;)

Things have evolved. CMakePackage is now preferred, and we are (slowly) converting old stuff to it.

Ok, good to know, thanks! Then I will convert to that!

davydden · 2016-12-16T08:00:17Z

var/spack/repos/builtin/packages/espressopp/package.py

+        options.extend(std_cmake_args)
+        with working_dir('build', create=True):
+            cmake('..', *options)
+            make("clean")


out of curiosity, why do you need this?

That issue will probably jsut go away once this converts to CMakePackage.

davydden · 2016-12-16T08:02:31Z

var/spack/repos/builtin/packages/espressopp/package.py

+                make("ug-pdf", parallel=False)
+            if '+dg' in spec:
+                make("doc", parallel=False)
+            if '+tests' in spec:


remove test variant and use self.run_tests: instead. The reasons is that running tests does not change the installed package per-se, it's not really a variant in this sense. It's just an option. see #2605

Agree it's not a variant per se. At the time I was confused how to reproduce that step and that was the hack I used. Thanks.

Has the self.run_tests stuff made it into the docs yet?

Has the self.run_tests stuff made it into the docs yet?

apparently not.

davydden · 2016-12-16T08:06:42Z

var/spack/repos/builtin/packages/espressopp/package.py

+
+    depends_on("cmake@3.7.1:", type='build')
+    depends_on("mpi", type=('build', 'link', 'run'))
+    depends_on("boost+serialization+filesystem+system+python+mpi", when='@1.9.4:', type=('build', 'link', 'run'))


i don't think you need boost as run dependency. You link against it and that's it. Same with mpi above and fftw below.

see http://spack.readthedocs.io/en/latest/packaging_guide.html?highlight=dependency%20type#dependency-specs

Agree. In one my previous version I didn't have ('build', 'link', 'run') everywhere but as you said. I will revert back to that!

i don't think you need boost as run dependency. You link against it and that's it.

I suppose we could remove 'run' from a lot of our dependencies.

Same with mpi above

How is MPI not a run dependency, when you need mpirun to launch it?

How is MPI not a run dependency, when you need mpirun to launch it?

you don't need it per-se. Anything that is build with MPI can be run as ./executable with single mpi core.

True in theory. But I would say that in practice, MPI is a run dependency. We don't build with MPI just to run single-core.

@citibeth I of course agree, but I am not sure this is the intended usage, from docu

“run”: the project is used by the project at runtime. The package will be added to PATH and PYTHONPATH.

so it looks like it will matter only for spack load espressopp, but no packages so far declare mpi as run dependency.

@tgamblin @mathstuf : don't remember who added dependency types, but could you comment on run for mpi?

run is only 100% necessary when the code does exec("mpiexec") or similar. Other than that, I'd put it as a "does one usually use via mpiexec or not?" question where the threshold for "usually" is really low.

citibeth · 2016-12-16T16:21:16Z

var/spack/repos/builtin/packages/espressopp/package.py

+    depends_on("mpi", type=('build', 'link', 'run'))
+    depends_on("boost+serialization+filesystem+system+python+mpi", when='@1.9.4:', type=('build', 'link', 'run'))
+    extends("python")
+    depends_on("python@2:")


Does this work with Python3, or only Python2? If just Python2, it should be depends_on('python@2:2.7').

It looks like this package builds a Python extension?? If so, is the extension REQUIRED for this package to work? Or are there viable use cases in which you can run this software without the extension? If the latter, then you need to add a +python variant, default it to false, and do something like this:

extends('python', when='+python') depends_on('python@3:', when='+python')

It only works with python2 and builds a python module, which includes a shared object.

Is Espresso++ still useful without the Python module? If so, +python should be a variant, for those who wish to use the non-Python parts of it without building a Python stack.

Doesn't work at the moment with Python3.
Doesn't depends_on("python@2:") already cover for that (Any version of the Python2)?

Yes, it builds a python extension which is required when one runs the scripts, so I think what's there in now for this it's ok.

depends_on("python@2:") requires any version of python from 2.0 to current. You want something like depends_on("python@2:2.7.13") (the last python2 version).

citibeth · 2016-12-16T16:26:26Z

var/spack/repos/builtin/packages/espressopp/package.py

+    url      = "https://github.com/espressopp/espressopp/archive/v1.9.4.zip"
+
+    version('develop', git='https://github.com/espressopp/espressopp.git', branch='master')
+    version('1.9.4', git='https://github.com/espressopp/espressopp.git', tag='v1.9.4')


Versioned releases need to have a stable checksum. You can either: (a) specify a specific git hash (which will never change) instead of a git tag (which could conceivably change, even if policy is that it won't), or (b) use HTTP tarball download instead (you don't have to change stuff on GitHub, just use something like this in your Spack package: url = "https://github.com/citibeth/icebin/tarball/v0.1.0"

I'm back! I used spack checksum, should be fine right?

citibeth · 2016-12-16T16:28:25Z

var/spack/repos/builtin/packages/espressopp/package.py

+
+    depends_on("cmake@3.7.1:", type='build')
+    depends_on("mpi", type=('build', 'link', 'run'))
+    depends_on("boost+serialization+filesystem+system+python+mpi", when='@1.9.4:', type=('build', 'link', 'run'))


i don't think you need boost as run dependency. You link against it and that's it.

I suppose we could remove 'run' from a lot of our dependencies.

Same with mpi above

How is MPI not a run dependency, when you need mpirun to launch it?

citibeth · 2016-12-16T16:32:55Z

var/spack/repos/builtin/packages/espressopp/package.py

+            options.append('-DCMAKE_BUILD_TYPE:STRING=Debug')
+        else:
+            options.append('-DCMAKE_BUILD_TYPE:STRING=Release')
+        options.extend(std_cmake_args)


@adamjstewart @alalazo This package should be subclassing CMakePackage and not writing its own install() method. Please grep for CMakePackage in the repository for other examples.

citibeth · 2016-12-16T16:33:22Z

var/spack/repos/builtin/packages/espressopp/package.py

+        options.extend(std_cmake_args)
+        with working_dir('build', create=True):
+            cmake('..', *options)
+            make("clean")


That issue will probably jsut go away once this converts to CMakePackage.

citibeth · 2016-12-16T16:33:46Z

var/spack/repos/builtin/packages/espressopp/package.py

+                make("ug-pdf", parallel=False)
+            if '+dg' in spec:
+                make("doc", parallel=False)
+            if '+tests' in spec:


Has the self.run_tests stuff made it into the docs yet?

citibeth · 2016-12-16T16:50:14Z

Doesn't ***@***.***:") already cover for that (Any version of the Python2)?

No, that will cover Python2, Python3, Python4, etc --- anything by Python1. Use `depends_on('python@2:2.7')`. But if you KNOW it doesn't work with earlier versions of Python, you could even do something like `depends_on('python@2.6:2.7')`, etc. (AFAIK, Python 2.5 is pretty much extinct now).

fedepad · 2016-12-18T16:09:07Z

@citibeth Thanks for the clarification on the python version dependency, I will change according to that!

citibeth · 2016-12-21T23:35:23Z

You want `depends_on('python@2:2.7')`

…

On Wed, Dec 21, 2016 at 6:34 PM, becker33 ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In var/spack/repos/builtin/packages/espressopp/package.py: > + url = "https://github.com/espressopp/espressopp/archive/v1.9.4.zip" + + version('develop', git='https://github.com/espressopp/espressopp.git', branch='master') + version('1.9.4', git='https://github.com/espressopp/espressopp.git', tag='v1.9.4') + + variant('debug', default=False, description='Build debug version') + variant('ug', default=False, description='Build user guide') + variant('pdf', default=False, description='Build user guide in pdf format') + variant('dg', default=False, description='Build developer guide') + variant('tests', default=False, description='Run the tests') + + ***@***.***:", type='build') + depends_on("mpi", type=('build', 'link', 'run')) + depends_on("boost+serialization+filesystem+system+python+mpi", ***@***.***:', type=('build', 'link', 'run')) + extends("python") + ***@***.***:") ***@***.***:") requires any version of python from 2.0 to current. You want something like ***@***.***:2.7.13") (the last python2 version). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2602>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd10Jq7AgEqxIPGTJfo9qKb4rm7a2ks5rKbd4gaJpZM4LOlmj> .

fedepad · 2017-01-04T21:15:27Z

@citibeth I subclassed from CMakePackage now. Our package has few targets like make doc, etc.
So how do you specify them now? Because in almost all CMakePackages in the repo I haven't seen these custom targets, but I've only seen the cmake_args method. So where/inside what should I place them?

citibeth · 2017-01-04T21:18:52Z

@fedepad Unfortunately, CMakePackage is currently undocumented.
@adamjstewart @alalazo Do you know the answers to these questions?

fedepad · 2017-01-04T21:26:27Z

@citibeth @alalazo @adamjstewart Ok, looking the code:
https://github.com/LLNL/spack/blob/17b13b161b3ddcd691ea7ed90165cfab6dec3950/lib/spack/spack/build_systems/cmake.py
Looks like one can override the build and install method to accomplish that...maybe!

adamjstewart · 2017-01-04T21:28:24Z

Our package has few targets like make doc, etc. So how do you specify them now?

I believe it would look something like this:

def build(self, spec, prefix):                                              
    with working_dir(self.build_directory()):                               
        make()
        make('docs')

You can see how CMakePackage works by taking a look through lib/spack/spack/build_systems/cmake.py.

Alternatively, if you want make and make docs to be separate phases, you could do:

phases = ['cmake', 'build', 'build_docs', 'install']

def build_docs(self, spec, prefix):                                              
    with working_dir(self.build_directory()):                               
        make('docs')

although I don't see any particular advantage to this.

@alalazo We should probably allow users to specify a list of build targets like we did for AutotoolsPackage. Let me submit a PR to do that.

fedepad · 2017-01-04T23:04:16Z

Please review with the latest changes. Main logic should be there. Remain to clarify the "type" (build, link, run) for some of the dependencies, otherwise, I would keep it like that until we are 100% sure.
Also, if/when #2742 gets in, we can move the overridden build method which has custom targets with the new way introduced in the PR, namely build,install targets as a list.

adamjstewart · 2017-01-04T23:09:15Z

var/spack/repos/builtin/packages/espressopp/package.py

+    variant('dg', default=False, description='Build developer guide')
+
+    depends_on("cmake@2.8:", type='build')
+    depends_on("mpi", type=('build', 'link', 'run'))


In general, the default type is fine for most dependencies. So for all of these dependencies that specify type=('build', 'link', 'run'), I would remove that.

py-mpi4py may be an exception. I usually use type='nolink' for Python dependencies.

adamjstewart · 2017-01-04T23:12:03Z

var/spack/repos/builtin/packages/espressopp/package.py

+    depends_on("texlive", when="+pdf", type='build')
+    depends_on("doxygen", when="+dg", type='build')
+
+    run_tests = True


You probably don't want this in here. Tests are buggy depending on what OS you are on. For example, if someone wants to build espressopp on bgq, the tests may not work. If you hard-code run_test = True in here, what you're saying is you cannot install the package unless the tests pass. This may be fine for you, but it may not be fine for other users. I would remove this line. You can specify it on the command line when you install:

spack install --run-tests espressopp

Ok! Thanks!

junghans · 2017-01-05T20:26:46Z

ping @davydden

davydden

LGTM apart from a minor issue

davydden · 2017-01-06T02:05:10Z

var/spack/repos/builtin/packages/espressopp/package.py

+    depends_on("boost+serialization+filesystem+system+python+mpi", when='@1.9.4:')
+    extends("python")
+    depends_on("python@2:2.7.13")
+    depends_on("py-mpi4py@2.0.0", when='@1.9.4:', type='nolink')


develop satisfies this "when" condition. Add some reasonable upper bound to the range

It is already back to >= py-mpi4py-1.3.1 in v1.9.4.1, so only v1.9.4 needs >=py-mpi-2.

Check latest commit...

fedepad · 2017-01-06T02:33:57Z

Somehow my comment might have disappeared...anyway, check the latest commit.

junghans · 2017-01-06T03:07:59Z

var/spack/repos/builtin/packages/espressopp/package.py

+    depends_on("boost+serialization+filesystem+system+python+mpi", when='@1.9.4:')
+    extends("python")
+    depends_on("python@2:2.7.13")
+    depends_on("py-mpi4py@2.0.0", when='@1.9.4', type='nolink')


should be py-mpi4py@2.0.0: (adding :)

Ok, but that means that 1.9.4 should work with mpi4py>=2.0.0 while I think we can guarantee for mpi4py=2.0.0., or? Just let me know what to do and I can add that!

You can never guarantee that a package will work with a future version ;-). But in https://github.com/espressopp/espressopp/blob/cdf24abc7f08e2c0fda8ad5bdb6041fe03471873/CMakeLists.txt#L147 we required mpi4py-2.0 or higher.

Ok, I'll add that and push!

This commit adds a package for the ESPResSo++ simulation software.

This commit moves Espressopp package to CMakePackage. Addresses some comments in the PR.

@adamjstewart

Addressed some comments from @adamjstewart. Removed global run_tests setting and removed type from many dependencies. Set type for py-mpi4py to 'nolink'.

Added latest espressopp version and changed mpi4py (version) dependency for different espressopp versions.

Changed py-mpi4py version requirements for espressopp@1.9.4 according to the package build requirements for that version.

fedepad · 2017-01-06T03:41:50Z

@junghans Check latest...

junghans · 2017-01-06T05:02:42Z

LGTM!

junghans · 2017-01-06T05:41:15Z

Also fixes espressopp/espressopp#130.

junghans mentioned this pull request Dec 15, 2016

E++ package for Spack package manager for HPC espressopp/espressopp#130

Closed

fedepad force-pushed the features/espressopp branch from 484a2c2 to b72664d Compare December 15, 2016 22:16

davydden reviewed Dec 16, 2016

View reviewed changes

junghans commented Dec 16, 2016

View reviewed changes

fedepad force-pushed the features/espressopp branch from b72664d to a319a19 Compare December 16, 2016 02:48

junghans commented Dec 16, 2016

View reviewed changes

davydden suggested changes Dec 16, 2016

View reviewed changes

davydden reviewed Dec 16, 2016

View reviewed changes

citibeth reviewed Dec 16, 2016

View reviewed changes

citibeth mentioned this pull request Dec 16, 2016

Document self.run_tests stuff #2607

Closed

junghans mentioned this pull request Dec 16, 2016

Internal boost is broken with >=gcc-5.4 and CXX-98 espressopp/espressopp#136

Closed

adamjstewart mentioned this pull request Jan 4, 2017

Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

Merged

fedepad force-pushed the features/espressopp branch from a319a19 to 282fc9d Compare January 4, 2017 22:42

adamjstewart requested changes Jan 4, 2017

View reviewed changes

adamjstewart approved these changes Jan 5, 2017

View reviewed changes

davydden approved these changes Jan 6, 2017

View reviewed changes

junghans commented Jan 6, 2017

View reviewed changes

espressopp: package for the ESPResSo++ software

81853e7

This commit adds a package for the ESPResSo++ simulation software.

Federico Padua added 4 commits January 6, 2017 04:18

Move Espressopp package to CMakePackage

85380c9

This commit moves Espressopp package to CMakePackage. Addresses some comments in the PR.

Remove run_tests and changed type of dependencies

eea39e7

Addressed some comments from @adamjstewart. Removed global run_tests setting and removed type from many dependencies. Set type for py-mpi4py to 'nolink'.

Updated to last version and fix mpi4py dependency

68dfb65

Added latest espressopp version and changed mpi4py (version) dependency for different espressopp versions.

Changed mpi4py version requirements for espressopp

dd0ec54

Changed py-mpi4py version requirements for espressopp@1.9.4 according to the package build requirements for that version.

fedepad force-pushed the features/espressopp branch from afdfc91 to dd0ec54 Compare January 6, 2017 03:33

tgamblin approved these changes Jan 6, 2017

View reviewed changes

tgamblin merged commit 8dc0561 into spack:develop Jan 6, 2017

fedepad mentioned this pull request Mar 1, 2017

Espresso++ in Spack: report of first experiences espressopp/espressopp#161

Closed

espressopp: package for the ESPResSo++ software #2602

espressopp: package for the ESPResSo++ software #2602

Conversation

junghans commented Dec 15, 2016

Choose a reason for hiding this comment

fedepad Dec 16, 2016 • edited Loading

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

fedepad Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

fedepad commented Dec 16, 2016

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

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

citibeth commented Dec 16, 2016 via email

fedepad commented Dec 18, 2016

citibeth commented Dec 21, 2016 via email

fedepad commented Jan 4, 2017

citibeth commented Jan 4, 2017

fedepad commented Jan 4, 2017

adamjstewart commented Jan 4, 2017 • edited Loading

fedepad commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junghans commented Jan 5, 2017

davydden left a 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

fedepad commented Jan 6, 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

fedepad commented Jan 6, 2017

junghans commented Jan 6, 2017

fedepad Dec 16, 2016 •

edited

Loading

fedepad Dec 16, 2016 •

edited

Loading

adamjstewart commented Jan 4, 2017 •

edited

Loading