do_install : allow for an arbitrary number of phases #1186
Conversation
|
@citibeth I'll probably need to ask you an example of use for |
| # # Create a dummy file so the build doesn't fail. | ||
| # # That way, the module file will also be created. | ||
| # with open(os.path.join(prefix, 'dummy'), 'w') as fout: | ||
| # pass |
There was a problem hiding this comment.
This won't be needed anymore, as the logic in Package is generic enough to deal with CMakePackage directly
There was a problem hiding this comment.
Sounds good to me.
On Jul 7, 2016 6:35 AM, "Massimiliano Culpo" notifications@github.com
wrote:
In lib/spack/spack/package.py:
+# if 'setup' in self._phases:
+# self.install_setup()
+#
+# if 'configure' in self._phases:
+# self.install_configure()
+#
+# if 'build' in self._phases:
+# self.install_build()
+#
+# if 'install' in self._phases:
+# self.install_install()
+# else:
+# # Create a dummy file so the build doesn't fail.
+# # That way, the module file will also be created.
+# with open(os.path.join(prefix, 'dummy'), 'w') as fout:
+# passThis won't be needed anymore, as the logic in Package is generic enough
to deal with CMakePackage directly—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLNL/spack/pull/1186/files/aa3c58885b33e6e7f81e6ea95c72b091a792ea25#r69885174,
or mute the thread
https://github.com/notifications/unsubscribe/AB1cd-1cgfmbeB1wryd963HE75cyqDjxks5qTNZXgaJpZM4JG8iX
.
|
You mean how to use ibmisc once it is built?
|
|
See #1189 for samples that use CMakePackage. |
|
@adamjstewart I'll try to reply here to your comments in #1169
The good use case is just what has been already started as a discussion : having specialized classes for different build-systems. The only point I would like to make is that, if we want to go that direction, it is better to have a base class that knows how to be extended for different workflows, test it the best we can, and then leave it alone for the longest time possible (as per the open-closed principle). Setting a base class that HAS an abstract workflow which may fit EVERY specialization we'll add in the future is instead very likely to end up in a Blob in the long term and will require tweaking the base class again if we need to add yet another step for a case that we didn't take into account today. Basically, for me it boils down to general maintainability and design considerations. |
|
|
||
| # This will be used as a registration decorator in user | ||
| # packages, if need be | ||
| PackageBase.sanity_check('install')(PackageBase.sanity_check_prefix) |
There was a problem hiding this comment.
@tgamblin @citibeth @adamjstewart @davydden @nrichart Here's a first stub at how a specialized class may look like right now :
phasesare described with a list of strings.- default implementations may (but need not to) be provided for every phase
- methods can be registered at package definition time as either
preconditionorsanity_checkfor any number of phases
Comments ?
There was a problem hiding this comment.
I still do not understand why this is useful, or how the user would control
things if phases could be named anything and differ between packages.
On Jul 8, 2016 9:58 AM, "Massimiliano Culpo" notifications@github.com
wrote:
In lib/spack/spack/package.py:
- def configure_args(self):
return list()
- def configure(self, spec, prefix):
options = ['--prefix={0}'.format(prefix)] + self.configure_args()inspect.getmodule(self).configure(*options)
- def build(self, spec, prefix):
inspect.getmodule(self).make()
- def install(self, spec, prefix):
inspect.getmodule(self).make('install')
This will be used as a registration decorator in user
packages, if need be
- PackageBase.sanity_check('install')(PackageBase.sanity_check_prefix)
@tgamblin https://github.com/tgamblin @citibeth
https://github.com/citibeth @adamjstewart
https://github.com/adamjstewart @davydden https://github.com/davydden
@nrichart https://github.com/nrichart Here's a first stub at how a
specialized class may look like right now :
- phases are described with a list of strings.
- default implementations may (but need not to) be provided for every
phase- methods can be registered at package definition time as either
precondition or sanity_check for any number of phasesComments ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/LLNL/spack/pull/1186/files/fa2783b9dc8c13e675a9b6791b240cdca61e4307#r70077657,
or mute the thread
https://github.com/notifications/unsubscribe/AB1cdy593mJztaO0eUhCWChaymSIb9Ihks5qTld_gaJpZM4JG8iX
.
There was a problem hiding this comment.
@citibeth A package knows it's phases, so that they could be added to spack info. Adding logic to stop at a particular phase of the installation doesn't require every package to follow the same workflow.
There was a problem hiding this comment.
@citibeth The way I understand it, PackageBase is like an abstract base class that allows arbitrary phases. Subclasses like AutotoolsPackage or PythonPackage define their own phases based on what is important for that particular build system. A user-defined software package can then extend AutotoolsPackage without having to define their own arbitrary phases. Of course, once in a while a package may want to add some phases, but most of the time the phases coming from AutotoolsPackage should be sufficient. At least that's the goal.
|
@alalazo You've convinced me! Now that I understand what you're trying to do, I like the idea of making an easily extendable abstract base class (Package), with other build-system specific classes extending it. That way we an encapsulate things like Python-specific RPATH handling into a PythonPackage, and CMake stuff into a CmakePackage. In a way this is just further integration of @citibeth's CmakePackage so that it becomes the default way to build Cmake packages. Although it would break backwards compatibility a bit, this could be used to simplify a lot of the code, so that I'm a little swamped with software install requests at the moment (we're decommissioning a machine to make way for a new one), so I won't be able to help out with the actual coding too much, but once things are merged and tested properly, I can make the necessary changes to |
|
In fact with the class Package the inherit from PackageBase and defines only the phase Just that |
|
@nrichart I know the current design doesn't break backwards compatibility. I'm suggesting that once this PR is merged and everyone becomes familiar with it, we start another PR to purposefully break backwards compatibility in order to simplify the code base. For example, only setup @alalazo Is the plan that most build systems will get their own class and any package that doesn't fit into a mold should extend Package or BasePackage? Either way, this should be easy for me to integrate into |
| from spack.stage import Stage, ResourceStage, StageComposite | ||
| from spack.util.compression import allowed_archive | ||
| from spack.util.environment import dump_environment | ||
| from spack.util.executable import ProcessError, Executable, which | ||
| from spack.util.executable import ProcessError, which |
There was a problem hiding this comment.
I'm trying to remember whether or not Executable was important... The last time I removed something like this due to Flake8, it resulted in #1095. If a user runs Executable in their package, will they still be able to find it if you remove it from here?
There was a problem hiding this comment.
Thanks for the tip, I'll have a closer look at this
|
I like this design a lot. I think the example package changes for Szip and SwitftSim do a good job of outlining the advantage of this design. Another good example to include would be a lot of purely GNU packages like Gmp, which if it extended AutotoolsPackage would no longer need an Another useful class might be LicensedPackage for licensed software. |
|
|
||
|
|
||
| class AutotoolsPackage(PackageBase): | ||
| phases = ['autoreconf', 'configure', 'build', 'install', 'log'] |
There was a problem hiding this comment.
maybe a short description of phases? e.g. what's log for?
There was a problem hiding this comment.
@davydden log will very likely disappear from the explicit list (it's a function that copies spack logs in the installation folder). But I agree on the idea : we should be able to write short descriptions of the stages for later use
There was a problem hiding this comment.
log == provenance? I think it's the part that copies build.out, build.env, spec.yaml, etc to prefix/.spack.
Maybe I was not clear on this. The code right now does EXACTLY what was done before in terms of sanity checks. Except that instead of having them in if 'install' in phases:
do_something() # do_install must know that 'install' may be among the phasesthey are attached to the phase they are related to. It's only a more modular design, not a change in behavior. Where do you see "a zillion broken sanity checks" ? The fact that EasyBuild may be broken on sanity checks is irrelevant here, right ?
I still need to take a look at "We can solve any problem by introducing an extra level of indirection" We may have a dictionary in the
We don't need dynamic introspection or inheritance to select phase names in |
… and sanity_checks of phases
Modifications : - added EditableMakefile to PackageBase subclasses - astyle modified as an example - preliminary hook to stop at a certain phase of install
The mechanism would be simpler if we could leverage exceptions to raise signals. Unfortunately forking does not permit to do so.
Now uses a StopIteration exception as a signal
Conflicts: lib/spack/spack/package.py
|
Will this change require more iports in packcge files?
|
|
On Sat, Oct 22, 2016 at 10:50 AM, Massimiliano Culpo <
|
No. these packages are included in the spack namespace in |
|
It seems so. :-) Sorry if I am not that responsive, but I am mostly working On Fri, Oct 21, 2016 at 5:56 PM, Todd Gamblin notifications@github.com
|
|
From your description, I surmise that 99% of packages don't crash or do The only danger would be that we can't wholesale add
I agree. As much as I despise Autotools, it seems they've come up with |
|
I thought I remembered some discussion on the issue. Please ignore if the On Fri, Oct 21, 2016 at 7:55 PM, Todd Gamblin notifications@github.com
|
|
It's nice to see that an e-mail that I sent when github was offline on Friday has been posted today 😄 |
|
@citibeth The way we ended up handling |
|
Another reason maybe not to add @adamjstewart:I actually think it should just be disabled by default for this reason. |
|
@tgamblin It is disabled by default. Packages will only run |
|
And yes, if the Makefile contains targets for both Although maybe this isn't such a good idea. In a lot of packages, |
|
Hold on, I'll try to clarify : the code that is triggered is in Then for def check(self):
"""Default test : search the Makefile for targets `test` and `check`
and run them if found.
"""
self._if_make_target_execute('test')
self._if_make_target_execute('check')for def check(self):
"""Default test : search the Makefile for the target `test`
and run them if found.
"""
with working_dir(self.build_directory()):
self._if_make_target_execute('test')These function can be overridden in packages and extended, see |
I don't agree because :
|
that's a few days old comment which is outdated by now, GitHub hiccups. p.s. I agree with your comments. |
|
Ah, sorry, same problem as me 😄 |
|
Should autoreconf and configure stages be merged?
|
I wouldn't go for that, but we could add a default behavior to |
TLDR : have a look at the
<name>/package.pyfiles to have an idea of what this PR does 😄Description
This PR allows sub-classes of
PackageBase(exPackage) to have an arbitrary number of phases.It also add a convenient syntax to register arbitrary methods as
preconditionsorsanity_checks, meaning that they will be run before or after the phases they are bound to.In the end it should give more flexibility for further development of features like the ones discussed in #169 or #1305 and reduce the boiler-plate code needed in many packages.
A few packages have been converted to exemplify some of the benefits of this refactoring.
Modifications
Packagehas been renamedPackageBase, it has logic to be extended by subclasses that are tailored to specific build systemsPackage: simple subclass having only aninstallphase (backward-compatible)EditableMakefile: examples inastyle/package.pyAutotoolsPackage: examples ingmp/package.pyand othersCMakePackage: examples inopenjpeg/package.pyand otherslog_outputhas been turned into a "double" context manager : the external context spawns a daemon that wait for things to write to a file, the internal plugstderrandstdoutto that fileos.fork(), exceptions are propagated from child process to parent #1228