Skip to content

cmake: minor improvements to cmake_uninstall.in.cmake#22201

Closed
vszakats wants to merge 9 commits into
curl:masterfrom
vszakats:cmtidy
Closed

cmake: minor improvements to cmake_uninstall.in.cmake#22201
vszakats wants to merge 9 commits into
curl:masterfrom
vszakats:cmtidy

Conversation

@vszakats

@vszakats vszakats commented Jun 27, 2026

Copy link
Copy Markdown
Member
  • reduce log noise by showing 'Uninstalling' message only if the file
    exists. (and a different message otherwise)
  • replace cmake rm command with file(REMOVE). For efficiency.
    Show manual message if the file could not be deleted.
    Ref: https://cmake.org/cmake/help/v3.18/command/file.html#remove
    Follow-up to 6d00835 cmake: replace deprecated remove command with rm and pass arg safely #22193
  • reduce log noise by showing 'Uninstalled' only if the deletion was
    successful. (and a different message otherwise)
  • display DESTDIR env value if set.
  • drop checking, setting and showing CMAKE_INSTALL_PREFIX.
    The value was never predefined, also not used, besides showing it,
    and showing it is misleading because --prefix may override the
    configure-time value, and also superfluous because the filenames
    are showing the actual prefix anyway.

Follow-up to 27e2a47

@vszakats vszakats added the cmake label Jun 27, 2026
@vszakats vszakats marked this pull request as draft June 27, 2026 01:39
@github-actions github-actions Bot added the build label Jun 27, 2026
@vszakats vszakats requested a review from Copilot June 27, 2026 01:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR performs small CMake-related cleanups and documentation/comment touch-ups to reduce noise and improve clarity in the CMake uninstall script and associated docs.

Changes:

  • Refines cmake_uninstall.in.cmake messaging (prefix/DESTDIR reporting, less “Uninstalling …” noise for non-existent files).
  • Minor CMakeLists tidy-ups (remove redundant local var, match existing configure_file(... @ONLY) formatting).
  • Documentation/comment corrections (note CMake 3.29+ env support for CMAKE_INSTALL_PREFIX, fix a copy/paste comment typo).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
docs/INSTALL-CMAKE.md Notes CMake 3.29+ behavior for CMAKE_INSTALL_PREFIX in the options list.
CMakeLists.txt Removes an unused intermediate variable and aligns configure_file(... @ONLY) formatting with nearby usage.
CMake/CurlTests.c Fixes a misleading comment for the SIOCGIFADDR test headers.
CMake/cmake_uninstall.in.cmake Improves uninstall script status output and avoids printing “Uninstalling …” when the target doesn’t exist.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@vszakats vszakats changed the title cmake: various tidy-ups cmake: minor tidy-ups Jun 27, 2026
@vszakats vszakats changed the title cmake: minor tidy-ups cmake: small tidy-ups Jun 27, 2026
@vszakats vszakats changed the title cmake: small tidy-ups cmake: tidy up uninstall and some more Jun 27, 2026
@vszakats vszakats changed the title cmake: tidy up uninstall and some more cmake: minor improvements to cmake_uninstall.in.cmake Jun 27, 2026
@vszakats vszakats requested a review from Copilot June 27, 2026 10:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comment thread CMake/cmake_uninstall.in.cmake
Comment thread CMake/cmake_uninstall.in.cmake Outdated
@vszakats vszakats requested a review from Copilot June 27, 2026 10:23
@vszakats vszakats marked this pull request as ready for review June 27, 2026 10:24

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

set(CMAKE_INSTALL_PREFIX "@CMAKE_INSTALL_PREFIX@")
message(STATUS "Install prefix: @CMAKE_INSTALL_PREFIX@")
set(_destdir "$ENV{DESTDIR}")
if(NOT _destdir STREQUAL "")

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Complete LLM fail.

Comment thread CMake/cmake_uninstall.in.cmake Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread CMake/cmake_uninstall.in.cmake

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread CMake/cmake_uninstall.in.cmake
@vszakats vszakats closed this in 88d8591 Jun 27, 2026
@vszakats vszakats deleted the cmtidy branch June 27, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants