Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow packages with multiple build-systems (inheritance) #30411

Closed
wants to merge 9 commits into from

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Apr 29, 2022

This PR extends the DSL that can be used in packages to allow declaring that a package uses different build-systems under different conditions. The main design decision both in here and in #30738 is to require each and every spec to have a build_system single valued variant. This variant can be used in many contexts to manipulate (force, query, display etc.) the build-system associated with a concrete spec.

Expected backward incompatibilities

  • Packages that define their own phases need to be converted to some existing build-system

Pros

  • This PR is expected to be almost 100% backward compatible

Cons

  • The PR maintain the same single class approach for coding recipes. To do so, it complicates the MRO logic by injecting the appropriate base class at build-time. This might be beneficial in the short term (less breakages in package recipes), but might not be in the long term (single blob class for packages).

  • As the package class responsibilities grow in numbers, so do the name clashes in different methods and attributes. In case an attribute defined for multiple build-systems used by the package needs customization, it's responsibility of the packager to distinguish all the different cases and return the appropriate value in each. For instance, a package buildable with both cmake and autotools that needs a custom build_directory for the latter, need to return in a property the default value for the former explicitly.

The buildsystem directive

To make it easy to declare the build-system, a new directive has been created. This directive is essentially a wrapper around variant:

def buildsystem(*values, **kwargs):
     default = kwargs.get('default', None) or values[0]
     return variant(
         'buildsystem',
         values=tuple(values),
         description='Build-systems supported by the package',
         default=default,
         multi=False
     )

For packages with a single buildsystem, the directive is already used in base classes - so no modifications are needed for packages in that sense.

Decorators to customize phases

To be able to run certain functions before or after a phase, or to override a phase, there is one decorator for each build-system:

class OpenJpeg(CMakePackage):
    @cmakelists
    def build(self, spec, prefix):
        pass

    @cmakelists.run_after('build')
    def foo(self):
        pass

The choice of the name to use, like cmakelists, have sometime been driven by the impossibility to use the most obvious choice (e.g. cmake) due to name clashes at the class scope.

Injection of the base class

A package object in this implementation lacks the base class to call any of the methods needed during build. That base class is injected when the builder is retrieved, by using an object wrapper:

class BuildWrapper(six.with_metaclass(BuilderMeta, object)):
    #: List of glob expressions. Each expression must either be
    #: absolute or relative to the package source path.
    #: Matching artifacts found at the end of the build process will be
    #: copied in the same directory tree as _spack_build_logfile and
    #: _spack_build_envfile.
    archive_files = []  # type: List[str]

    def __init__(self, package_object):
        package_cls = type(package_object)
        wrapper_cls = type(self)
        bases = (package_cls, wrapper_cls)
        new_cls_name = package_cls.__name__ + "BuildWrapper"
        new_cls = type(new_cls_name, bases, {})
        new_cls.__module__ = package_cls.__module__
        for attribute_name in (
            _RUN_BEFORE_PACKAGE_ATTRIBUTE,
            _RUN_AFTER_PACKAGE_ATTRIBUTE,
            _PHASE_OVERRIDE_PACKAGE_ATTRIBUTE,
        ):
            package_callbacks = getattr(package_cls, attribute_name, [])
            wrapper_callbacks = getattr(wrapper_cls, attribute_name, [])
            # FIXME: check that wrapper_cls has no phase overrides,
            # FIXME: they should be simple callbacks
            setattr(new_cls, attribute_name, package_callbacks + wrapper_callbacks)

        self.__class__ = new_cls
        self.__dict__ = package_object.__dict__

In the implementation above the relevant part is that type(self) will evaluate to specialized bases from different build-systems, and each builder must define a PackageWrapper which is effectively implementing the base class methods.

Example of packages with multiple build-systems

  • arpack-ng
  • nasm
  • uncrustify
$ spack install -v arpack-ng~mpi buildsystem=cmakelists
[ ... ]
$ spack install -v arpack-ng~mpi buildsystem=autotools
[ ... ]

@spackbot-app

This comment was marked as off-topic.

@alalazo alalazo force-pushed the feature/multiple_build_systems branch 3 times, most recently from 7c2b441 to 2b66f8d Compare May 3, 2022 14:04
@alalazo alalazo force-pushed the feature/multiple_build_systems branch from 2b66f8d to 87c2098 Compare May 3, 2022 14:09
@spackbot-app spackbot-app bot added the patch label May 3, 2022
@alalazo alalazo force-pushed the feature/multiple_build_systems branch 2 times, most recently from 4805d10 to c0e675f Compare May 3, 2022 21:07
@alalazo alalazo changed the title Rework build-systems to extract default build methods Allow packages with multiple build-systems May 5, 2022
@alalazo alalazo force-pushed the feature/multiple_build_systems branch from 20da7c9 to 98a5f2f Compare May 5, 2022 19:17
@alalazo alalazo mentioned this pull request May 6, 2022
@scheibelp scheibelp self-assigned this May 10, 2022
@alalazo alalazo force-pushed the feature/multiple_build_systems branch 3 times, most recently from e7f1c84 to b5abd75 Compare May 13, 2022 11:19
@adamjstewart adamjstewart self-assigned this May 14, 2022
Copy link
Member

@adamjstewart adamjstewart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to start following this PR more closely because many of the packages I maintain are dependent on these changes. Here are a couple of use cases to consider while working on this:

Python

Right now, all of our Python libraries are built with pip. However, pip is a full-fledged package manager, and we currently go out of our way to disable most of its features like dependency management and automatic download. I would really like to use build and installer instead. However, build and installer will likely be difficult to use for Python 2 due to additional dependencies or dropping Python 2 support a long time ago. What I would really like is for ^python@3.6: packages to be built with build/installer and ^python@:3.5 packages to be built with pip. Ideally, this would work without changing every python package in Spack. Is it possible to support this within PythonPackage?

GDAL

See #30668 for details. Basically, the GDAL package is becoming quite complicated, and depending on which version you are trying to build, the entire build system and every single variant will change. What I would really like is to be able to do something like:

@when('@3.5:')
class Gdal(CMakePackage):
    # driver variants

@when('@3.0:3.4')
class Gdal(AutotoolsPackage):
    # driver variants

@when('@:2')
class Gdal(AutotoolsPackage):
    # dependency variants

In this case, different versions of GDAL are so different that I want to store them separately. This allows me to ignore them (if they worked before then they should hopefully continue to work) but also keep the class for the latest versions as simple and easy to maintain as possible. I would also be fine with storing different classes in different files. Would something like this be possible? From what I've seen so far in this PR you can only do:

class Gdal(CMakePackage, AutotoolsPackage):
    with when('@3:'):
        # driver variants
        with when('@3.5:'):
            # cmake installation steps
        with when('@3.0:3.4'):
            # autotools installation steps
    with when('@:2'):
        # dependency variants
        # autotools installation steps

As you can imagine, this doubly nested structure (is this even possible?) is kinda of a nightmare to maintain and wrap your head around.

@@ -717,6 +718,17 @@ def _execute_resource(pkg):
return _execute_resource


def buildsystem(*values, **kwargs):
default = kwargs.get('default', None) or values[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is used heavily through Spack, but we should really avoid the use of *args and **kwargs. It makes type checking impossible, and incorrect parameter spellings go unnoticed.

Copy link
Member Author

@alalazo alalazo May 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is doable when we drop Python 2.7. Until then we cannot do:

def buildsystem(*values, default=None):
    ...

@alalazo
Copy link
Member Author

alalazo commented May 15, 2022

What I would really like is for ^python@3.6: packages to be built with build/installer and ^python@:3.5 packages to be built with pip. Ideally, this would work without changing every python package in Spack. Is it possible to support this within PythonPackage?

I think it is. The way it would be done is:

  1. Define two build-systems one for pip the other for build and installer
  2. Define PythonPackage as a base class inheriting from both, with the condition you mention

In PythonPackage we would use something like:

buildsystem(
    conditional('pip', when='^python@:3.5'),
    conditional('buildinstaller', when='^python@3.6:'),
    default='buildinstaller'
)

@alalazo alalazo force-pushed the feature/multiple_build_systems branch from b5abd75 to 8532e67 Compare May 16, 2022 14:18
@alalazo alalazo changed the title Allow packages with multiple build-systems Allow packages with multiple build-systems (inheritance) May 18, 2022
@alalazo
Copy link
Member Author

alalazo commented May 19, 2022

@tgamblin tgamblin assigned tgamblin and unassigned scheibelp and adamjstewart Jun 30, 2022
@tgamblin tgamblin self-requested a review June 30, 2022 05:46
@alalazo alalazo closed this Oct 25, 2022
@alalazo alalazo deleted the feature/multiple_build_systems branch October 25, 2022 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants