Check the installed HDF5 library for consistency by eschnett · Pull Request #812 · spack/spack

eschnett · 2016-04-20T20:38:25Z

I added a routine to the HDF5 install script that checks the installed library. This is not the self-check that HDF5 itself can perform as part of its build; rather, this tests whether Spack installed the library in such a way that its customers (other packages etc.) find a self-consistent and usable HDF5 library.

In particular, this routine builds and runs a small C program that uses the HDF5 library, thus checking include, library, and run time path settings. It also checks the version numbers, both via preprocessor constants (from include headers) and via function calls (from the library found at run time), and compares these to the expected version numbers. A mismatch leads to an error.

I think such a system is generically useful for many packages. I'm suggesting to add this to HDF5 as a test balloon, and if it works, it can be cleaned up, made part of Spack, and generally be used for many other packages as well.

As a side node, I think packages could define environment variables that define the list of library names that should be used to link against a library; in this case, HDF5 could define e.g. the equivalent of HDF5_LIBS='-lhdf5' or similar. This is just a note, this is not implemented here.

eschnett · 2016-04-20T20:56:44Z

The Travis failure looks unrelated.

adamjstewart · 2016-04-20T21:01:10Z

Perhaps we should also add make("test")

eschnett · 2016-04-20T21:03:17Z

@adamjstewart Yes, make test makes sense as well -- but that should be optional because it is often quite slow, and/or fails spuriously. (I think HDF5's self-test checks extended file system attributes that don't work over NFS.) It's a very different test from the one I do (although also very important).

tgamblin · 2016-04-22T02:34:07Z

The travis failure is indicating that the new HDF5 package failed the Python 2.6 compatibility check:

======================================================================
FAIL: test_package_module_compatibility (spack.test.python_version.PythonVersionTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/LLNL/spack/lib/spack/spack/test/python_version.py", line 104, in test_package_module_compatibility
    self.check_python_versions(*self.pyfiles(spack.packages_path))
  File "/home/travis/build/LLNL/spack/lib/spack/spack/test/python_version.py", line 96, in check_python_versions
    self.assertTrue(len(all_issues) == 0)
AssertionError: 
-------------------- >> begin captured stdout << ---------------------
File                                                         Reason
----------------------------------------------------------   --------------------
spack/var/spack/repos/builtin/packages/hdf5/package.py:180   subprocess.check_output

Just change the subprocess call.

tgamblin · 2016-04-22T02:38:26Z

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

+               "-L%s" % join_path(spec.prefix, "lib"), "-lhdf5",
+               "-lz")
+            try:
+                output = subprocess.check_output("./check")


Suggest either using Popen or:

check = Executable('./check') output = check(return_output=True)

alalazo · 2016-04-25T11:47:26Z

@eschnett Can we factor to some place in the framework (if not already present) functions like :

def try_compile(sources, ...):
    ...
    return executable

def try_execute(executable, ...):
    ...
    return output

and use them here to have a concise description of the tests within a package? Also, can we keep C code in C sources, instead of raw string literals?

Per discussion in #806 with @davydden I like the idea of having tests to ensure that things were compiled correctly, but I think the amount of boilerplate will be too much if we start to repeat (i.e. cut and paste) this all over the place. FWIW, I can help with the implementation if you don't have time to do that.

eschnett · 2016-04-25T14:00:05Z

@alalazo Splitting things up into functions is a nice idea. I'm sure they will evolve over time, since people will also want to build C++ and Fortran source code.

A third function might be compare_output.

C code in C source files: Ideally, these tests run after the library has been installed, and after the staging directory has been removed (or renamed), to ensure that nothing accidentally points back into the staging directory. The install tests should run in their own directory. How would I copy the C sources and the expected output into a new directory?

Yes, I might be busy this week with other things. I would be happy if you were to take a stab at refactoring my checking routine. If so, it might be easiest if you opened a pull request against origin, not against my repository.

davydden · 2016-04-25T14:05:42Z

A third function might be compare_output.

👍

tgamblin · 2016-05-10T08:34:32Z

@eschnett @davydden: merged -- do you guys want to refactor as discussed?

davydden · 2016-05-10T15:40:14Z

@tgamblin in which file/where would you put that functionality?

tgamblin · 2016-05-10T15:43:36Z

how about lib/spack/spack/testing.py or lib/spack/spack/package_test.py? They should also be added to the __all__ list for from spack import * at the bottom of __init__.py.

robertdfrench · 2016-05-11T13:31:23Z

Is the intention of package tests that they run on the same host which builds the packages?

davydden · 2016-05-11T13:37:55Z

@robertdfrench for now, i would say yes. But i think this discussion belongs to a separate thread/issue.

tgamblin · 2016-05-11T19:44:00Z

Once we have the newarch support it should be easy to get the frontend for the build platform, even if the build itself is for the backend. So we can revisit the tests for cray and BG then

* highfive pinned to release brion only * eyescale deps only for 3.1.0 * remove lunchbox from sonata-network-reduction in bbp-packages.yaml * disable subproprojects for emsim * choosePython.cmake path can vary * emsim to continue using [email protected] * brion patch in brayns * style in brion package.py * reorder brayns@immersive deps

Check the installed HDF5 library for consistency

7c155f7

Remove outdated comment

867e133

tgamblin reviewed Apr 22, 2016
View reviewed changes

Don't use subprocess module

0cbaecc

davydden mentioned this pull request Apr 25, 2016

openblas: do not use OpenMP with Clang #806

Merged

davydden mentioned this pull request Apr 26, 2016

encapsulate and run small unit tests for 'xyz' in provides('xyz') #835

Closed

tgamblin merged commit a1ebdf6 into spack:develop May 10, 2016

davydden mentioned this pull request May 11, 2016

Features/package_test #937

Merged

eschnett deleted the eschnett/hdf5-check-install branch June 8, 2016 14:20

Conversation

eschnett commented Apr 20, 2016

Uh oh!

eschnett commented Apr 20, 2016

Uh oh!

adamjstewart commented Apr 20, 2016

Uh oh!

eschnett commented Apr 20, 2016

Uh oh!

tgamblin commented Apr 22, 2016

Uh oh!

tgamblin Apr 22, 2016

Choose a reason for hiding this comment

Uh oh!

alalazo commented Apr 25, 2016

Uh oh!

eschnett commented Apr 25, 2016

Uh oh!

davydden commented Apr 25, 2016

Uh oh!

tgamblin commented May 10, 2016

Uh oh!

davydden commented May 10, 2016

Uh oh!

tgamblin commented May 10, 2016

Uh oh!

robertdfrench commented May 11, 2016

Uh oh!

davydden commented May 11, 2016

Uh oh!

tgamblin commented May 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants