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

Apply szip LINK_COMP_LIBS CmakeFilters.cmake patch from msys2 #2409

Closed
wants to merge 1 commit into from

Conversation

mkitti
Copy link
Contributor

@mkitti mkitti commented Jan 18, 2023

@Biswa96
Copy link

Biswa96 commented Jan 18, 2023

Warning: A valid patch in msys2 may not mean it can be applied upstream. Please check if the change is compatible with the expected behavior in all use cases.

Also, I guess that the condition in the patch would be better if it was written like this if (ZLIB_FOUND) ... else() ... endif().

endif ()
endif ()
if (ZLIB_FOUND)
set (LINK_COMP_LIBS ${LINK_COMP_LIBS} ${ZLIB_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

This line should just be added to the if block that is next. (line 67)

Copy link
Contributor

Choose a reason for hiding this comment

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

Look at line 94. I think this set (LINK_COMP_LIBS line should replace the one at line 94 as that is the end of any ZLIB_FOUND checking and ZLIB_LIBRARIES is set correctly no matter which path was taken.

Copy link
Contributor

@byrnHDF byrnHDF left a comment

Choose a reason for hiding this comment

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

Looking further at this, there is another problem - once pass this section and ignoring the External build section, there is another set LINK_COMP_LIBS command.

@byrnHDF
Copy link
Contributor

byrnHDF commented Feb 17, 2023

I will incorporate this into a new PR that will add FetchContent options that was implemented in hdf4 CMake.

@byrnHDF byrnHDF closed this Feb 17, 2023
@mkitti
Copy link
Contributor Author

mkitti commented Mar 7, 2023

I believe this was fixed by #2492

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.

3 participants