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

Removed leftover setting of CMAKE_REQUIRED_FLAGS #4322

Merged
merged 1 commit into from
Nov 16, 2024

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Nov 14, 2024

Back in 88d91f7 I removed a check_c_source_compiles to detect some pthread stuff and converted to std::mutex instead.

This stuff I believe I should have removed back then too.

The -Werror caused a problem in VTK, which still has the check_c_source_compiles, because a totally unrelated warning made the try-compile fail when it should have passed.

@seanm
Copy link
Contributor Author

seanm commented Nov 14, 2024

This I think I should have done back in #3425.

The Werror removed in this PR is causing me grief in VTK: https://gitlab.kitware.com/vtk/vtk/-/issues/19532

if(MSVC)
set(CMAKE_REQUIRED_FLAGS "${CMAKE_C_FLAGS} /WX /W4")
else()
set(CMAKE_REQUIRED_LIBRARIES m)
Copy link
Member

Choose a reason for hiding this comment

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

we should probably keep set(CMAKE_REQUIRED_LIBRARIES m)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the whole block isn't there just for the now-removed check_c_source_compiles?

Copy link
Member

Choose a reason for hiding this comment

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

hum, I see there were past math function detection in 2622d38, but they have gone to now. So, probably disregard my comment and this is indeed no longer needed

@rouault
Copy link
Member

rouault commented Nov 14, 2024

your pull request is based on a not so recent master. Can you please rebase it on top of latest master HEAD ?

Back in 88d91f7 I removed a `check_c_source_compiles` to detect some pthread stuff and converted to std::mutex instead.

This stuff I believe I should have removed back then too.

The -Werror caused a problem in VTK, which still has the `check_c_source_compiles`, because a totally unrelated warning made the try-compile fail when it should have passed.
@seanm
Copy link
Contributor Author

seanm commented Nov 14, 2024

your pull request is based on a not so recent master. Can you please rebase it on top of latest master HEAD ?

Weird, I swear I did, but indeed not. Should be fixed now.

@rouault rouault added the backport 9.5 Backport to 9.5 branch label Nov 16, 2024
@rouault rouault merged commit c936171 into OSGeo:master Nov 16, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 9.5 Backport to 9.5 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants