SCR CMake package #3916

gonsie · 2017-04-19T23:56:41Z

SCR is now using CMake to build.

This package is still being developed, please add the WIP label.

tgamblin

mostly LGTM -- some requests/comments.

tgamblin · 2017-04-20T06:16:55Z

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

+
+    version('master', git='https://github.com/llnl/scr.git', branch='master')
+
+    depends_on('pdsh')


Is this a build/link dependency? I'm suspicious it's a run dependency.

I think it should be build/run. CMake will fail if it's not there, and it's a run dependency.

tgamblin · 2017-04-20T06:17:50Z

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

+    depends_on('zlib')
+    depends_on('mpi')
+
+    variant('dtcmp', default=True, description="DTCMP")


Is there maybe a better description than saying it louder? 😄

tgamblin · 2017-04-20T06:19:27Z

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

+    # variant('yogrt', default=True, description="Lib YOGRT")
+    # depends_on('yogrt', when="+yogrt")
+
+    ## MySQL not yet in spack


mariadb is, though. Is it worth making mysql a vdep and making mariadb and (some future) mysql providers?

It doesn't look like there's a good guarantee of the two APIs staying identical, so I'm not sure a vdep fits well.

tgamblin · 2017-04-20T06:23:07Z

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

+    ## NOTE: scr-v1.1.8 is built with autotools and is not properly build here.
+    ## scr-v1.1.8 will be deprecated with the upcoming release of v1.2.0
+    # url      = "https://github.com/LLNL/scr/releases/download/v1.1.8/scr-1.1.8.tar.gz"
+    # version('1.1.8', '6a0f11ad18e27fcfc00a271ff587b06e')


Is it worth preserving old versions (i.e. are there going to be codes that depend on them?) If so, it might be useful to look at dyninst, which is an example of a package with two build systems. Only disadvantage is that it extends Package, which lacks many of the nice phases that AutotoolsPackage and CMakePackage provideI. I don't think anyone's tried to do this with multiple inheritance, though it was discussed with @alalazo here in #3642.

There should not be any codes dependent on older versions, and as of now the SCR team does not plan to support versions before 1.2.0 once that version is released.

@tgamblin How frequent are cases where a package changes build system and we want to support both? I ask because multiple inheritance won't work out of the box, but we can think of adding a build system class that is a "variant" (in the design pattern sense) and can defer to a later stage the instantiation of its actual type.

For example we can have this "shell" build system and instruct it to turn either in AutotoolsPackage or CMakePackage after the inspection of the version of the concrete spec.

Personally, I only care about installing the latest version of a package. Keeping a single build system for this package makes everyone's life easier. It's an interesting technical problem if @alalazo gets bored, but there are so many other open issues that I think we should drop the old versions altogether and only support CMake releases until we know of someone who actually needs the old releases.

@alalazo @adamjstewart: Yeah in this case the old releases are not necessary, that I know of. SCR has mainly been used internally at LLNL and people are likely to only need the latest release.

I think this is more of an issue for things we've been supporting in Spack for old versions.

tgamblin · 2017-04-20T06:24:36Z

@gonsie: gave you write privs -- you can add labels now 😄

adamjstewart · 2017-04-25T17:13:47Z

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


-class Scr(Package):
+from llnl.util.filesystem import join_path


This import isn't necessary. join_path is already imported by from spack import *. To see exactly what comes with that, check out spack.__all__.

adamjstewart · 2017-04-25T17:16:19Z

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

+    ## NOTE: scr-v1.1.8 is built with autotools and is not properly build here.
+    ## scr-v1.1.8 will be deprecated with the upcoming release of v1.2.0
+    # url      = "https://github.com/LLNL/scr/releases/download/v1.1.8/scr-1.1.8.tar.gz"
+    # version('1.1.8', '6a0f11ad18e27fcfc00a271ff587b06e')


Personally, I only care about installing the latest version of a package. Keeping a single build system for this package makes everyone's life easier. It's an interesting technical problem if @alalazo gets bored, but there are so many other open issues that I think we should drop the old versions altogether and only support CMake releases until we know of someone who actually needs the old releases.

becker33 · 2017-04-25T22:42:26Z

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

+
+    def configure_args(self):
+        args = []
+        return args


I know this needs to go away if it stays empty, leaving it here for now as I'm not sure whether I'll need to change it yet.

adamjstewart · 2017-04-26T22:34:02Z

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

+
+
+class Libyogrt(AutotoolsPackage):
+    """FIXME: Put a proper description of your package here."""


Needs description.

adamjstewart · 2017-04-28T20:00:13Z

Here are the things causing flake8 tests to fail:

var/spack/repos/builtin/packages/scr/package.py:30: [E302] expected 2 blank lines, found 1
var/spack/repos/builtin/packages/scr/package.py:37: [E266] too many leading '#' for block comment
var/spack/repos/builtin/packages/scr/package.py:38: [E266] too many leading '#' for block comment
var/spack/repos/builtin/packages/scr/package.py:57: [E266] too many leading '#' for block comment
var/spack/repos/builtin/packages/scr/package.py:107: [E126] continuation line over-indented for hanging indent
var/spack/repos/builtin/packages/scr/package.py:110: [E126] continuation line over-indented for hanging indent
var/spack/repos/builtin/packages/scr/package.py:113: [E126] continuation line over-indented for hanging indent
var/spack/repos/builtin/packages/scr/package.py:116: [E126] continuation line over-indented for hanging indent
var/spack/repos/builtin/packages/scr/package.py:119: [E126] continuation line over-indented for hanging indent
var/spack/repos/builtin/packages/scr/package.py:127: [E501] line too long (82 > 79 characters)
var/spack/repos/builtin/packages/scr/package.py:130: [E116] unexpected indentation (comment)

All issues addressed, Todd not available to confirm.

tgamblin previously requested changes Apr 20, 2017

View reviewed changes

tgamblin added the WIP label Apr 20, 2017

adamjstewart reviewed Apr 25, 2017

View reviewed changes

becker33 reviewed Apr 25, 2017

View reviewed changes

becker33 mentioned this pull request Apr 25, 2017

Update the Spack Package LLNL/scr#22

Closed

adamjstewart reviewed Apr 26, 2017

View reviewed changes

adamjstewart approved these changes Apr 27, 2017

View reviewed changes

gonsie and others added 12 commits July 18, 2017 11:02

SCR CMake package

b4b5d89

Fix dtcmp variant description and set pdsh deptypes to build/run

02ecb7d

added variants for system config file location

d2a5422

add variants for several scr cmake options

3de6ab0

Added more variants for compile time options

8f8d87d

Added libyogrt and made corrosponding changes to scr package.

7c95ca2

Cleanup yogrt package now that Ive verified it works

4a4901c

Added description for libyogrt

e05c204

upper case async api names

7ea9f4c

comment for clarity

7dcdaf0

Make use of mv variants

d1e6ed2

fix pdsh package for scr

ac9ee39

gonsie and others added 4 commits July 28, 2017 13:45

added IBM BBAPI as async option

30763c2

update pdsh and scr to use static pdsh modules

9ac4279

flake

f4624ea

Fix spelling error

0c27d7c

becker33 merged commit bb4692f into spack:develop Aug 3, 2017

becker33 removed the WIP label Aug 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SCR CMake package #3916

SCR CMake package #3916

gonsie commented Apr 19, 2017 •

edited

Loading

tgamblin left a comment

tgamblin Apr 20, 2017

becker33 Apr 24, 2017

tgamblin Apr 20, 2017

tgamblin Apr 20, 2017

becker33 Apr 25, 2017

tgamblin Apr 20, 2017

becker33 Apr 24, 2017

alalazo Apr 25, 2017

adamjstewart Apr 25, 2017

tgamblin Apr 29, 2017

tgamblin Apr 29, 2017

tgamblin commented Apr 20, 2017

adamjstewart Apr 25, 2017

adamjstewart Apr 25, 2017

becker33 Apr 25, 2017

adamjstewart Apr 26, 2017

adamjstewart commented Apr 28, 2017


		version('master', git='https://github.com/llnl/scr.git', branch='master')

		depends_on('pdsh')


		class Scr(Package):
		from llnl.util.filesystem import join_path



		class Libyogrt(AutotoolsPackage):
		"""FIXME: Put a proper description of your package here."""

SCR CMake package #3916

SCR CMake package #3916

Conversation

gonsie commented Apr 19, 2017 • edited Loading

tgamblin 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

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

tgamblin commented Apr 20, 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

adamjstewart commented Apr 28, 2017

gonsie commented Apr 19, 2017 •

edited

Loading