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: add uninstall target #478

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

ChillerDragon
Copy link
Contributor

The code is yoinked from the SDL repo. Works fine on my machine.

chiller@debian:~/Desktop/git/SDL_image/build$ sudo make install
[sudo] password for chiller: 
[ 84%] Built target SDL3_image-shared
[ 92%] Built target showanim
[100%] Built target showimage
Install the project...
-- Install configuration: ""
-- Installing: /usr/local/lib/libSDL3_image.so.0.1.0
-- Up-to-date: /usr/local/lib/libSDL3_image.so.0
-- Set runtime path of "/usr/local/lib/libSDL3_image.so.0.1.0" to ""
-- Up-to-date: /usr/local/lib/libSDL3_image.so
-- Installing: /usr/local/include/SDL3_image/SDL_image.h
-- Installing: /usr/local/lib/cmake/SDL3_image/SDL3_imageConfig.cmake
-- Installing: /usr/local/lib/cmake/SDL3_image/SDL3_imageConfigVersion.cmake
-- Installing: /usr/local/lib/cmake/SDL3_image/SDL3_image-shared-targets.cmake
-- Installing: /usr/local/lib/cmake/SDL3_image/SDL3_image-shared-targets-noconfig.cmake
-- Installing: /usr/local/lib/pkgconfig/sdl3-image.pc
-- Installing: /usr/local/share/licenses/SDL3_image/LICENSE.txt
chiller@debian:~/Desktop/git/SDL_image/build$ sudo make uninstall
-- Uninstalling "/usr/local/lib/libSDL3_image.so.0.1.0"
-- Uninstalling "/usr/local/lib/libSDL3_image.so.0"
-- Uninstalling "/usr/local/lib/libSDL3_image.so"
-- Uninstalling "/usr/local/include/SDL3_image/SDL_image.h"
-- Uninstalling "/usr/local/lib/cmake/SDL3_image/SDL3_imageConfig.cmake"
-- Uninstalling "/usr/local/lib/cmake/SDL3_image/SDL3_imageConfigVersion.cmake"
-- Uninstalling "/usr/local/lib/cmake/SDL3_image/SDL3_image-shared-targets.cmake"
-- Uninstalling "/usr/local/lib/cmake/SDL3_image/SDL3_image-shared-targets-noconfig.cmake"
-- Uninstalling "/usr/local/lib/pkgconfig/sdl3-image.pc"
-- Uninstalling "/usr/local/share/licenses/SDL3_image/LICENSE.txt"
Built target uninstall
chiller@debian:~/Desktop/git/SDL_image/build$ 

CMakeLists.txt Outdated
@@ -1126,3 +1126,13 @@ string(TOLOWER "${unavail_str}" unavail_str)
message(STATUS "SDL3_image backends:")
message(STATUS "- enabled: ${avail_str}")
message(STATUS "- disabled: ${unavail_str}")

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this block inside the if(SDLIMAGE_INSTALL) block? At around line 1080.
Please also use 4-space indentation.

Copy link
Contributor Author

@ChillerDragon ChillerDragon Nov 18, 2024

Choose a reason for hiding this comment

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

done.

I was wondering about that. In SDL there is also a option to disable uninstall. I did not port that. Do we need that here as well? So a if(NOT SDLIMAGE_DISABLE_UNINSTALL) what is that for?

https://github.com/libsdl-org/SDL/blob/e027b85cc457556071cbb2f3f1bcf8803c1bc001/CMakeLists.txt#L262

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently to support sub projects better according to libsdl-org/SDL@bb9ebad

But I do not think that case needs a separation between install and uninstall. Should be fine to make the uninstall dependent on the install option.

@ChillerDragon
Copy link
Contributor Author

Is the failling CI on me?

CMakeLists.txt Outdated Show resolved Hide resolved
@madebr
Copy link
Contributor

madebr commented Nov 18, 2024

Is the failling CI on me?

We have a known test issue with newer libjxl.

@slouken slouken merged commit dbda6d8 into libsdl-org:main Dec 6, 2024
@slouken
Copy link
Collaborator

slouken commented Dec 6, 2024

@madebr, does this make sense to add to the other satellite libraries?

@madebr
Copy link
Contributor

madebr commented Dec 6, 2024

@madebr, does this make sense to add to the other satellite libraries?

For those crazy enough to install directly to their root prefix, it's useful.
But I would not promote this kind of behavior.

@slouken
Copy link
Collaborator

slouken commented Dec 6, 2024

We should either remove this PR or apply it universally.

My usual workflow is:
mkdir build
cd build
cmake ..
make install

I have often wanted to be able to
make uninstall

Does this PR make that possible?

@madebr
Copy link
Contributor

madebr commented Dec 6, 2024

I have often wanted to be able to make uninstall

Does this PR make that possible?

Yes it does

@slouken
Copy link
Collaborator

slouken commented Dec 6, 2024

Sweet, then let's propagate it across SDL and satellite libraries.

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