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.

Sign up for GitHub

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

Jump to bottom

Add uninstall target #1624

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Add uninstall target #1624

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
81cecbd
Add uninstall target
cary-ilm Feb 1, 2024
3e57c10
mention uninstall in install instructions
cary-ilm Feb 1, 2024
59f6f40
poke
cary-ilm Feb 1, 2024
73d64d7
COPY_ON_ERROR
cary-ilm Feb 9, 2024
50fcd31
clarify the uninstall instructions
cary-ilm Feb 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ if(OPENEXR_INSTALL_TOOLS AND NOT OPENEXR_INSTALL)
message(SEND_ERROR "OPENEXR_INSTALL_TOOLS requires OPENEXR_INSTALL")
endif()

# uninstall target
if(NOT TARGET uninstall)
configure_file(
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/cmake_uninstall.cmake.in"
"${CMAKE_CURRENT_BINARY_DIR}/cmake_uninstall.cmake"
IMMEDIATE @ONLY)
add_custom_target(uninstall
COMMAND ${CMAKE_COMMAND} -P ${CMAKE_CURRENT_BINARY_DIR}/cmake_uninstall.cmake)
endif()

include(cmake/LibraryDefine.cmake)
include(cmake/OpenEXRSetup.cmake)
add_subdirectory(cmake)
Expand Down
9 changes: 0 additions & 9 deletions cmake/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,15 +74,6 @@ configure_file(OpenEXRConfigInternal.h.in ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRCon
# make a temp copy in the binary dir for OpenEXRConfig.h to include
configure_file(../src/lib/OpenEXRCore/openexr_version.h ${CMAKE_CURRENT_BINARY_DIR}/OpenEXRCore/openexr_version.h COPYONLY)

if(OPENEXR_INSTALL)
install(
FILES
${CMAKE_CURRENT_BINARY_DIR}/OpenEXRConfig.h
DESTINATION
${CMAKE_INSTALL_INCLUDEDIR}/${OPENEXR_OUTPUT_SUBDIR}
)
endif()

###################################################
####### IexConfig.h and IexConfigInternal.h

Expand Down
3 changes: 2 additions & 1 deletion cmake/LibraryDefine.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ function(OPENEXR_DEFINE_LIBRARY libname)
string(TOUPPER "${CMAKE_BUILD_TYPE}" uppercase_CMAKE_BUILD_TYPE)
set(verlibname ${CMAKE_SHARED_LIBRARY_PREFIX}${libname}${OPENEXR_LIB_SUFFIX}${CMAKE_${uppercase_CMAKE_BUILD_TYPE}_POSTFIX}${CMAKE_SHARED_LIBRARY_SUFFIX})
set(baselibname ${CMAKE_SHARED_LIBRARY_PREFIX}${libname}${CMAKE_${uppercase_CMAKE_BUILD_TYPE}_POSTFIX}${CMAKE_SHARED_LIBRARY_SUFFIX})
install(CODE "execute_process(COMMAND ${CMAKE_COMMAND} -E chdir \"\$ENV\{DESTDIR\}${CMAKE_INSTALL_FULL_LIBDIR}\" ${CMAKE_COMMAND} -E create_symlink ${verlibname} ${baselibname})")
file(CREATE_LINK ${verlibname} ${baselibname} SYMBOLIC)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work right on Windows? I recall that some link thinks don't work quite the same way as you'd expect from Unix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could someone with a Window please confirm the symlinks are still valid?

Copy link
Member

Choose a reason for hiding this comment

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

Based on https://cmake.org/cmake/help/latest/command/file.html#create-link, we could pass COPY_ON_ERROR so that cmake will copy instead of symlinking if it fails?

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding Windows, the entire logic here is inside if (... AND NOT WIN32), so hopefully it's not an issue. And hopefully cmake -E create_simlink has a similar effect to file(CREATE_LINK ... SYMBOLIC).

I've added COPY_ON_ERROR as a failsafe anyway.

install(FILES ${CMAKE_BINARY_DIR}/${baselibname} DESTINATION ${CMAKE_INSTALL_FULL_LIBDIR})
install(CODE "message(STATUS \"Creating symlink ${CMAKE_INSTALL_FULL_LIBDIR}/${baselibname} -> ${verlibname}\")")
set(verlibname)
set(baselibname)
Expand Down
23 changes: 23 additions & 0 deletions cmake/cmake_uninstall.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# Source: https://gitlab.kitware.com/cmake/community/-/wikis/FAQ#can-i-do-make-uninstall-with-cmake

if(NOT EXISTS "@CMAKE_BINARY_DIR@/install_manifest.txt")
message(FATAL_ERROR "Cannot find install manifest: @CMAKE_BINARY_DIR@/install_manifest.txt")
endif()

file(READ "@CMAKE_BINARY_DIR@/install_manifest.txt" files)
string(REGEX REPLACE "\n" ";" files "${files}")
foreach(file ${files})
message(STATUS "Uninstalling $ENV{DESTDIR}${file}")
if(IS_SYMLINK "$ENV{DESTDIR}${file}" OR EXISTS "$ENV{DESTDIR}${file}")
exec_program(
"@CMAKE_COMMAND@" ARGS
"-E remove \"$ENV{DESTDIR}${file}\""
OUTPUT_VARIABLE rm_out
RETURN_VALUE rm_retval)
if(NOT "${rm_retval}" STREQUAL 0)
message(FATAL_ERROR "Problem when removing $ENV{DESTDIR}${file}")
endif()
else(IS_SYMLINK "$ENV{DESTDIR}${file}" OR EXISTS "$ENV{DESTDIR}${file}")
message(STATUS "File $ENV{DESTDIR}${file} does not exist.")
endif()
endforeach()
16 changes: 16 additions & 0 deletions website/install.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,22 @@ You can customize these options three ways:
2. Use the UI ``cmake-gui`` or ``ccmake``.
3. Specify them as command-line arguments when you invoke cmake.

Uninstall
~~~~~~~~~

If you have installed from source, you can uninstall via:

.. code-block::

% cmake --build $builddir --target uninstall

or if using ``make``:

.. code-block::

% make uninstall


Copy link
Contributor

Choose a reason for hiding this comment

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

This implies that if you still have the exact working source and build tree that built your installation, then running this would remove the installed components.

But if you don't have source, or a build tree, or a build tree that exactly matches the commit and build-time options of your installed OpenEXR, this might be unreliable or entirely unavailable. It's not at all like having an "openexr-uninstall" script installed with the rest of the binary components.

I'm not sure if this satisfies the general requirement to have an uninstall command. Is that sufficient from an OpenSSF standpoint? Do they mean "the person who built it can reverse an install if they catch it before they blow away their build tree?" Or do they mean "any user with permissions to alter the install area can reliably remove all installed components at any time?"

Copy link
Member Author

Choose a reason for hiding this comment

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

@jmertic mentioned on slack a while back that:

For source builds, just a note saying "files are created at x, y, z that would need to be removed for uninstall" is sufficent

So the uninstall target might not be absolutely necessary to satisfy the badge requirements. But I do see this construct adopted by other respectable projects like pybind11 and c-blosc and libjpSo the uninstall target might not be absolutely necessary to satisfy the badge requirements. But I do see this construct adopted by other projects like pybind11 and c-blosc and libjpeg-turbo
eg-turbo, so it seems reasonable to follow that convention.

I can also add a mention to the obvious - if you installed via a package manager, uninstall with the package manager. This applies only if you've built from source (and still have the configuration files).

Copy link
Member

Choose a reason for hiding this comment

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

I think it has value in the sense that if a contributor accidentally installs into its root filesystem, at least there is an uninstall target to undo the damage. When I started compiling stuff a couple of years ago, I did this mistake quite often and make uninstall or equivalents (when available) saved me a lot of trouble on multiple occasions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a bit more clarifying wording.

Library Naming Options
~~~~~~~~~~~~~~~~~~~~~~

Expand Down
Loading