Added customization for make targets in 'build' and 'install' phases for AutotoolsPackage by alfredo-gimenez · Pull Request #2464 · spack/spack

alfredo-gimenez · 2016-12-02T19:11:33Z

Note: for some reason make('target1', 'target2') fails, so I had to run a separate make for each target.

…Package

adamjstewart · 2016-12-02T19:13:56Z

alalazo

I think adding an hook to change make targets for build and install is a good idea. Adding another indirection to modify check is not: in that case it is better to code the custom behavior in the package that requires it.

alalazo · 2016-12-02T19:20:55Z

lib/spack/spack/build_systems/autotools.py

        options = ['--prefix={0}'.format(prefix)] + self.configure_args()
        inspect.getmodule(self).configure(*options)

+    def make_targets_test(self):


I am not in favor of adding make_targets_test. The two common cases check and test are already in the base class. If you need to add targets that are specific to Blitz++ I would use a decorators in Blitz++ package to run something after the build phase when --run-tests is given.

alalazo · 2016-12-02T19:25:14Z

lib/spack/spack/build_systems/autotools.py

+
+        return targets
+
+    def make_targets_build(self):


I am instead in favor of coding an hook to generalize the targets for build. Anyhow I would change the name to build_targets. For symmetry reason I would add a similar install_targets method.

Sure, I agree, its easy enough to override check anyhow.

alalazo · 2016-12-02T19:26:41Z

lib/spack/spack/build_systems/autotools.py

+        for the targets specified in `build_make_targets`
+        """
+        for build_target in self.make_targets_build():
+            inspect.getmodule(self).make(build_target)


You mention you can't expand make_targets_build. Did you check why?

Didn't dig into it, but actually now that I think about it it's a bigger problem--this won't run make with no arguments :/

alalazo · 2016-12-02T19:28:36Z

lib/spack/spack/build_systems/autotools.py

@@ -175,11 +198,14 @@ def _run_default_function(self):
            tty.msg('Skipping default sanity checks [method `check` not implemented]')  # NOQA: ignore=E501

    def check(self):


I wouldn't change the check method here. If you need some custom behavior for Blitz++ it's better to code it in the package.

…ed to figure out issues with multiple make targets

alalazo · 2016-12-02T20:11:27Z

@alfredo-gimenez Didn't try it yet, but the code looks much better to me now! 👍

adamjstewart · 2016-12-02T20:42:31Z

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

    version('1.0.0', '9f040b9827fe22228a892603671a77af')
+
+    def build_targets(self):
+        return ['lib']


Is there any advantage of this over:

def build(self, spec, prefix): make('lib')

Not really, @mamelara was looking for a way to add make targets, I mentioned overriding build and that it would be trivial to add a make_targets member, @alalazo liked the idea, so I figured I'd go for it. I don't think it introduces problems, unless someone incorrectly overrides both build_targets and build, which would be silly.

alalazo

Looking at @adamjstewart comment, I think this can be polished a bit more without losing generality

alalazo · 2016-12-02T20:51:06Z

lib/spack/spack/build_systems/autotools.py

        options = ['--prefix={0}'.format(prefix)] + self.configure_args()
        inspect.getmodule(self).configure(*options)

+    def build_targets(self):


A suggestion for a final polishing: could you try to make this a normal attribute?

build_targets = []

I did that at first, but changed it to match configure_args, which is also a member function. I agree though, I would also change configure_args to be a normal attribute.

Well, it's a minor point, but configure_args is a function because most of the time you'll need to code complex logic and we decided to avoid packagers having to write @property over and over. Make targets I think they will be 99% list of literal strings.

Unless there's any reason not to make configure_args a normal attribute?

@alfredo-gimenez Reason above 😄 I would leave configure_args as is and change only build_targets and install_targets.

alalazo · 2016-12-02T20:51:41Z

lib/spack/spack/build_systems/autotools.py

+        """
+        return []
+
+    def install_targets(self):


Also here:

install_targets = ['install']

alalazo · 2016-12-02T20:52:31Z

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


    version('1.0.0', '9f040b9827fe22228a892603671a77af')
+
+    def build_targets(self):


Here:

build_targets = ['lib']

alalazo · 2016-12-02T20:54:06Z

lib/spack/spack/build_systems/autotools.py

-        """The usual `make` after configure"""
-        inspect.getmodule(self).make()
+        """Make the build targets"""
+        inspect.getmodule(self).make(*self.build_targets())


Remove parenthesis here and below

alfredo-gimenez · 2016-12-02T21:47:59Z

Note: don't merge this yet, multiple make targets were broken when I tried to do multiple test targets, still have to test and fix this on a package with multiple build targets.

alalazo · 2016-12-02T21:50:38Z

@alfredo-gimenez I added the WIP tag, feel free to remove it when ready

alfredo-gimenez · 2016-12-02T22:05:04Z

I tested again, looks like the issue was with one of blitz's check targets, make for multiple targets works fine :) I don't have powers to remove the WIP tag but it's ready.

citibeth · 2016-12-04T15:55:18Z

@tgamblin
At what point is it better to just tell the packager to write their own install() method (overriding the default for AutotoolsPackage)? Extra hooks all over the place can make it hard for someone not well-versed in the system to understand what a package does?

…with spack#2464 openblas: derives from MakefilePackage

* MakefilePackage: changed build_args and install_args for consistency with #2464 openblas: derives from MakefilePackage * MakefilePackage: changed default edit behavior

tgamblin · 2016-12-19T00:27:43Z

@citibeth: I think if the build starts to deviate from the standard three configure; make; make install commands, people should probably just write a new install method, but this particular change doesn't seem ridiculously opaque to me.

alfredo-gimenez added 2 commits December 2, 2016 11:07

Customization for make targets in build and test phases for Autotools…

9680b78

…Package

Updated Blitz++ to use customized make build and test targets

ee82330

Removed flake8 error

84e7303

alalazo reviewed Dec 2, 2016

View reviewed changes

Removed make test customization, added make install customization, ne…

150ed95

…ed to figure out issues with multiple make targets

adamjstewart reviewed Dec 2, 2016

View reviewed changes

alalazo reviewed Dec 2, 2016

View reviewed changes

alfredo-gimenez changed the title ~~Added customization for make targets in 'build' and 'test' phases for AutotoolsPackage~~ Added customization for make targets in 'build' and 'install' phases for AutotoolsPackage Dec 2, 2016

Changed build_targets and install_targets to normal attributes

45adab1

alalazo approved these changes Dec 2, 2016

View reviewed changes

alalazo added the WIP label Dec 2, 2016

adamjstewart approved these changes Dec 2, 2016

View reviewed changes

adamjstewart removed the WIP label Dec 2, 2016

alalazo added a commit to epfl-scitas/spack that referenced this pull request Dec 5, 2016

MakefilePackage: changed build_args and install_args for consistency …

2c6c460

…with spack#2464 openblas: derives from MakefilePackage

alalazo mentioned this pull request Dec 5, 2016

openblas: derives from MakefilePackage #2488

Merged

3 tasks

alalazo added feature A feature is missing in Spack ready labels Dec 6, 2016

tgamblin merged commit 68e9a2e into spack:develop Dec 19, 2016

adamjstewart mentioned this pull request Jan 4, 2017

Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

Merged

		@@ -175,11 +198,14 @@ def _run_default_function(self):
		tty.msg('Skipping default sanity checks [method `check` not implemented]') # NOQA: ignore=E501

		def check(self):


		version('1.0.0', '9f040b9827fe22228a892603671a77af')

		def build_targets(self):

Conversation

alfredo-gimenez commented Dec 2, 2016

Uh oh!

adamjstewart commented Dec 2, 2016

Uh oh!

alalazo 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!

alalazo commented Dec 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo 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!

alfredo-gimenez commented Dec 2, 2016

Uh oh!

alalazo commented Dec 2, 2016

Uh oh!

alfredo-gimenez commented Dec 2, 2016

Uh oh!

citibeth commented Dec 4, 2016

Uh oh!

tgamblin commented Dec 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants