Added an uninstall target for make

[PJ-Finlay]

I modified CMakeLists.txt to add an uninstall target for make. If you run "make uninstall" after running "make install" a cmake script at cmake/uninstall.cmake will be run that attempts to delete all of the files in install_manifest.txt. This is to fix issue #1676.

[Wallacoloo]

Looks good to me.

[PhysSong]

The second message won't be show up because first one will make the script exit. I think you can join those text with \n and show in one MESSAGE().

[PJ-Finlay]

Good catch, I just amended the commit to combine them into one line with a \n character.

[lukas-w]

The variable CMAKE_CURRENT_BINARY_DIR is probably not what you think it is in this context. When run with -P, CMake sets this variable to the current working directory, so this approach won't work when run from a different directory. You'll need to process the variable @CMAKE_BINARY_DIR@ using configure_file instead.

[lukas-w]

Could you change the control flow to something like this so it's easier to read?

IF(NOT EXISTS ${INSTALL_MANIFEST_PATH})
    MESSAGE(FATAL_ERROR […error message…])
ENDIF()

MESSAGE(STATUS "install_manifest.txt found")
[… the rest of the uninstall code …]

MESSAGE(FATAL_ERROR …) aborts the script so the rest of the script won't execute if the error occurs.

[lukas-w]

exec_program is deprecated, you should use execute_process instead.

[lukas-w]

Typo in has non been run.

[PJ-Finlay]

I think I made the requested changes. However, I think CMAKE_CURRENT_BINARY_DIR is the right way to find the install_manifest.txt file.

Changes:
-Are you sure about changing CMAKE_CURRENT_BINARY_DIR to configure file? I realize that CMAKE_CURRENT_BINARY_DIR is set to the current directory. When a user runs make they need to be in the build directory where the the install_manifest.txt file is. make has a -C option which lets you run it from a different directory, but this solution still works while doing that.

-I changed to the requested control flow.

-I switched exec_program to execute_process.

-I fixed the typo in "has non been run"

[lukas-w]

Thanks! 👍

Are you sure about changing CMAKE_CURRENT_BINARY_DIR to configure file?

I believe it depends on the generator used, but it works at least with GNU Make and Ninja in my tests, so I guess it's fine.

[PhysSong]

BTW, this pull request targets to stable-1.2 which is on a feature freeze. @lukas-w Is it okay to merge this into stable-1.2?

[lukas-w]

Running make uninstall still leaves all the (empty) folders created by installing. If we can address that in this PR, I guess it's fine to merge into stable-1.2. Otherwise, I'd rather merge it into master.

[PJ-Finlay]

I can try to also delete the folders.

[PJ-Finlay]

@lukas-w how should removing folders be done? Currently it's just deleting all of the files in install_manifest.txt. I could just delete the include/lmms & lib/lmms folders, and the things that are in share/ directly but I don't know how portable this would be. Also it seems like most other people I can find online don't do this and just delete from install_manifest.txt: https://gergap.wordpress.com/2015/08/18/cmake-uninstall/, https://bitbucket.org/eigen/eigen/pull-requests/91/added-cmake-uninstall-target/diff, https://gist.github.com/royvandam/3033428.

[lukas-w]

I hacked something together, but it requires CMake 3.3 because it uses IN_LIST so it would only be good for master:

# Checks if a directory is empty and saves the result in out_var
function(is_empty_dir out_var dir)
	file(GLOB files "${dir}/*")
	list(LENGTH files num_files)
	if(num_files EQUAL 0)
		set(${out_var} TRUE PARENT_SCOPE)
	else()
		set(${out_var} FALSE PARENT_SCOPE)
	endif()
endfunction()

# Recursively gets all parent directories
function(parent_directories out_var path)
	get_filename_component(parent "${path}" DIRECTORY)
	if(parent AND NOT parent STREQUAL path AND NOT parent IN_LIST ${out_var})
		LIST(APPEND ${out_var} ${parent})
		parent_directories(${out_var} "${parent}")
	endif()
	set(${out_var} ${${out_var}} PARENT_SCOPE)
endfunction()

# Removes all empty parent directories of the given files
function(remove_empty_directories files)
	set(directories)
	foreach(FILE_TO_REMOVE ${FILES_TO_REMOVE})
		parent_directories(directories "${FILE_TO_REMOVE}")
	endforeach()
	list(REMOVE_DUPLICATES directories)
	# Sort and reverse so we remove subdirectories first
	list(SORT directories)
	list(REVERSE directories)

	foreach(dir ${directories})
		# Skip directories not inside the install prefix
		if(NOT (EXISTS "${dir}" AND dir MATCHES "^${CMAKE_INSTALL_PREFIX}/"))
			continue()
		endif()

		is_empty_dir(dir_empty "${dir}")
		if(dir_empty)
			message(STATUS "Removing empty directory ${dir}")
			file(REMOVE_RECURSE "${dir}")
		elseif()
			message(STATUS "Skipping non-empty directory ${dir}")
		endif()
	endforeach()
endfunction()

...

remove_empty_directories("${FILES_TO_REMOVE}")

ADD_CUSTOM_TARGET(uninstall
	COMMAND ${CMAKE_COMMAND} -DCMAKE_INSTALL_PREFIX="${CMAKE_INSTALL_PREFIX}" -P "${CMAKE_CURRENT_SOURCE_DIR}/cmake/uninstall.cmake"
)

[PJ-Finlay]

I added @lukas-w's code to remove the empty directories created during installation into my pull request:

• I fixed a typo in the remove_empty_directories function where the ${FILES_TO_REMOVE} variable was being used from the parent scope instead of the ${FILE} variable from the functions arguments.

• I set Cmake policy CMP0057 to NEW to use the IN_LIST operator.

• I changed the minimum Cmake version to 3.3 which is required for the IN_LIST operator.

• I switched to upper case because that's what the rest of the project seemed to be doing.

[lukas-w]

Thanks @PJ-Finlay. I made some minor changes via 63fd427 and will merge into master soon.