Skip to content

Add macro for uninstall target and fix symlink tracking#4775

Open
jmsexton03 wants to merge 2 commits intoAMReX-Codes:developmentfrom
jmsexton03:cmake_symlink_to_install
Open

Add macro for uninstall target and fix symlink tracking#4775
jmsexton03 wants to merge 2 commits intoAMReX-Codes:developmentfrom
jmsexton03:cmake_symlink_to_install

Conversation

@jmsexton03
Copy link
Copy Markdown
Contributor

@jmsexton03 jmsexton03 commented Nov 10, 2025

Summary

Additional background

Previously, the libamrex symlink was not tracked in the install manifest. Newer CMake versions provide better ways to reference source locations using variables instead of hardcoded paths, allowing manifest information to be stored more accurately. The new macro also prevents conflicts when multiple projects using AMReX each try to create their own uninstall targets.

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@jmsexton03 jmsexton03 requested a review from ax3l November 10, 2025 21:59
@ax3l ax3l self-assigned this Nov 11, 2025
@jmsexton03 jmsexton03 marked this pull request as ready for review November 11, 2025 13:25
@ax3l
Copy link
Copy Markdown
Member

ax3l commented Apr 10, 2026

Thank you for this PR and sorry for the long wait.

There is a small problem: the install prefix now lacks libamrex.a, while the symlink appears in the original configure-time prefix instead.

The symlink install logic around Tools/CMake/AMReXInstallHelpers.cmake:74 still derives symlink_name from the configure-time prefix, so cmake --install --prefix <new-prefix> can silently write libamrex.a into the old tree instead of the requested one. So also the uninstall targets the wrong location.

@ax3l
Copy link
Copy Markdown
Member

ax3l commented Apr 10, 2026

Thanks again for putting this together, and sorry this sat for so long.

After looking at a recent PR in #5277, one can fold the symlink/install-manifest bug fix in there, mainly because that PR was already the narrower diff for the existing cmake --install --prefix ... failure.

The legacy link is now also recorded in install_manifest.txt, so manifest-based uninstall flows will remove it correctly as well. In other words, the install bookkeeping piece from your PR is covered there now, even though we did not carry over the separate uninstall target helper.

If you still want the reusable uninstall target machinery, that could still make sense to add (e.g. update this PR?). This is useful on its own and independent from the symlink bug.

@ax3l ax3l mentioned this pull request Apr 10, 2026
6 tasks
@ax3l
Copy link
Copy Markdown
Member

ax3l commented Apr 10, 2026

updated PR accordingly :) does that work?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants