typo: th -> the

Good catch

You should remove these debug statements.

D'oh.

I think these are handled automatically by the compiler wrappers?

As of the time of the pull request, CMakePackage wasn't passing the flags forward. I wasn't entirely sure if that was a bug or not.

FWIW, the developers of qmcpack provide their own logic for deciding on processor flags that includes the equivalent of -march=native in CMake/${Compiler}.cmake that are possibly bad ideas in multi-architecture environments, but the Intel version at least is smart enough to not override Spack if CMAKE_C_FLAGS includes -x or -ax and is cumulative for GNU.

Are you sure they aren't being passed forward? They won't appear in the logs, but they should be added through the compiler wrappers.

Pretty sure. I'll give things another go in a bit - I was getting architectural mismatches until that was added.

FYI. I think that I saw other Spack packages that use CMake doing similar things.

@becker33 Can you take a look at this? If every package has to manually access the flags added in compilers.yaml, then they aren't very helpful.

@naromero77 There are currently no packages in Spack that access self.compiler.flags.

@adamjstewart You are right, my own poor recollection.

The package should be getting its flags through CFLAGS, CXXFLAGS, etc environment variables. If you want them to be injected through the compiler wrapper instead, overwrite the default_flag_handler method or the specific cflags_handler method or equivalent for another flag. The documentation of flag handlers should give you the idiom for passing them to the compiler directly, it is

def default_flag_handler(self, env, flag_val):
    return flag_val[1]

If you want to send them to a different environment variable (if your cmake is reading different variables than expected) then you can override the flag handlers for that as well.

@adamjstewart @naromero77 does this answer your questions?

@wscullin just making sure william is still following.

Definitely don't want these in here. self.compiler.cc is the actual compiler, so you would be bypassing Spack's compiler wrappers. These should already be set, so you can remove the else clause.

Can you elaborate on "technical difficulties"? Just curious.

Will clarify the text. This is on the qmcpack end. The libraries qmcpack generates lack headers, but an include directory is created... with a header for fftw.

Relative paths are fine. prefix now supports arbitrary sub-directories. prefix should already exist. What about:

def install(self, spec, prefix):
    install_tree('manual', prefix.manual
    install_tree('nexus', prefix.nexus)
    
    with working_dir(self.build_directory):
        install_tree('bin', prefix.bin)

Fixing this.

I've never seen this before in a package. What is self.ctest? By default, Spack runs make test. Is this different than ctest?

I'll have to ask @naromero77 for details.

Yes, self.ctest runs ctest at the command line. It is different from make test, but in the same spirit. It is a builtin function/method in the Spack CMake Python module. Actually, its not robust enough for what I want to do. As ctest should be able to take command line arguments which the Python module does not support.

Regarding the large patch file for QE. Didn't realize it was that big. The patch file is also part of the QMCPACK distribution -- instead of committing the patch into Spack can we have Spack patch QE using the patch file from the downloaded QMCPACK distribution. I think William and I were just trying to follow the convention of other Spack packages.

It is a builtin function/method in the Spack CMake Python module.

You mean in CMakePackage? I don't see it anywhere.

@adamjstewart It's in build_environment.py

Ah, I see.

As ctest should be able to take command line arguments which the Python module does not support.

Actually, it can take command-line arguments, and you don't need self:

ctest('VERBOSE=ON')

We don't actually do line-length checks on variants (or any other directive).

sorry, not following, what is the requested action here?

Not necessarily requesting a change, but I noticed that you changed all the variants from being on a single line to multiple lines (presumably to make sure that it is under 80 characters). I'm just saying that you don't need to do this as we ignore variants.

Ah, travis seemed upset about PEP8 compliance via Flake.

Is the patch compatible with 5.3.1? What about 5.2.0?

As far as I know, it is only compatible with 5.3.0.

Then you either want:

conflicts('+qmcpackconv', when='@5.3.1:')
conflicts('+qmcpackconv', when='@:5.2.999')

or no conflicts() at all. Unfortunately there is currently no not in the spec syntax.

ok, simple to fix. Yes, there did not seem to be a syntax for only use this version.

You should also add depends_on('hdf5', when='+qmcpackconv'). Otherwise you can't do spack install espresso +qmcpackconv ^[email protected].

This can be on a single line. We don't do line-length checks on patches.

ok

You should also add depends_on('hdf5') here. Otherwise, you can't do spack install qmcpack ^[email protected]. Concretizer bug.

ok

Same here, add depends_on('fftw').

ok

This should go in a patch() function.

@wscullin William, can you take care of this?

@naromero77 certainly.

This text is duplicated below. Let's just put it in the install() docstring.

ok

looks like here you ingnore altogether any options used in intel-mkl:

    ilp64 [off]       True, False       64 bit integers
    threads [none]    openmp, none      Multithreading support

that's why packages should really use libs() interface.

if you use CMake, can you not explicitly set Blas/Lapack libraries, like for example here. Of course, it depends on how CMake is being used inside...

the same is with other Blas/Lapack, say the build system will be free to pick up single threaded or multithreaded Atlas libs, regardless of package settings:

threads [none]    pthreads, none    Multithreading support

I think QMCPACK is not compatible with threaded BLAS. This may be for legacy reason related to the OpenMP runtime which may no longer be true. We only support sequential without 64-bit integers.

We only support sequential without 64-bit integers.

you need to add conflicts, something like:

conflicts('^intel-mkl threads=openmp')

to make sure users don't use qmcpack in a stack of software with multithreaded blas.

if you use CMake, can you not explicitly set Blas/Lapack libraries, like for example here. Of course, it depends on how CMake is being used inside...

if you set Blas/Lapack explicitly, you won'd need to bother with this part at all.

The way CMAKE in QMCPACK is designed we use the built-in support for finding MKL. I believe this works with the Intel compiler without any extra effort. The complication comes when using non-Intel compilers with MKL. I will take a closer look at those references, thanks.

The way CMAKE in QMCPACK is designed we use the built-in support for finding MKL

by built-in, you probably refer to find_package, then it should possible to disable the whole search and explicitly specify blas/lapack.

@davydden Here is the issue which I am not sure how to fix. My understanding is that with a non-Intel compiler, the MKL library directory must be included in the CMAKE_PREFIX_PATH -- otherwise, the built-in CMake (part of CMake, not part of QMCPACK) won't properly detect MKL.

the MKL library directory must be included in the CMAKE_PREFIX_PATH

it's up to how CMake is being used. But certainly not a requirement! I regularly compile with GCC+Intel-MKL at least these two package: trilinos and dealii and everything works as expected.

@naromero77 if you directly call FIND_PACKAGE(LAPACK), then try following the way we configure dealii in spack: <package>_FOUND=TRUE should disable any search and just provide the libs using Spack's interfaces to blas/lapack.