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

cmake: allow overriding GME_ZLIB and GME_UNRAR options by user #632

Merged
merged 2 commits into from
Sep 5, 2024

Conversation

mechakotik
Copy link
Contributor

I am using vendored SDL_mixer in my project and I need zlib support in libgme to load compressed VGMs. Current SDL_mixer CMake config disables it and doesn't allow user to enable it.

@sezero sezero requested a review from madebr September 5, 2024 20:22
CMakeLists.txt Outdated
Comment on lines 704 to 709
if(NOT DEFINED GME_ZLIB)
set(GME_ZLIB OFF)
endif()
if(NOT DEFINED GME_UNRAR)
set(GME_UNRAR OFF)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this work for you too?

Suggested change
if(NOT DEFINED GME_ZLIB)
set(GME_ZLIB OFF)
endif()
if(NOT DEFINED GME_UNRAR)
set(GME_UNRAR OFF)
endif()
set(GME_ZLIB "OFF" CACHE BOOL "Build GME with zlib support")
set(GME_UNRAR "OFF" CACHE BOOL "Build GME with unrar support")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't work for me. I am setting GME_ZLIB from the project CMakeLists.txt before including SDL_mixer.

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to do the following in your cmake script:

set(GME_ZLIB ON CACHE BOOL "GME with zlib" FORCE)
set(GME_UNRAR ON CACHE BOOL "GME with unrar" FORCE)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now it works, but as user just writing:

set(GME_ZLIB ON)

seems like more intuitive way for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one has a better behavior, I think.

Suggested change
if(NOT DEFINED GME_ZLIB)
set(GME_ZLIB OFF)
endif()
if(NOT DEFINED GME_UNRAR)
set(GME_UNRAR OFF)
endif()
option(GME_ZLIB "GME with zlib" OFF)
option(GME_UNRAR "GME with unrar" OFF)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this seems like a better way. Also libgme CMakeLists no longer has GME_UNRAR option.

@madebr madebr merged commit 62a4100 into libsdl-org:main Sep 5, 2024
5 checks passed
@mechakotik
Copy link
Contributor Author

Could you please merge it into SDL2 too?

@madebr
Copy link
Contributor

madebr commented Sep 5, 2024

Sure, see f66ffea (SDL2) and 33668fa (2.8.x)

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.

2 participants