@eschnett @tgamblin @nrichart ping.

--no-tests looks very useful, and I agree that the default should be to run tests. But I would negate the internal name. Most of the time, install testing will look like:

if self.run_tests:
    make('check')

No sense in having to negate it every time.

@adamjstewart agree, will do.

It looks fine to me.

@davydden @adamjstewart @nrichart: I like this in theory but i think the right way to do it is to put another method on package.py called run_tests() (or similar), and have do_install() call that.

That would also open up the possibility of independently running tests outside the install process. I think it would actually be nice if spack test became spack self-test and a new spack test <spec> command were introduced to run the tests for a specific package.

How opposed would you all be to implementing that? I thin it's not actually that much more work than this PR.

@tgamblin I agree. I also think we need to have the idea of a pre-install test and a post-install test. Many packages comes with a make check or make test, which is very helpful. The only problem is that it can sometimes take quite a while to run, and may not complete successfully on every computer. Here is what I propose. We should separate install() into several components. For the most basic Autotools package, it would look something like:

def configure(self, spec, prefix):
    configure('--prefix={0}'.format(prefix))

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

def build_test(self, spec, prefix):
    make('check')

def install(self, spec, prefix):
    make('install')

def install_test(self, spec, prefix):
    make('installcheck')

For more complicated packages, install_test can be hand-written and include some of the tests in hdf5 for example. Of course, not all build systems will have every one of these steps, and we don't want to have to change all of the packages currently in Spack, so if we see these functions, we run them, and if we don't, we skip them. Anyway, by default, Spack should run all of these during installation. If --no-tests is supplied, we skip build_test and install_test. If we run a new spack test command, it would run install_test only. Thoughts?

@adamjstewart: I like it.

Note that @citibeth has actually already done much of this in her StagedPackage which was merged in #543. I think most of her default support is for CMake, and I think we can build on that to get this type of integration for other packages. I am not quite as concerned about having her spack setup support everywhere, but the stuff you mention would be nice to have in general for autotools, etc. Adding command line integration (spack configure, spack build, etc.) would be great as well, esp. for package debugging.

A few things:

1. I think making things backward-compatible with existing spack packages is pretty easy -- the packages that only define install() can remain unmodified until we fix them; they just won't have configure, build, or test phases.
2. I am worried about running tests by default, as not all clusters launch things the same way (a huge problem in HPC IMHO). There's not always mpirun available, and centers are particular about how you run jobs. Sometimes you have to submit a script via, say, MOAB. I would almost rather have install run tests only if you supply --tests, as I suspect that tests are going to fail on many systems given the state of HPC scheduler/resource manager portability. What do you think?
3. In my ideal universe, the tests would be run automatically in a CI environment, and users would NOT have to run tests on install. I hope to have something like that running at LLNL soon. So what if we make the default no tests?

I am worried about running tests by default, as not all clusters launch things the same way (a huge problem in HPC IMHO). There's not always mpirun available, and centers are particular about how you run jobs. Sometimes you have to submit a script via, say, MOAB. I would almost rather have install run tests only if you supply --tests, as I suspect that tests are going to fail on many systems given the state of HPC scheduler/resource manager portability. What do you think?

👍

Hmm, I see your point. I agree that we shouldn't be running MPI jobs across multiple nodes by default. But some tests (make test, make installcheck, sanity checks, and the check_install in hdf5) would be fine to run. Is there some way we could distinguish those?

@citibeth's StagedPackage would work fine. I think we should make StagedPackage -> Package. Like you said, for backwards compatibility, if packages only have an install, we only run that. I also think spack create and the documentation should be changed to support this new style.

I am fine with making tests false by default. Indeed, MPI-tests will most likely fail in HPC environment. To fix the original issue, i would suggest we stick for a moment with the current solution which does not involve creating a separate run_tests() function. The latter can be addressed by other individuals who are willing to take a shot at it 😄

@adamjstewart one could have --run-tests and --run-mpi-tests, to separate the two, but i think it will be confusing for users. So i would just stick with a single --run-tests even though some tests are serial and therefore should work fine on HPC.

But some tests (make test, make installcheck, sanity checks, and the check_install in hdf5) would be fine to run. Is there some way we could distinguish those?

Not generally. What about the cross-compiled case (blue gene?). Those commands won't actually work either. Typically on Cray you can get away with running backend-compiled stuff on the frontend, but not always.

That said, the new architecture support actually does distinguish backend (compute) and frontend (login) architectures on a platform. So you can tell if you're building for one or the other. You could skip the tests or force an mpirun if you discover you're building a backend binary. Really we need a more general around test running, and platforms need to define how you would run tests on them. I think this is a much larger project, though.

@citibeth's StagedPackage would work fine. I think we should make StagedPackage -> Package. Like you said, for backwards compatibility, if packages only have an install, we only run that. I also think spack create and the documentation should be changed to support this new style.

👍

To fix the original issue, i would suggest we stick for a moment with the current solution which does not involve creating a separate run_tests() function. The latter can be addressed by other individuals who are willing to take a shot at it 😄

@davydden: how about we update this PR to add a --run-tests option to install, and we can accept the current PR as a stopgap. The --run-tests arg would stay the same after refactor and tests would be off by default.

@davydden: how about we update this PR to add a --run-tests option to install, and we can accept the current PR as a stopgap. The --run-tests arg would stay the same after refactor.

will do.

w.r.t. separating into configure(self, spec, prefix), build(self, spec, prefix), build_test(self, spec, prefix), install(self, spec, prefix), nstall_test(self, spec, prefix), i think it is too fine grain. What would be the gains for package developers? From my perspective it brings disadvantage of obscuring things, one needs to know the order and can't use the same variables defined in configure stage in build_test stage.

Seprating a build into a separate function does not make much sense for me as in 99% of cases there will be just make() there.

Also the order of build, isntall and build_test might be different. For example here it is: build, install and build_test. So i would say it is more flexible to keep a single function to do everything.

I would stick with the current install and additionally introduce test which is post-install tests.

@davydden: I'm also torn about this and kind of like the simplicity of install().

I'm not sure why we need build_test and intsall_test -- can we make do with just install_test?

I see a few pros and cons:

* Pro*

1. For package debugging, having separate configure/build/install/test is handy. I've wanted to just run configure a bunch of times and I've had to comment out/insert return statements in packages. Not that hard, but I like the separation.
2. There is interest from @gartung, @citibeth, our code teams, and various other collaborators at NASA, Fermi, etc. in making Spack more friendly to developers. e.g., right now it's an install tool. I think it could also manage deps for developers with some tweaks. Specifically, it could rebuild library dependencies when needed and handle dev environment setup for something outside of spack. This would likely benefit from more fine-grained control over the install pipeline.

Con

1. For first-time packagers, configure/build/install is more complicated than just install.

Con (1) is mitigated somewhat by @adamjstewart and @citibeth's proposals of more specialized package classes (I believe Homebrew does things like this) that could contain support for things like autotools and cmake, and would mean you probably don't have to implement anything but configure, and maybe not even that. For many packages, for example, you might just have a property in the package that tells it the args to use for configure. Depends on the package.

I don't think the changes we're proposing here require people to implement configure, build, and install. If a package just has install, we could assume it's a "simple" package and just run that. Enterprising packagers could, of course, update simple packages with the new methods, and I guess the packages could then stay in their "advanced" state.

What do you think?

I'm not sure why we need build_test and intsall_test -- can we make do with just install_test?

some tests like make(check) require build folder to run. Others, like compiling and running an example program, can be done post-installation (assuming that the .cc example files are installed into share or alike).

Con (1) is mitigated somewhat by @adamjstewart and @citibeth's proposals of more specialized package classes (I believe Homebrew does things like this) that could contain support for things like autotools and cmake, and would mean you probably don't have to implement anything but configure, and maybe not even that.

I thinks that's what hashdist does with its stages, whereas homebrew is a single install IIRC. Having worked with both, i prefer a homebrew's single install, as it is less obscure and more flexible.

As @davydden said, build_check() and install_check() are very different things. Especially for developers who want to be able to confirm whether or not their package built correctly without installing it first. That's why Autotools separates make check and make installcheck. Another reason is that most packages come with some kind of make test or make check (which depends on the build directory to run), but only a few have make installcheck. The only reason we can't do everything with install() and test() is because build_check() needs to come after build() and before install().

don't think the changes we're proposing here require people to implement configure, build, and install. If a package just has install, we could assume it's a "simple" package and just run that. Enterprising packagers could, of course, update simple packages with the new methods, and I guess the packages could then stay in their "advanced" state.

I'll try to push this a little further... what about implementing in spack.Package the logic to execute an arbitrary number of phases based e.g. on an iterable attribute. We may then :

• have constraints on phases (e.g. install should be present among the stages)
• specialize spack.Package for a number of build systems (generalizing the work done by @citibeth and @adamjstewart)

This will allow a packager to be as flexible as we are now (or even more) and will permit more concise logic if the package is well behaved.

As the phases will be named we may then have a command like :

spack manual-install --phase <name> <spec>

to stop after phase name of the installation.

@alalazo If you have an arbitrary number of phases, how do you tell Python which of them to run and in what order?

@adamjstewart simple metaprogramming in spack.Package and ordered iterable...

@adamjstewart the simplest example I could conceive on the language mechanism

@tgamblin i switched to --run-tests.

I prefer a homebrew's single install, as it is less obscure and more flexible

Moving to sub-phases in #543 was done out of necessity: I needed a way to run what's now install_configure() without also running the rest of the install(). This enables the spack setup functionality.

For PR #543 I would admit that install_build() did not need to be separate from install_install(). Those two sub-phases could be re-combined without breaking anything (I doubt they've been used). I do think that adding another sub-phase for tests could be good, since it could then be turned on/off via install_phases. We are still at the point where we can make significant changes to this sub-phase system and not break much (yet). We should not let backwards compatibility with the current CMakePackage limit our designs. I like @alalazo's suggestions on arbitrary stages, although I'm not sure how it would work out in the end.

CMakePackage is specialized because it has CMake smarts built into install_build() and install_install(). When you subclass CMakePackage, you only have to write install_configure() in most cases.

I've had to comment out/insert return statements in packages. Not that hard, but I like the separation.

With what we have right now, this could be done in a permanent way for any package: subclass StagedPackage instead of Package, and then separate the existing install() into install_configure() and install_install().

On how Spack decides whether or not to use the sub-phases... Spack just calls the top-level install(). The method StagedPackage.install(), which overrides Package.install(), then calls the sub-phases. Any of these methods that you override, it will run your version. Any you don't override, it will run the standard. If you subclass StagedPackage for your package and implement an install() method, it will be as if the extra "features" of StagedPackage never existed

I think we should make StagedPackage -> Package

This would in one sense not break anything. But what happens when you run spack setup on a "legacy" package that just implements install(). An exception should be raised. How would this behavior be preserved if StagedPackage were merged into Package? Subclassing StagedPackage is a nice way for the developer to indicate that the sub-stages have been separated out for this Package. (There may be a solution to this issue: Ensure that all install_XXX() methods of StagedPackage raise an exception --- and then ensure that the default implementation for those sub-phases in CMakePackage do not. I'm not sure if this solution is 100% correct or desirable).

Yes, PR #543 is CMake-only. Autotools and maybe Python Setuptools are obvious extensions. But since I don't create projects with Autotools, I'm not in a position to do the work. I'm happy to help out any Autotools developers who want to see it happen.

Other comments on this PR:

1. Avoid negative logic. The flag should be called test, not no-test. (skip_patch should also be reversed to patch or do_patch).
2. Adding a flag to spack install just to pass parameters to a package seems like overkill to me. Other approaches to this problem would be:
   a) Use a variant.
   b) Create the idea of a package flag. A flag would be like a variatn, except it's not included in the hash.
   c) Have a general way of telling spack install which sub-phases you want to run.

a) Use a variant.

I agree with @alalazo here that +test variant will change the hash for no valid reason.

b) Create the idea of a package flag. A flag would be like a variatn, except it's not included in the hash.

would do, but this looks like an overkill to me as a simple flag to spack install does the job just fine and was also suggested elsewhere (see OP).

c) Have a general way of telling spack install which sub-phases you want to run.

That's not yet implemented, whereas we need something to fix current issues with packages which run tests and fail in HPC environment.

For who's interested, I started to sketch the design idea I was talking about yesterday in #1186

I thought a little more about this and I've come to agree with making "no tests" the default. A large portion of the packages I install, even simple GNU libraries, fail make check, even though they work fine. I would like the tests to still be in place for these packages, but obviously we don't want half of Spack to break on install. Personally I like -t, ---test better than --run-tests for the flag name.

Another consideration I thought of: It would be nice to run post-install tests without the Spack compiler wrappers. This would help us discover cases where we need to filter out the compilers.

@alalazo I took a look at #1186. I know arbitrary install phases are doable, but can you give me good use cases? Another one I thought of would be filter_compilers, which is pretty common.

@citibeth I'm not opposed to the idea of subclassing Package for each build system. I know you already have one for CMake, and you mentioned doing the same for Python to solve the RPATH issue. I'm still not sure whether we should make all of the stages I mentioned defaults in Package or integrate them into your StagedPackage. I can see merits for both.

Personally I like -t, ---test better than --run-tests for the flag name.

how about --with-tests? As i see it --test kind of suggest that it will only test, same for --run-tests. But again, i am not a native speaker so I am ok either way.

On Thu, Jul 7, 2016 at 5:17 PM, Adam J. Stewart [email protected]
wrote:

I thought a little more about this and I've come to agree with making "no
tests" the default. A large portion of the packages I install, even simple
GNU libraries, fail make check, even though they work fine. I would like
the tests to still be in place for these packages, but obviously we don't
want half of Spack to break on install. Personally I like -t, ---test
better than --run-tests for the flag name.

What if tests get run when available, with the results stored in a DB, plus
a warning? Users can later peruse the results of tests run at
install-time. If Spack does NOT run the tests, it's hard to see how users
would be able to do so after-the-fact.

@citibeth https://github.com/citibeth I'm not opposed to the idea of
subclassing Package for each build system. I know you already have one for
CMake, and you mentioned doing the same for Python to solve the RPATH
issue. I'm still not sure whether we should make all of the stages I
mentioned defaults in Package or integrate them into your StagedPackage. I
can see merits for both.

Whether you integrate StagedPackage->Package or not, I think that's a
separate issue from per-build system subclasses.

@citibeth Another reason I'm hesitant to run tests by default is that they can take very long (I'm currently installing something that took 30 seconds to build and has been testing for over an hour now).

There are two issues here:

1. Are tests run by default?
2. Does successful package installation depend on tests passing?

I'm suggesting we set up a framework so that package installation does NOT
depend on tests passing; but that tests are still run (if/when the user
desires).

On Thu, Jul 7, 2016 at 5:33 PM, Adam J. Stewart [email protected]
wrote:

@citibeth https://github.com/citibeth Another reason I'm hesitant to
run tests by default is that they can take very long (I'm currently
installing something that took 30 seconds to build and has been testing for
over an hour now).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1169 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AB1cd7c6w_rewZC9LXuT-EcpPmaHwSUtks5qTXC4gaJpZM4JFGX_
.

I think tests should not be run by default, but if users decide to enable them then installation should depend on tests passing.

The problem is that for some packages, the tests just don't work. Users
will figure that out, but won't know which packages they don't work for ---
so they will disable tests overall. (Remember that when you say spack install X, it install installs Y and Z.)

On Thu, Jul 7, 2016 at 10:34 PM, Adam J. Stewart [email protected]
wrote:

I think tests should not be run by default, but if users decide to enable
them then installation should depend on tests passing.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1169 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AB1cd2SXQgegm21UTq5XmBEZfVY6vsY0ks5qTbdJgaJpZM4JFGX_
.

Exactly, which is why tests should be off by default 😃

+1 for build tests passing be prerequisite for installation if the user asked them to run.

@tgamblin if there are no further comments on the --run-tests, can we merge it so that petsc is fixed in HPC environment by skipping mpi tests?

Ok I merged this. It would be nice to converge on how best to get a separate test method soon, e.g. though #1186 (though I haven't had a chance to look at that one).

What if tests get run when available, with the results stored in a DB, plus
a warning? Users can later peruse the results of tests run at
install-time. If Spack does NOT run the tests, it's hard to see how users
would be able to do so after-the-fact.

Given that some packages build really fast but the tests take forever, I think we ought to keep them off by default. I would like stuff like this (public package build results) to be done by CI / nightly testing.

I don't like the idea of continuing to hack out temporary fixes and integrate them into Spack. That just digs us deeper and gives us another thing to remove eventually, if we even remember to. If a package won't install, that's one thing. But if we're changing the main Spack library just to temporarily add some new functionality, then that's unnecessary. I think we should sit down and design a testing framework, then implement it.

@davydden What I meant by this, if I wasn't clear, is that we shouldn't add --run-tests or start using self.run_tests everywhere in Spack if we already plan on removing it and replacing it with the functionality in #1186. PRs like this and #1235 are just giving @alalazo more things to remove if/when #1186 is integrated. And if he doesn't update petsc and p4est for you, they will no longer work.

@adamjstewart nobody plans to remove --run-tests. #1186 will be backwards compatible anyway, so there is no problem using self.run_tests inside install() or, when multistages are there, refactor each package into different stages. In other words, i don't see any conflicts here nor more things to remove.

Once #1186 is merged, the correct way to support testing will be to subclass AutotoolsPackage and implement check(). If you are modifying a package to make use of self.run_tests, then you are doing something that should be undone once #1186 is merged. I just don't want to see you (or anyone who has to undo it) have to do too much work, or have too much churn in packages.

@adamjstewart In #1186, at the moment, tests are just another sanity_check that is run after a phase, e.g. :

@CMakePackage.sanity_check('build')
@CMakePackage.on_package_attributes(run_tests=True)
def check(self):
    make('test')

just means that the function check is being registered to be run after the build phase only if the package has the attribute run_tests and its value is True.

I tried to maintain complete backward compatibility so far 😄

EDIT : just to be completely clear, the function check could be named in any way you want, and sanity_checks are not reserved for tests. Maybe I should rename them postconditions if the PR is considered for merging...

@alalazo Hmm, is there any way we can get by with:

def check(self):
    make('test')

Instead of:

@CMakePackage.sanity_check('build')
@CMakePackage.on_package_attributes(run_tests=True)
def check(self):
    make('test')

The point of the package.py is to abstract out as much of the complicated stuff as we can so you don't even need to know Python to add a new package. Personally, I don't even know how Python decorators work.

Once #1186 is merged, the correct way to support testing will be to subclass AutotoolsPackage and implement check().

agree

If you are modifying a package to make use of self.run_tests, then you are doing something that should be undone once #1186 is merged.

don't agree. #1186 is backward compatible, so all packages eventually are going to be refactored into stages if/when it is merged. From that perspective using if self.run_tests inside install() is nothing and I would not call it un-done.

I just don't want to see you (or anyone who has to undo it) have to do too much work, or have too much churn in packages.

moving packages to stages aint gonna happen in 1 day, the fact that if self.run_tests is used inside install() will not make a slightest difference in this process.

The point of the package.py is to abstract out as much of the complicated stuff as we can so you don't even need to know Python to add a new package

IMHO in most cases this is simply copy-pasting, so if the author of a new package see another one with

@CMakePackage.sanity_check('build')
@CMakePackage.on_package_attributes(run_tests=True)
def check(self):
    make('test')

he will just copy paste it into its own and define appropriate checks within this function. So i personally like the current design of #1186.

p.s. As a matter of fact before i started contribute to Spack i new nothing about python.

Copy-pasting is fine, but not as fun as not-copy-pasting. If build and install don't need any fancy decorators, I don't see why we can't make check work without decorators.

Does this warrant a new test deptype?

Does this warrant a new test deptype?

Yes! For example, GCC requires DejaGnu, Tcl, and Expect to run its testsuite:
https://gcc.gnu.org/install/test.html

In terms of linking, it should be pretty similar to the build deptype. The only difference is that it isn't required unless someone installs with --run-tests.

Does this warrant a new test deptype?

👍, as an example some packages might require for testing numdiff to have clever comparison of output floating precision numbers ignoring small numeric differences. But this is certainly not a build/link/run dependency.

@mathstuf opened #1279 to talk about this further.