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

Don't make the uninstall target if it already exists #172

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

jmcarcell
Copy link
Contributor

@jmcarcell jmcarcell commented Jul 14, 2023

BEGINRELEASENOTES

  • Don't make the uninstall target if it already exists

ENDRELEASENOTES

CMake will throw an error and stop if the uninstall target already exists

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Is the cmake_uninstall.cmake necessary for anything else or could that be removed as well?

@jmcarcell
Copy link
Contributor Author

If we remove cmake_uninstall.cmake then ilcsoft_default_uninstall_target.cmake can also be removed; I wasn't proposing that but for me it would be fine since I never make use of the uninstall target.

@tmadlener
Copy link
Contributor

My main concern would be to not have "dead" code around and then having to wonder down the line, whether it can be removed.

Maybe @andresailer or @gaede can shed some light on the use cases of the uninstall target (and how it was used in the past)?

@jmcarcell
Copy link
Contributor Author

The only useful case that I have found has been when installing to the system in /usr/local, for example, and then when you have multiple packages installed like this uninstalling them is hard since they share installation folders

@tmadlener
Copy link
Contributor

Looking at cmake_uninstall.cmake.in it looks like it pretty much follows what is suggested by cmake:
https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#can-i-do-make-uninstall-with-cmake

So, I think we keep this PR as it is and just guard against a duplicate definition.

@jmcarcell
Copy link
Contributor Author

Sounds good, iLCSoft/iLCUtil#29 is the same PR

@tmadlener tmadlener merged commit 6aef732 into iLCSoft:master Jul 17, 2023
10 checks passed
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