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

Building with Clang on Windows #300

Merged
merged 25 commits into from Jul 23, 2020
Merged

Building with Clang on Windows #300

merged 25 commits into from Jul 23, 2020

Conversation

shibatch
Copy link
Owner

@shibatch shibatch commented Apr 2, 2020

With this patch, the library can be built with Clang on Windows.

@shibatch shibatch requested a review from fpetrogalli Apr 2, 2020
Configure.cmake Outdated
if (NOT LIBM)
set(LIBM "")
endif()
if (NOT SLEEF_CLANG_ON_WINDOWS)
Copy link
Collaborator

@fpetrogalli fpetrogalli Apr 3, 2020

Choose a reason for hiding this comment

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

I don't get this - why do you need to customize this for clang on windows? If the libraryes are not available, the variables below (LIB_MPFR, LIBFFTW3, ..) would evaluate to false and teh corresponding compilation flags will not be set. Is that breaking things?

Copy link
Owner Author

@shibatch shibatch Apr 3, 2020

Choose a reason for hiding this comment

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

CMake finds libraries for Cygwin, and this causes build errors.
We need to check the builds with Cygwin testers.

Copy link
Collaborator

@fpetrogalli fpetrogalli Apr 7, 2020

Choose a reason for hiding this comment

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

May I ask you to add this explanation as a comment? It will help people like me who doesn't know much about compiling on windows in understanding why there is this if not clang_on_windows condition.

Copy link
Owner Author

@shibatch shibatch Apr 7, 2020

Choose a reason for hiding this comment

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

I added the comment.

Configure.cmake Show resolved Hide resolved
@@ -60,7 +60,7 @@ foreach(SIMD ${SLEEFQUAD_SUPPORTED_EXT})
endif()
endforeach()

if(MSVC OR MINGW AND WIN32)
if(MSVC OR SLEEF_CLANG_ON_WINDOWS OR MINGW AND WIN32)
Copy link
Collaborator

@fpetrogalli fpetrogalli Apr 7, 2020

Choose a reason for hiding this comment

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

Why is this condition different from the one if((MSVC OR MINGW AND WIN32) OR SLEEF_CLANG_ON_WINDOWS) 25 of src/libm/CMakeLists.txt?

Copy link
Owner Author

@shibatch shibatch Apr 7, 2020

Choose a reason for hiding this comment

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

Those two are now same.

Configure.cmake Outdated
string(COMPARE NOTEQUAL "" "$ENV{TRAVIS}" RUNNING_ON_TRAVIS)
string(COMPARE NOTEQUAL "" "$ENV{APPVEYOR}" RUNNING_ON_APPVEYOR)
Copy link
Collaborator

@fpetrogalli fpetrogalli Apr 7, 2020

Choose a reason for hiding this comment

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

Please make these variables to be set at cmake invocation, as in cmake -DRUNNING_ON_TRAVIS=On .... It is safer than having to deal with things breaking in the case the name of the env variable change.

Copy link
Owner Author

@shibatch shibatch Apr 7, 2020

Choose a reason for hiding this comment

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

Done.

@shibatch shibatch merged commit 9b89c53 into master Jul 23, 2020
5 checks passed
@shibatch shibatch deleted the Clang_Windows branch Jul 23, 2020
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

2 participants