PLASMA: add version 18.9.0 (w/CMake) and remove 17.1 (no config. system) by luszczek · Pull Request #9489 · spack/spack

luszczek · 2018-10-11T04:09:29Z

No description provided.

healther · 2018-10-11T06:32:00Z

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

+    hg = "https://[email protected]/icl/plasma"

    version("develop", hg=hg)
-    version("17.1", "64b410b76023a41b3f07a5f0dca554e1")


typically we don't remove old versions

Acknowledged, but the old version had no configuration system and the new one has CMake. The Spack package class has to either extend MakefilePackage or CMakePackage as far as I understand.

Yes, but you can override the Cmake-specific methods and implement the make flow in e.g. the install method. See for example the nest package

I added the old version back that uses Makefile. nest package was useful - thanks.

healther · 2018-10-12T07:40:48Z

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

+        return options
+
+    # Before 18.9.0 it was an Makefile package
+    @when("@:17.1")


Do you know that only 18.9 introduced Cmake? If yes, why don't you use @:18.8.99? In theory someone could add a version in between

In theory, maybe. In practice, I approve commits to PLASMA, I develop PLASMA, I package PLASMA, I release PLASMA, I decide versioning scheme of PLASMA. And the version scheme is YEAR.MONTH.

healther · 2018-10-12T07:41:20Z

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


+    @when("@18.9.0:")
+    def cmake_args(self):
+        if self.spec.satisfies('@:17.1'):


this may never be triggered

healther · 2018-10-12T07:42:55Z

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

+        make("install")
+
+    @when("@:17.1")
    def edit(self, spec, prefix):


I'm not sure if this will work. It is a CmakePackage and I don't think it executes an edit-phase. I implemented everything needed to be done in the "old" build flow under a single (decorated) install method

edit() method is called from install. I tried it with all combinations of versions and dependencies and it install()/edit() seemed to have done its job.

healther · 2018-10-12T07:44:22Z

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

        header_flags = ""
        # accumulate CPP flags for headers: <cblas.h> and <lapacke.h>
        for dep in ("blas", "lapack"):
            try:  # in case the dependency does not provide header flags


but how would it find the headers then? If it doesn't work (or we might pick up system libraries) just throw an error and tell the user how to fix it.

This is what for dep loop is doing: walk dependencies, get the header paths, add it to build. Dependencies that do not provide headers are explicitly blocked and the comments next to conflict() say that the headers are missing or incomplete.

So we may never hit the except block because we explicitly disable all non-complete providers? Why do we ignore the exception then? What am I missing?

Suppose someone adds a new provider that also doesn't provide all headers.

The reason why there is a try-except because I had an exception for one of the combinations of dependencies.

Providing "blas" and "lapack" is not a well defined functionality. Some of this is taken care of by conflict designation. The usable packages are handled here. But the problem is "blas" might not provide something because "lapack" provides that. And looking at the packages of "blas" and "lapack", I didn't see a common theme I could just use in a consistent manner.

scheibelp · 2018-10-15T18:52:20Z

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

    def build_targets(self):
        targets = list()

+        if self.spec.satisfies('@18.9.0:'):


@luszczek It looks like this is intended to only return build targets for 17.1. However, I don't see anything making use of build_targets for 17.1, i.e. it looks like the entire build is defined in the install method. I wanted to check that (a) in fact you want these constraints to apply to 17.1 and (b) I was curious if you saw them getting applied (e.g. in the build log).

From looking at this, I think the return value of this method is never used, so you could remove all of the logic here.

I don't know if I'm misusing build_targets but install method is trivial. It is edit that does the real work as much as it can. What is left is things I cannot edit through code and I can just stick on the invocation line of make. And this is exactly what I'm trying to do in build_targets for 17.1. More specifically, MKLROOT is not nicely available for automated editing so I override it in build_targets and make MKLROOT=-lmkl does the rest.

@scheibelp Did I justified build_targets logic? Could you please approve the PR?

@luszczek Your intended use of build_targets is fine (and necessary for the time being), but what I meant was that I don't think that function is being used anywhere. When I pulled this PR and tested it for version 17.1, I didn't see the function get called. So I'm curious if:

you built 17.1 and it works for you: in that case I don't think the build target customization is required (because these build targets are not being added to the make invocation as far as I can tell)

you have not yet tried 17.1: in that case I suspect it may not succeed, especially if those make args are necessary

I should also say that while we like keeping old versions around. IMO in your case, as the developer, if you find it tedious or not worthwhile to maintain support across the two build systems, I'm not against it being removed.

I tested 17.1 with supported dependencies and they work with the property build_targets removed. So I removed it with another commit. Thank you for checking.

scheibelp · 2018-10-22T18:26:29Z

Thanks!

* Add version 18.9.0 (w/CMake) * Add version dependent install methods to handle transition from Make-based package (17.1) to CMake-based package, using the NEST package as an example * Remove unnecessary build_targets method for older Make-based version * Don't retrieve just the C/Fortran interfaces for netlib-lapack - blas/lapack libs are now handled the same for all implementations * Remove netlib-lapack detection patch

PLASMA: add version 18.9.0 (w/CMake) and remove 17.1 (no config. system)

094afbc

healther reviewed Oct 11, 2018

View reviewed changes

Port back the support for Makefile configuration

c01dbde

healther reviewed Oct 12, 2018

View reviewed changes

Adjust styling for flake8

8c7e2da

balay added the xSDK label Oct 12, 2018

healther approved these changes Oct 13, 2018

View reviewed changes

scheibelp reviewed Oct 15, 2018

View reviewed changes

Remove unused preperty and code unnecessary for build

2bf63a9

scheibelp merged commit 01913fb into spack:develop Oct 22, 2018

adamjstewart mentioned this pull request Jul 23, 2019

Updated prefix in cmake_args for Plasma #12103

Closed

Conversation

luszczek commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Oct 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scheibelp Oct 15, 2018 •

edited

Loading