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: support sudo make uninstall #1674

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@jasjuang
Contributor

jasjuang commented Jul 10, 2017

This features reduces the pain of having to remove the installed libraries and headers in /usr/local/ one by one.

Before this change, after sudo make install

Install the project...
-- Install configuration: ""
-- Up-to-date: /usr/local/bin/curl-config
-- Up-to-date: /usr/local/lib/pkgconfig/libcurl.pc
-- Up-to-date: /usr/local/include/curl/curlbuild.h
-- Up-to-date: /usr/local/include/curl
-- Up-to-date: /usr/local/include/curl/multi.h
-- Up-to-date: /usr/local/include/curl/easy.h
-- Up-to-date: /usr/local/include/curl/mprintf.h
-- Up-to-date: /usr/local/include/curl/curl.h
-- Up-to-date: /usr/local/include/curl/curlrules.h
-- Up-to-date: /usr/local/include/curl/typecheck-gcc.h
-- Up-to-date: /usr/local/include/curl/curlver.h
-- Up-to-date: /usr/local/include/curl/stdcheaders.h
-- Up-to-date: /usr/local/lib/libcurl.so
-- Up-to-date: /usr/local/bin/curl

there is no easy ways to remove these files, but with this change we can simply type sudo make uninstall and

Scanning dependencies of target uninstall
/usr/local
-- Uninstalling /usr/local/bin/curl-config
-- Uninstalling /usr/local/lib/pkgconfig/libcurl.pc
-- Uninstalling /usr/local/include/curl/curlbuild.h
-- Uninstalling /usr/local/include/curl/multi.h
-- Uninstalling /usr/local/include/curl/easy.h
-- Uninstalling /usr/local/include/curl/mprintf.h
-- Uninstalling /usr/local/include/curl/curl.h
-- Uninstalling /usr/local/include/curl/curlrules.h
-- Uninstalling /usr/local/include/curl/typecheck-gcc.h
-- Uninstalling /usr/local/include/curl/curlver.h
-- Uninstalling /usr/local/include/curl/stdcheaders.h
-- Uninstalling /usr/local/lib/libcurl.so
-- Uninstalling /usr/local/bin/curl
Built target uninstall
@mention-bot

This comment has been minimized.

mention-bot commented Jul 10, 2017

@jasjuang, thanks for your PR! By analyzing the history of the files in this pull request, we identified @billhoffman, @Sukender and @jzakrzewski to be potential reviewers.

@coveralls

This comment has been minimized.

coveralls commented Jul 11, 2017

Coverage Status

Coverage increased (+0.05%) to 76.062% when pulling badda39 on jasjuang:master into e909de6 on curl:master.

@bagder bagder added the cmake label Jul 11, 2017

@bagder bagder changed the title from support sudo make uninstall to cmake: support sudo make uninstall Jul 13, 2017

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 27, 2017

@jasjuang Bad idea. If you required sudo to install why don't you call uninstall without sudo and push it into build system?

@bagder I suggest declining this PR.

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 27, 2017

@jasjuang @bagder Sorry for my previous comment. Didn't get it for the first time. Will check it today evening.
Again, sorry for the noise.

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 27, 2017

@bagder this PR just copy-paste from CMake FAQ https://cmake.org/Wiki/CMake_FAQ#Can_I_do_.22make_uninstall.22_with_CMake.3F

So depending on your decision.
If make uninstall is required, you can merge it.
Personally, I don't like this idea and suggesting @jasjuang focus on CPack support instead.

@jasjuang

This comment has been minimized.

Contributor

jasjuang commented Jul 27, 2017

@snikulov Not sure why you don't like this idea. This is a common feature for most open source libraries that supports cmake like OpenCV, glm, glfw, glew, pcl, ceres, glog, gflags, tinyxml2 etc. Without this PR, how would you remove the files installed in /usr/local easily?

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 27, 2017

@jasjuang As I already mentioned, I prefer to use CPack to make a package for installation.
I rarely keep build directory to run make uninstall from it.
To uninstall something from build directory I will use

xargs rm < install_manifest.txt

as mentioned on the link where you copy it.
It simply shorter.

@jasjuang

This comment has been minimized.

Contributor

jasjuang commented Jul 27, 2017

@snikulov Well, sudo make uninstall is shorter than sudo xargs rm < install_manifest.txt. Also, this is beneficial for windows as well, I can run the uninstall target to remove the installed files without having to manually delete it.

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 28, 2017

@jasjuang matter of taste. It's shorter because of increased code size under your control and support.

@bagder

This comment has been minimized.

Member

bagder commented Jul 28, 2017

In general I think it makes sense to support "make uninstall" as an opposite to "make install", and the autotools build supports it already.

This code seems to look for and use a install_manifest.txt file to know what specific files to uninstall? What's that and how does it get created?

@jasjuang

This comment has been minimized.

Contributor

jasjuang commented Jul 29, 2017

@bagder install_manifest.txt is created by cmake as long as the install functions are been written inside the CMakeLists. It records what files should be installed.

@snikulov

This comment has been minimized.

Member

snikulov commented Jul 29, 2017

@bagder then merge it. As I've already mentioned it's copy-paste from cmake F.A.Q., so I see no issue with it.

@bagder bagder closed this in 27e2a47 Jul 29, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 29, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment