Allow packages with multiple build-systems (separate classes)

[alalazo]

fixes #12736 (in this PR the run_after decorator has an optional when= argument)
closes #30411

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 #30411 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

• Methods decorated with @when that expect the base case to be present in the build-system base class
• Methods using super to access build properties of the package, or expecting some of the methods that moved to be present in the Package class hierarchy
• Packages defining a custom phases attribute

Pros

• This PR moves all the build behavior to separate classes that have a package. Doing so makes the overall design less coupled, and Builder objects easier to swap in and out.
• Since each builder is a new separate class, there are no name clash issues. The buildsystem variant will determine which Builder class is instantiated.

Cons

• This PR is fairly compatible with what we had before but not as compatible as Allow packages with multiple build-systems (inheritance) #30411.
• Moving forward packagers would be required to code multiple classes in package.py

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

There will be only a single run_after and a single run_before decorator, as it is the case currently. These decorators can be used within each specific builder class:

class OpenJpeg(CMakePackage):
	...
	
class CMakeBuilder(BaseCMakeBuilder):
	def build(self, pkg, spec, prefix):
		pass
	
	@run_after('build')
	def foo(self):
		pass

There is no restriction on the choice of the names that can be used for buildsystems, e.g. we can have buildsystem=cmake. This is because we don't need to have separate decorators, since the partitioning of information is implemented by the class design.

Dynamic adapter for backward compatibility

To obtain a reasonable degree of backward compatibility, if the package.py doesn't contain specializations of the base builder classes, an adapter class is constructed on the fly. This class tries to, in order:

1. Look for specific attributes, methods or phases in the package
2. Fall back to the default implementation in the builder if they're not found

The class is created dynamically by a metaclass which generates methods to intercept certain attributes, methods or phases. As an example this is how attribute interceptors are created:

    @staticmethod
    def legacy_attribute_adapter(attribute_name):
        def _adapter(self):
            # Check if the phase was overridden by the package
            pkg_override = getattr(self.pkg, attribute_name, None)
            if pkg_override is not None:
                return pkg_override
            super_attribute = getattr(super(type(self), self), attribute_name)
            return super_attribute
        return property(_adapter)

To know which names to look for, each build-system must define at a class level the legacy attribute and method names

class GenericBuilder(BaseBuilder):
    """A builder for a generic build system, that require packagers
    to implement an "install" phase.
    """
    #: A generic package has only the "install" phase
    phases = ('install',)

    #: Names associated with package methods in the old build-system format
    legacy_methods = ()  # type: Tuple[str, ...]

    #: Names associated with package attributes in the old build-system format
    legacy_attributes = ('archive_files',)  # type: Tuple[str, ...]

Example of packages with multiple build-systems

• arpack-ng
• nasm
• uncrustify

[adamjstewart]

Took a look at this version but it's really unclear what's going on here. Some packages like python switched back to AutotoolsPackage with no explanation of how they work on Windows, other packages like uncrustify look the same as in #30411, other packages like zlib have a new *Builder class and I'm not sure what that's for or why it's necessary. Were all packages updated for this PR or is only zlib representative of what the package would need to look like? I wonder if it would be easier for me to propose what I would like a package.py to look like and you can tell me which of these are possible and which aren't.

[alalazo]

Some packages like python switched back to AutotoolsPackage with no explanation of how they work on Windows,

Can you point which ones are you referring to? Might have missed something during a rebase.

other packages like uncrustify look the same as in #30411,

Packages like uncrustify look the same because they don't need to override the default behavior of their build-system.

other packages like zlib have a new *Builder class and I'm not sure what that's for or why it's necessary

Packages like zlib are how packages should override anything related to their build if we go this route. Since we want for old packages to continue working as much as possible, I am creating on the fly an adapter from existing packages to this new scheme that should work most of the times, with the exceptions in the PR description.

From the point of view of functionality this PR and #30411 are the same, but this one moves building concerns to a separate class internally - it's a way to start moving all the responsibilities now in PackageBase to multiple classes. I think this might be appealing for maintainers.

For instance we don't need to fiddle with Python's class model to have a mock build. Just create a new Builder class and initialize it with a package object in a test. Or we might have simpler structures to be serialized and pushed to a worker process on macOS. Whatever is needed for the build is created directly at install time.

[alalazo]

Marked as "ready for review" to start running pipelines on this branch, and fix the failures

[scheibelp]

I've looked through most of this and wanted to start with some high-level questions/comments:

• There is some ambiguity in what goes into a Package vs. a Builder

  • e.g. IntelPackage has "configure" as a run_before method in the package
  • I assume the rule of thumb is "if it has anything to do with the build process, it should go in the builder" (e.g. cmake_args)
  • Even this concept is sometimes murky: e.g. cases where setup_build_environment sets environment variables that are later used by the build system
  • (a) Allow packages with multiple build-systems (inheritance) #30411 avoids forcing users to confront this ambiguity so IMO that has an advantage
    • I should make sure to clarify: the main thing I like is the ability to designate methods in packages as being associated with a specific build system superclass phase
    • I'm not sure if I perceive what about this leads to the problems Allow packages with multiple build-systems (inheritance) #30411 has (there's some related discussion in next point)

• PackageAdapterMeta uses legacy_* attributes of builders to determine what the Package can override

  • At the moment, if you take cmake.py, and combine all the legacy_* definitions, it appears to "cover" the entire builder
  • Why not defer all method calls to the package generically vs. having different categories of overrides?
  • (I assumed that can be achieved by using *args/**kwargs)
  • Is the goal to eventually remove these (or in fact raise an error if the subclass defines them)?
    • If so, you could use decorators to track Package methods that should override the build class
    • _PackageAdapterMeta could use these instead of legacy_* attributes (so in that sense I don't think the PR would be complicated much by supporting (a))

• It looks easy to support custom phases:

  • If _PackageAdapterMeta.__new__ changes
    for phase_name in default_builder_cls.phases:
    to
    for phase_name in default_builder_cls.phases + package_class.phases:
    then it looks like phases would be retrieved from the package just fine.
  • (I assume it would be trivial to retrieve the package class when creating the builder)

• (minor) NMakeBuilder and MakefileBuilder both define build_directory

  • I assume users might be confused that they have to override both of these for packages that build on Linux and Windows
  • My concern is possible confusion/frustration when builders with identically-named attributes also coincidentally would have identical intent/implementation

• (minor) The concept that you inherit a buildsystem class, then define a sibling class with a specific name and it will be used automatically, is not immediately clear

  • It avoids having to come up with names like FooCmakeBuilder, so the concept is useful
  • It will need some explanation though

• If someone overrides an Autotools package and wants to define setup_build_environment/configure then, following the idioms established in this PR, they now have to modify/define two classes

  • My understanding is that this isn't initially required because of PackageAdapterMeta, but that eventually that would be the intent
  • My point with this is that even for users that don't need to inherit multiple build systems, they will (eventually) need to consider this layout

[alalazo]

Just a few "easy" answers, before discussing the PR in the call today:

There is some ambiguity in what goes into a Package vs. a Builder

e.g. IntelPackage has "configure" as a run_before method in the package

The IntelPackage has not been ported yet to the new system. Note that all it does is to derive from Package. If I recall correctly
I just tried a few builds, and relied on the adapter class generated at runtime to make it work.

Even this concept is sometimes murky: e.g. cases where setup_build_environment sets environment variables that are later used by the build system

This is true, I don't remember if I raised the issue somewhere but I agree with what you say. At the moment I left the setup-*-environment in the package to do things gradually, and avoid touching the code in the build-environment. I agree though that the methods setting up the build environment should go into builders. Also flags needed to setup the build should go into builders.

PackageAdapterMeta uses legacy_* attributes of builders to determine what the Package can override

Is the goal to eventually remove these (or in fact raise an error if the subclass defines them)?

Yes, the idea is to deprecate the current layout of package.py in the long term. I don't think we should raise, but e.g. emit diagnostic on demand or something similar, and try to be clear that in version X.Y the old format will not be supported anymore. That, of course, if this gets merged. Another thing we should do in that case is to accept new packages only if written in this new way.

NMakeBuilder and MakefileBuilder both define build_directory

That's already the case for e.g. Autotools and CMake, and that's one of the win here. If you need to override the default for just one of them, you are not forced to repeat the default implementation for the other as it would happen in #30411

The concept that you inherit a buildsystem class, then define a sibling class with a specific name and it will be used automatically, is not immediately clear

Nothing has been documented so far, as documentation changes will also be big - and no design has been thoroughly discussed yet.

[balay]

adding @bangerth to dealii/oce discussion

[tgamblin]

@alalazo should we think about making it so the build system variant doesn't count towards the default/non-default variant count? I guess for most scenarios we probably do want to prioritize the default build system, so maybe this really is a specific thing we need to tweak dealii.

[alalazo]

So far I think it's fine, in the sense that this issue is really due to dealii dependencies being severely outdated. Yesterday I merged minimal updates to both suite-sparse and oce and now dealii concretizes using intel-tbb by default. That said:

1. Users probably want to use recent versions of opencascade instead of oce
2. For outdated references specifying the build system solves the issue

[bangerth]

Thanks everyone. deal.II should be able to build against modern versions of TBB (and if it doesn't, then that's a bug we need to fix) and the same should be true for OpenCASCADE vs OCE: deal.II should be able to build against either, and their most current versions, and if that is not true we need to fix this.

As mentioned above, what these packages do is (regrettably) out of our hands. I wished they all used cmake, but we can't force them to :-(

[dylex]

Unfortunately openvdb also needs an older version of intel-tbb (this isn't recorded in the dependencies, but at least for us it won't build with 2021, only 2020). Now that 2020 won't build, we can't build openvdb. There are newer versions of openvdb available though, which may be worth trying (@eloop).

[alalazo]

@dylex There's no issue if openvbd needs an old tbb, we just need to add the correct constraint in the openvbd recipe. That's what I did in #33553 and other similar PRs.

[eloop]

Unfortunately openvdb also needs an older version of intel-tbb (this isn't recorded in the dependencies, but at least for us it won't build with 2021, only 2020). Now that 2020 won't build, we can't build openvdb. There are newer versions of openvdb available though, which may be worth trying (@eloop).
@dylex I just submitted a pull request that updates the openvdb package to handle up to openvdb v10.0.0. There is still work to add some of the newer additions (e.g. nanovdb) but the core library and python extension now work for me on Ubuntu 22.04.

[alalazo]

@tgamblin I also timed develop at 83ee500 with this PR at commit 30c9ff5 to check the impact of adding a variant everywhere:

radiuss_develop.csv
radiuss_multiple.csv
radiuss.txt

There's some impact, but overall not really noticeable: