Directive inheritance: laziness for the win by alalazo · Pull Request #2623 · spack/spack

alalazo · 2016-12-18T22:22:57Z

This PR should fix #2466 and without touching a lot of code. The basic ideas:

directives now are lazy, and used just to capture the values passed to them
a list of commands stemming from directives gets stored in the class being created
these commands are executed at Meta.__init__ time if the class being created comes from a package repository (if not it's just code that has been factored into a common base)

This also means that DictWrapper and inspect.stack are not used anymore, as we defer code execution to a later time where pkg is already available. ~~Currently this PR supports only single inheritance~~

Modifications

directives can be inherited
multiple inheritance of base classes containing directives is supported
tests that stress the new functionality

davydden · 2016-12-18T22:25:47Z

is there a way to specify which cmake is needed? Maybe a given package depends on [email protected]:, can we do this somehow?

tgamblin · 2016-12-18T22:27:25Z

@davydden: if the parent says depends_on('cmake'), the subclass should be able to refine that to [email protected]: by just writing depends_on('[email protected]:') explicitly. I believe @alalazo just wants this PR to allow CMakePackages to have an implicit depends_on('cmake').

tgamblin · 2016-12-18T22:28:26Z

@alalazo is on a roll here.

tgamblin · 2016-12-18T22:29:41Z

lib/spack/spack/directives.py

+        when = kwargs.get('when', pkg.name)
+        destination = kwargs.get('destination', "")
+        placement = kwargs.get('placement', None)
+        # Check if the path is relative


@alalazo: is there some chance that I can convince you to use blank lines before sub-parts of long sections of code?

tgamblin · 2016-12-18T22:29:50Z

lib/spack/spack/directives.py

+            'can\'t be an absolute path.\n'
+            message += "\tdestination : '{dest}\n'".format(dest=destination)
+            raise RuntimeError(message)
+        # Check if the path falls within the main package stage area


^^ like here

tgamblin · 2016-12-18T22:29:57Z

lib/spack/spack/directives.py

+        normalized_destination = os.path.normpath(
+            join_path(test_path, destination)
+        )  # Normalized absolute path
+        if test_path not in normalized_destination:


davydden · 2016-12-18T22:32:10Z

the subclass should be able to refine that to [email protected]: by just writing depends_on('[email protected]:') explicitly.

i see. As long as it's un-conditional (does not depend on a variant), then the union of depends_on('cmake') in parent and depends_on('[email protected]:)` in child should indeed work without a problem, whereas in general there could be some issues with conretizer, like this one #1781

I believe @alalazo just wants this PR to allow CMakePackages to have an implicit depends_on('cmake').

i like the idea for sure.

alalazo · 2016-12-18T22:38:25Z

@tgamblin This PR was one of the best code-puzzle so far in Spack! 😄

tgamblin · 2016-12-18T22:40:54Z

@alalazo: It's a really good idea. I bet this actually makes loading package modules quite a bit faster. The original version relied on walking up the call stack to find the package scope, but this won't need to do that work. I'm a little sad because this was actually one of the original code puzzles in Spack -- how to make the DSL. But I think the lazy DSL is better 😄

tgamblin · 2016-12-18T22:43:20Z

@alalazo: it might also make it easier to implement conditional directive groupings -- something I had thought of a while ago but that @TomasPuverle recently suggested. e.g.:

    with satisfies('@3.6:'):
        depends_on('foo')
        depends_on('bar')

    with satisfies('@3.8:'):
        depends_on('baz')

basically a way to factor out common when= clauses.

adamjstewart · 2016-12-19T00:03:18Z

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

    version('2.0',   'cdf266530fee8af87454f15feb619609')
    version('1.5.2', '545f98923430369a6b046ef3632ef95c')
    version('1.5.1', 'd774e4b5a0db5f0f171c4fc0aabfa14e')
-


If we just take a step back and think about it, it's insane how many packages we can install with almost no code. There are probably hundreds of would be Autotools and CMake packages that no longer need any special install method. #1186 might just go down as the biggest line saving PR in Spack history. I'm looking at this empty package and it's hard to think of anything else we could remove. Now if we could just get rid of that stupid license and from spack import *...

tgamblin

@alalazo: a few minor requests, but otherwise LGTM!

Do you want to merge this first or #2502 first? I would think #2502 first, then this can be rebased on the new test infrastructure.

tgamblin · 2016-12-27T08:37:55Z

lib/spack/spack/build_systems/cmake.py


 import llnl.util.tty as tty
 import spack.build_environment
+import spack.directives


Why not just from spack import * like regular Packages?

Just to track what we are importing from where. As we are in core I tried to avoid:

from XXX import *

but if you think it's more readable, I have no objection changing it.

tgamblin · 2016-12-27T08:38:33Z

lib/spack/spack/build_systems/cmake.py

    # build-system class we are using
    build_system_class = 'CMakePackage'

+    spack.directives.depends_on('cmake', type='build')


Why no just regular old depends_on? Seems more readable.

We definitely want thing to be readable so that other users can write their own Package subclasses or understand what is already present.

This comes with the comment above. Let me know if you want this changed.

I think the narrow import is good but can you do from spack.directives import depends_on so this is readable?

tgamblin · 2016-12-27T08:43:13Z

lib/spack/spack/directives.py

+            )
+            _directives_to_be_executed = []
+
+        return super(DirectiveMetaMixin, meta).__new__(meta, name, bases, attr_dict)  # NOQA: ignore=E501


just wrap it after __new__(.

tgamblin · 2016-12-27T08:49:21Z

lib/spack/spack/directives.py

-    pkg.versions[Version(ver)] = kwargs
+        # Store kwargs for the package to later with a fetch_strategy.
+        pkg.versions[Version(ver)] = kwargs
+    _directives_to_be_executed.append(_execute)


Could the directive decorator either:

Pass this list in as a parameter (kind of like how pkg was passed in before)? or

Make directives return either something callable on a Package or a list of them?

Either of these would make it so that not all directives have to refer to a global, and they'd consolidate any use of globals to the directive definition. It would also make it clearer to potential contributors how they might write their own directive (as the interface would be clear from the structure).

I went for solution number 2 and moved all the global variables into the meta-class, see 30ae175 . Let me know if you want further modifications here.

Do you want to merge this first or #2502 first?

I think this one is ready, while #2502 needs some work to fix the coverage. That said, I have no preference on the order and no hurry in having any of the two merged immediately.

…ming from directives into packages + lazy directives * _dep_types -> dependency_types * using a meta-class to inject directives into packages * directives are lazy fixes spack#2466

@tgamblin

…suggested by @tgamblin

CMakePackage: importing names from spack.directives directives: wrap __new__ to respect pep8

directives: removed global variables in favor of class variables. Simplified the interface for directives (they return a callable on a package or a list of them).

tgamblin · 2016-12-28T20:33:25Z

Ok this LGTM. Thanks! I'll merge this one then #2502 when it's ready. Basically I want to get #2502 in so that @citibeth can write tests for her PR and so that I can rebase #2681 on the new test framework.

@alalazo: can you take a quick look at #2681? It actually reworks Spec significantly.

tgamblin reviewed Dec 18, 2016

View reviewed changes

adamjstewart reviewed Dec 19, 2016

View reviewed changes

davydden mentioned this pull request Dec 19, 2016

Adds catch, cppunit, spdlog, tinyxml(1 and 2), google benchmark #2627

Merged

alalazo mentioned this pull request Dec 21, 2016

Remove mechanism that auto-accepts Oracle license agreements without … #1298

Closed

tgamblin requested changes Dec 27, 2016

View reviewed changes

alalazo added 5 commits December 28, 2016 17:18

inheritance of directives: using meta-classes to inject attributes co…

763487e

…ming from directives into packages + lazy directives * _dep_types -> dependency_types * using a meta-class to inject directives into packages * directives are lazy fixes spack#2466

directives.py: allows for multiple inheritance. Added blank lines as …

6c4865c

…suggested by @tgamblin

directives.py: added a test for simple inheritance of directives

1830a77

Minor improvement requested by @tgamblin

f36fb88

CMakePackage: importing names from spack.directives directives: wrap __new__ to respect pep8

Refactoring requested by @tgamblin

30ae175

directives: removed global variables in favor of class variables. Simplified the interface for directives (they return a callable on a package or a list of them).

alalazo force-pushed the features/inheritance_of_directives branch from ac4aaba to 30ae175 Compare December 28, 2016 16:25

tgamblin approved these changes Dec 28, 2016

View reviewed changes

tgamblin merged commit 17b13b1 into spack:develop Dec 28, 2016

alalazo deleted the features/inheritance_of_directives branch December 29, 2016 01:02

adamjstewart mentioned this pull request Dec 29, 2016

Remove cmake build dep from CMakePackages #2684

Merged

adamjstewart mentioned this pull request Jan 14, 2017

New package: CppAD #2832

Merged

alalazo mentioned this pull request Sep 1, 2017

Set default cmake build_type to 'Release' for llvm #5261

Merged

Conversation

alalazo commented Dec 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Modifications

Uh oh!

davydden commented Dec 18, 2016

Uh oh!

tgamblin commented Dec 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Dec 18, 2016

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!

davydden commented Dec 18, 2016

Uh oh!

alalazo commented Dec 18, 2016

Uh oh!

tgamblin commented Dec 18, 2016

Uh oh!

tgamblin commented Dec 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgamblin left a comment

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!

tgamblin commented Dec 28, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alalazo commented Dec 18, 2016 •

edited

Loading

tgamblin commented Dec 18, 2016 •

edited

Loading