Have you considered just changing the default value for openmp and exit with failure if openblas+openmp%clang is built on OSX?

that is another option, to do via raise InstallError('This package does not build on Mac OS X!').

@alalazo i updated PR as you suggested. But I would wait for a feedback from other guys.

i updated the PR, @eschnett : could you please check that it does not break the usage of MacPorts clang which does support OpenMP? (i don't have Macports myself).

@davydden I'll check.

Note that anybody can test this, as the current condition does not depend on whether one uses OS X or not. Testing with clang on Linux would answer the question as well.

Having said this, I don't think the test is right. This test will disallow all versions of clang up to and including 7.3.0. I don't think that Spack matches against the -apple suffix.

@eschnett i updated the test to @5.0.0-apple:7.3.0-apple. This would work for a while, at least until clang will get to version 5.

Funny, I just see this:

ifeq ($(C_COMPILER), CLANG)
$(error OpenBLAS: Clang didn't support OpenMP yet.)
CCOMMON_OPT    += -fopenmp
endif

That is, OpenBlas (!) hardcodes that OpenMP cannot be used with a compiler called clang. (This is OpenBLAS 0.2.17.) Given this, we should do the same in our package -- any compiler called clang (independent of the operating system) should be disallowed with +openmp.

That also means that the current check in package.py in this pull request is working fine.

so are you suggesting to change from

if spec.satisfies('%[email protected]:7.3.0-apple'):

to

if spec.satisfies('%clang'):

with an appropriate comment that Openblas hardcoded that OpenMP cannot be used with any clang?

@davydden Yes, exactly.

Also, OpenBLAS 0.2.18 has been released. Let me check whether it handles things differently -- if so, we will need your more complex condition after all.

ok...

No change with OpenBLAS 0.2.18; it still checks for clang.

Might as well add 0.2.18 as part of this PR then.

I agree that the easiest thing to do is probably just to remove the variant altogether and always build with OpenMP unless the compiler is %clang.

@eschnett @adamjstewart i updated the PR to do OpenMP always unless we use clang.

@davydden openblas~openmp is needed e.g. by multi-threaded applications whose logic is parallelized at a different level than lapack-blas

@alalazo: ok, i see. I will put back the variant...

Great. I guess let's keep it as a variant then? Would everyone like to vote on defaulting to True or False?

@davydden @adamjstewart Any default value for the variant would do for me.

@adamjstewart @alalazo @eschnett : updated the PR with a variant.

Looks good to me!

What if you revert the last change and open a new PR for this test?

will do.

@davydden Thank you very much

@alalazo for consistency, you may want to comment on this PR with which I was inspired with #812

@davydden Thanks for pointing me to that, I am quite behind the schedule of my notifications and missed #812 😄

@alalazo i move this commit to another PR. This one is clean again and should be ready to merge.

@tgamblin this is ready to merge 👍 😄

@tgamblin @trws ping-ping 😄

Changing the default seems reasonable. I'm coming to this late, but why have spack print an error rather than just let the package fail? Either way, the result is the same, but this way there will be more work to do in spack if and when they fix their clang+openmp handling right?

@trws: Openblas takes like 45 minutes to build. I would rather have Spack fail instantly than have Openblas fail much later. But what we could do is call compiler.openmp_flag. The flag itself wouldn't be useful, but it would be able to determine whether or not the compiler supports OpenMP and raise an error if it doesn't. Does that seem reasonable @davydden?

Is that functionality going in like that @adamjstewart? I'm all good with checking whether the compiler supports OpenMP and bailing early (really that's what their build system should do...), but encoding it the way they did just based on the compiler name feels wrong is all.

@adamjstewart Maybe I remember it the wrong way, but the clang+OpenMP combination fails immediatly...

Is that functionality going in like that @adamjstewart?

It will when #882 gets merged :)

@alalazo: That's good. I guess either one would be fine, but calling compiler.openmp_flag would produce a more useful error message and would be quicker than reading through a config.log. That's just my opinion on it.

I'll probably comment on #882, but having it be based on flags rather than capabilities feels odd. Build systems shouldn't count on spack to supply the right flag for a compiler like that, either way though checking functionality is a good thing.

@adamjstewart:

But what we could do is call compiler.openmp_flag.

That would rule out Apple's clang (after #882 is merged), which does not support openmp as of now, but we still need to fail with openmp+any clang as openblas assumes no clang can ever support openmp.

@trws

this way there will be more work to do in spack if and when they fix their clang+openmp handling right

correct. But I think it's still better to make it fail before with some reasonable message. Otherwise it fails to configure (hopefully during configure and not compilation) and then the user needs to dig into logs to see what's wrong.

Ah, you're right. In that case, I'm good with the way things are now.

It does, for now, but they will eventually regain their sanity and fix their build system. I'd rather not have to have someone come back in and special-case this again because of it.

@trws so you suggest to keep openmp false by default and let the user debug the logs in case he tries to compile with openmp and clang?

Either that, or do an actual check for whether it does or does not support OpenMP, yes.

whether it does or does not support OpenMP, yes.

by it you mean openblas or compiler? The current check is essentially an unconditional check for openblas.

I think Spack should contain logic to help people navigate broken build systems. If ./configure; make; make install worked fine, then there'd be little need for a tool such as Spack. But this is the real world, and things are broken and require work-arounds, and Spack should help people as much as it can. These helpers will necessarily be dirty and system-specific and version-specific, but there's no other place to put them but in Spack.

If all it takes is a single if statement to make things easier for myself down the line when I forgot about this conversation, then please, add it, even if it needs to be changed in the future.

i agree with @eschnett and would suggest to keep it the way it is now with a code comment which explains the problem and a readable error message for a user.