dynamic inheritance for packages that change build system by becker33 · Pull Request #10411 · spack/spack

becker33 · 2019-01-23T00:41:21Z

Began with @gonsie changing mpifileutils to use cmake.

Implemented DynamicInheritancePackage build system that can dynamically change its phases and attributes to mirror the other build system classes. The classes field is a dictionary of build system classes and anonymous specs describing when they are used.

Package classes are responsible for shadowing any dependencies and variants from the build system classes they intend to inherit from.

mpifileutils provides an example with a package changing from autotools to cmake, including handling the build_type variant from CMakePackage through the flag_handler method for the earlier versions that use autotools.

@tgamblin I think you will find this interesting.

adamjstewart

Overall, very interesting! I was hoping there would be some kind of way to specify multiple base classes, like:

class Mpifileutils(CMakePackage, AutotoolsPackage):

in terms of syntax, but this is better than nothing.

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

healther

I very much like this! And actually I think I prefer this implementation, i.e. an explicit list of build types for different versions, rather than trying to abuse multiple inheritances. But I guess that is just a personal opinion.

Some comments, but mainly typos (or suspected ones) that I found. There should be some documentation for DynamicInheritancePackage at least I don't currently completely grasp what the intended behavior and use is in most cases.

lib/spack/spack/build_systems/dynamic_inheritance.py

healther · 2019-01-23T10:48:57Z

lib/spack/spack/build_systems/dynamic_inheritance.py

+    Properties are captured using fget and attached to the package class
+    All others are attached directly.
+
+    This will need to be revisited if/when a build system adds a multimethod


This assumption should probably be guarded by a unit test...

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

healther · 2019-01-23T10:52:11Z

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


+    depends_on('[email protected]', when='@0.9:', type='build')
+
+    variant('build_type', default='RelWithDebInfo',


Doesn't this variant come from CMakePackage? Why does it need to be created here again?

Packages inheriting from DynamicInheritancePackage will have to shadow the directives from the parent class, since they need access to those before they get a chance to set their inheritance. That's variants, dependencies, and conflicts. If anyone has a brilliant idea let me know, but as of now I don't know of any way to do it other than shadowing those.

alalazo · 2019-01-23T13:33:14Z

TL;DR I think this PR addresses a need that is present in more than a few packages and has been overlooked so far, but I also have a few strong concerns on the current implementation where all the attributes of different build-systems are merged in a single class.

This can become quite messy and doesn't keep the information pertaining to different build-systems separated from each other. To get this feature right we might need some support from the concretizer and possibly an extension of the spec syntax.

I'll share below a few random thoughts that I noted down in the past on this issue. Hope they can be useful to foster some discussion.

Packages that can be built with different build systems fall into two categories:

Software that changed build-system at a particular point in time (e.g. mpifileutils)
Software that for a non-null time window could be built with two different build systems (e.g. hdf5 or fftw)

In case number 1. we have a 1:1 correspondence from a version number to a build-system. Case number 2. is more problematic as for a given version there can be multiple build-systems. The latter case might not be resolved with a "pick your preferred build-system" approach, as the artifacts of the build might be different for the same version of the software.

For all the examples that I could find right now, the time period in which a package could be built with a given build-system can be mapped to a version range. Nothing prevents though to identify this set with further constraints (like the os or anything - think of a package using one build system on Linux and one on Windows). Not sure though if generalizing this aspect is only of theoretical interest or if there are actual use cases.

If we model packages supporting multiple build-systems under the assumption that for a given version number we have a corresponding build-system (restrictive with respect to the use cases found so far) we can employ the current spec syntax. If we go instead with overlapping build-systems that the user can choose, the spec syntax needs to be extended (e.g. fftw can currently be built with CMake and Autotools, the user needs to be able to specify which one he wants to use).

Different build-systems might mean different variants or even different dependencies for the package. Nonetheless it would be good to keep the information well factored and avoid mixing together variants or attributes that come from different places. A tentative package API could be obtained with decorators (the pseudo-code below is only for illustrative purposes):

class FFTW(MultipleBuildSystems):
    """FFTW is ..."""

    # All the common directives and attributes


@FFTW.when('@:3.1')
class _FFTW(AutotoolsPackage):

    # All the directives and attributes related to the autotools build-system only
    # Here I need to be able to refer to variants in the FFTW main class


@FFTW.when('@3.0:')
class _FFTW(CMakePackage):

    # All the directives and attributes related to the CMake build-system only
    # Here I need to be able to refer to variants in the FFTW main class

The FFTW class should behave, during concretization, as a finite state machine with 2 states:

build-system-undefined
build-system-selected

When in the build-system-undefined state it should accept any constraint imposed by the concretizer, as the class doesn't know yet its full set of dependencies or variants etc. As soon as the build-system can be selected the constraints that have been imposed so far need to be checked and any inconsistency needs to be reported. This will probably need some support from the concretizer.

At install time the underlying spec is already concretized, and the behavior of the class is well-defined, so that should not pose any problem.

AndrewGaspar · 2019-05-08T22:44:15Z

Just want to leave some input that this would be useful for a few packages that transitioned from merely header-only to support a CMake style package system (supporting find_package).

closes spack#10411 closes spack#12941 This commit introduces a new repository where to move deprecated recipes e.g. for packages that changed build system. This will allow to benefit from base classes specialized for each build-system while still providing support for both recipes. Mpifileutils used as an example.

alalazo · 2020-03-10T15:24:32Z

Just want to mention a simpler approach for the same issue in #15420 meant for comparison with other proposals.

alalazo · 2021-09-16T10:45:12Z

@becker33 I'm closing this PR and I added a link to it in spack/seps#3

gonsie and others added 4 commits January 22, 2019 15:49

update mpifileutils for cmake build

11fb502

initial implementation of dynamic inheritance for build systems

adb4552

Move dynamic inheritance to its own build_system class

8d1c3b7

mpifileutils older versions conflict with clang + flake

83be559

adamjstewart reviewed Jan 23, 2019

View reviewed changes

var/spack/repos/builtin/packages/mpifileutils/package.py Outdated Show resolved Hide resolved

adamjstewart requested a review from alalazo January 23, 2019 03:01

healther reviewed Jan 23, 2019

View reviewed changes

becker33 force-pushed the mfu-cmake branch 2 times, most recently from 8001540 to 29c78f7 Compare January 24, 2019 00:31

Add testing, support adding phases, cleanup cmd.install --only bug

729e0b5

becker33 force-pushed the mfu-cmake branch from 29c78f7 to 729e0b5 Compare January 24, 2019 01:26

tgamblin self-requested a review January 24, 2019 09:30

gonsie mentioned this pull request Feb 21, 2019

mpiFileUtils v0.9 #10676

Merged

alalazo mentioned this pull request May 31, 2019

mesa18: Added a legacy mesa package using the older autotools build #11553

Closed

alalazo mentioned this pull request Oct 4, 2019

Multi Build System Packages [Prototype; Please Review] #12941

Closed

alalazo mentioned this pull request Mar 10, 2020

Added legacy repository for deprecated recipes #15420

Closed

2 tasks

adamjstewart added the build-systems label Aug 22, 2020

adamjstewart mentioned this pull request Oct 14, 2020

Newer preseq versions use a different build system #19298

Closed

cosmicexplorer mentioned this pull request Dec 22, 2020

unify "compiler" and "build system" to make complex dependencies more explicit #20518

Closed

2 tasks

adamjstewart mentioned this pull request Aug 1, 2021

Documentation: add section on changing build systems #22287

Closed

alalazo mentioned this pull request Sep 16, 2021

Add proposal to deal with multiple build-systems using directives spack/seps#3

Closed

alalazo closed this Sep 16, 2021

adamjstewart deleted the mfu-cmake branch September 16, 2021 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic inheritance for packages that change build system#10411

dynamic inheritance for packages that change build system#10411
becker33 wants to merge 5 commits intodevelopfrom
mfu-cmake

becker33 commented Jan 23, 2019

Uh oh!

adamjstewart left a comment

Uh oh!

Uh oh!

healther left a comment

Uh oh!

Uh oh!

healther Jan 23, 2019

Uh oh!

Uh oh!

healther Jan 23, 2019

Uh oh!

becker33 Jan 23, 2019

Uh oh!

alalazo commented Jan 23, 2019

Uh oh!

AndrewGaspar commented May 8, 2019

Uh oh!

alalazo commented Mar 10, 2020

Uh oh!

alalazo commented Sep 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants


		depends_on('[email protected]', when='@0.9:', type='build')

		variant('build_type', default='RelWithDebInfo',

Conversation

becker33 commented Jan 23, 2019

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

healther left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

healther Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

healther Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

becker33 Jan 23, 2019

Choose a reason for hiding this comment

Uh oh!

alalazo commented Jan 23, 2019

Uh oh!

AndrewGaspar commented May 8, 2019

Uh oh!

alalazo commented Mar 10, 2020

Uh oh!

alalazo commented Sep 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants