Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add is_clang_based compiler property #43376

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

alexrichert
Copy link
Contributor

This PR adds a property to compilers called is_clang_based, and sets it to True for clang-based compilers. This will save us copying and pasting lists of clang-based compilers across recipes when clang-specific compiler flags etc. are needed.

@spackbot-app spackbot-app bot added compilers core PR affects Spack core functionality tests General test capability(ies) labels Mar 26, 2024
@alexrichert
Copy link
Contributor Author

FYI @bernhardkaindl

@pranav-sivaraman
Copy link
Contributor

I like this! Do the IBM Clang compilers also apply? However, I think Spack doesn't recognize them at this moment?

@alexrichert
Copy link
Contributor Author

@pranav-sivaraman I added the property with return value True for all the clang compilers present to the best of my ability, so if/when merged, the same should be done for any other clang-based compilers added in the future. I didn't see the clang version of the IBM XL compilers in the configurations, if that's what you're referring to specifically, so yes as far as I know those are not supported yet.

@haampie
Copy link
Member

haampie commented Mar 27, 2024

but what does it mean? :) you need to document how this is supposed to be used.

@alexrichert
Copy link
Contributor Author

@haampie just added it to packaging_guide.rst, let me know if there's an appropriate place to put it in addition or instead.

@haampie
Copy link
Member

haampie commented Mar 27, 2024

Documenting its existence in insufficient. The question is what is guaranteed by compiler.is_clang_based.

@alexrichert
Copy link
Contributor Author

Can you elaborate on what you mean by guaranteed, and where that documentation should go?

@alalazo
Copy link
Member

alalazo commented Mar 28, 2024

Can you elaborate on what you mean by guaranteed,

Take for instance Apple Clang. It has explicitly disabled direct use of -fopenmp, while upstream clang obviously supports the flag. I guess other clang based compilers might have their own differences too? So, what is guaranteed to be valid for all these compilers when the function returns True?

Can you show where you would like to use this function?

@alalazo
Copy link
Member

alalazo commented Mar 28, 2024

Having a link to #43357 (review) would have helped getting the context

@alexrichert
Copy link
Contributor Author

Thanks for clarifying. Yes, that boost issue was the immediate motivation here. Other places where I think it's likely to be useful would be the few dozen packages that have some kind of -Wno-error= setting inside if self.spec.satisfies('%clang/oneapi/apple-clang/etc.') where it's almost certainly the case that the same setting will be needed for all Clang-based compilers. Or in the case of the hdf5 recipe for example, if spec.compiler.name in ["gcc", "clang", "apple-clang", "oneapi"] could be something like if spec.compiler.name=='gcc' or self.compiler.is_clang_based so to avoid keeping a running list of Clang-based compilers.

sethrj
sethrj previously approved these changes Apr 26, 2024
Copy link
Contributor

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this will clean up a fair number of packages and prevent further errors.

It looks like rocm doesn't have a compilers test?

@alexrichert
Copy link
Contributor Author

It appears not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compilers core PR affects Spack core functionality documentation tests General test capability(ies)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants