-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Added customization for make targets in 'build' and 'install' phases for AutotoolsPackage #2464
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -151,9 +151,32 @@ def configure(self, spec, prefix): | |||
options = ['--prefix={0}'.format(prefix)] + self.configure_args() | |||
inspect.getmodule(self).configure(*options) | |||
|
|||
def make_targets_test(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
return targets | ||
|
||
def make_targets_build(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I agree, its easy enough to override check
anyhow.
for the targets specified in `build_make_targets` | ||
""" | ||
for build_target in self.make_targets_build(): | ||
inspect.getmodule(self).make(build_target) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mention you can't expand make_targets_build
. Did you check why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :/
@@ -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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@alfredo-gimenez Didn't try it yet, but the code looks much better to me now! 👍 |
@@ -31,3 +31,10 @@ class Blitz(AutotoolsPackage): | |||
url = "https://github.com/blitzpp/blitz/tarball/1.0.0" | |||
|
|||
version('1.0.0', '9f040b9827fe22228a892603671a77af') | |||
|
|||
def build_targets(self): | |||
return ['lib'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any advantage of this over:
def build(self, spec, prefix):
make('lib')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at @adamjstewart comment, I think this can be polished a bit more without losing generality
@@ -151,13 +151,25 @@ def configure(self, spec, prefix): | |||
options = ['--prefix={0}'.format(prefix)] + self.configure_args() | |||
inspect.getmodule(self).configure(*options) | |||
|
|||
def build_targets(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A suggestion for a final polishing: could you try to make this a normal attribute?
build_targets = []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's any reason not to make configure_args
a normal attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alfredo-gimenez Reason above 😄 I would leave configure_args
as is and change only build_targets
and install_targets
.
""" | ||
return [] | ||
|
||
def install_targets(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also here:
install_targets = ['install']
@@ -31,3 +31,10 @@ class Blitz(AutotoolsPackage): | |||
url = "https://github.com/blitzpp/blitz/tarball/1.0.0" | |||
|
|||
version('1.0.0', '9f040b9827fe22228a892603671a77af') | |||
|
|||
def build_targets(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here:
build_targets = ['lib']
"""The usual `make` after configure""" | ||
inspect.getmodule(self).make() | ||
"""Make the build targets""" | ||
inspect.getmodule(self).make(*self.build_targets()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove parenthesis here and below
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. |
@alfredo-gimenez I added the WIP tag, feel free to remove it when ready |
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. |
@tgamblin |
…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
@citibeth: I think if the build starts to deviate from the standard three |
Note: for some reason
make('target1', 'target2')
fails, so I had to run a separatemake
for each target.