Skip to content

Fix build issue with ICPC when using CGAL #25843

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Marc-Pierre-Barbier
Copy link
Contributor

@Marc-Pierre-Barbier Marc-Pierre-Barbier commented Nov 5, 2024

Summary

The original code was adding "-fp-model strict" to all targets Regardless of the language which led to compilation issues with CMake Project that contained CUDA or C code (untesed with C).

Motivation

This issue made my cuda builds fail.

I didn't open an issue, as the work required to create a detailed issue is the same as fixing it directly.

Details

I simply repeated what was already done for other compilers but applied it to intel.

I didn't test on Windows or using older CMAKE <3.3, so be careful with the review.

As the conditions are copied and pasted from the other compilers and the linux & cmake > 3.3 test worked well. I believe it should work.

I also changed "fp-model strict" to "fp-model=strict" for more compatibility with Wrappers like nvcc which can transmit unknown arguments to the underlying compiler.
Relevant intel doc: https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/fp-model-fp.html


Sorry, something went wrong.

@CLAassistant
Copy link

CLAassistant commented Nov 5, 2024

CLA assistant check
All committers have signed the CLA.

The original code was adding "-fp-model strict" to all targets
Regardless of the language which led to compilation issues with CMake
Project that contained CUDA or C code (untesed with C).

Change fp-model strict to fp-model=strict for more compatibility with
Wrappers like nvcc.

Relevant intel doc: https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/fp-model-fp.html
@AbrilRBS
Copy link
Member

AbrilRBS commented Nov 6, 2024

Asking @jcar87 to take a look at this :)

@lrineau
Copy link
Contributor

lrineau commented Nov 7, 2024

Hi @Marc-Pierre-Barbier. The CGAL project does not officially support the Intel Compiler, but tries to improve the support for non-tested compilers anyway. Should not this fix be contributed upstream, so that it applies also to CGAL versions 6.0 or later?

@Marc-Pierre-Barbier
Copy link
Contributor Author

Hi @Marc-Pierre-Barbier. The CGAL project does not officially support the Intel Compiler, but tries to improve the support for non-tested compilers anyway. Should not this fix be contributed upstream, so that it applies also to CGAL versions 6.0 or later?

I saw the cmake code in the conanfile and thought it came from here, but if it came from upstream then I will create a pr there too.

Marc-Pierre-Barbier pushed a commit to Marc-Pierre-Barbier/cgal that referenced this pull request Nov 27, 2024
Related to conan's conan-io/conan-center-index#25843

replaced 'fp-model strict' by 'fp-model=strict' https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/fp-model-fp.html
for more compatibility with nvcc. as 'fp-model=strict' will be directly
transmited to the compiler while 'fp-model strict' will have 'strict'
being treated as a file input.

Also added a CXX only filter for newer cmake 3.3+
Marc-Pierre-Barbier pushed a commit to Marc-Pierre-Barbier/cgal that referenced this pull request Nov 27, 2024
Related to conan's conan-io/conan-center-index#25843

replaced 'fp-model strict' by 'fp-model=strict' https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/fp-model-fp.html
for more compatibility with nvcc. as 'fp-model=strict' will be directly
transmited to the compiler while 'fp-model strict' will have 'strict'
being treated as a file input.

Also added a CXX only filter for newer cmake 3.3+
@Marc-Pierre-Barbier
Copy link
Contributor Author

i just opened a pr upstream with the changes.

sloriot added a commit to CGAL/cgal that referenced this pull request Dec 4, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
## Summary of Changes

Related to conan's conan-io/conan-center-index#25843

replaced 'fp-model strict' by 'fp-model=strict'
https://www.intel.com/content/www/us/en/docs/cpp-compiler/developer-guide-reference/2021-8/fp-model-fp.html
for more compatibility with nvcc. as 'fp-model=strict' will be directly
transmited to the compiler while 'fp-model strict' will have 'strict'
being treated as a file input.

Also added a CXX only filter for newer cmake 3.3+

_Please use the following template to help us managing pull requests._

## Release Management

* Affected package(s): at least conan
@Marc-Pierre-Barbier
Copy link
Contributor Author

The Upstream pr has been merged!

Copy link
Contributor

@lrineau lrineau left a comment

Choose a reason for hiding this comment

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

Looks good. Why is this PR blocked?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants