Skip to content

Fix Jsonnet Library Loading On MacOS#170

Merged
knoepfel merged 5 commits intoFramework-R-D:mainfrom
marcpaterno:fix-macos-jsonnet
Dec 8, 2025
Merged

Fix Jsonnet Library Loading On MacOS#170
knoepfel merged 5 commits intoFramework-R-D:mainfrom
marcpaterno:fix-macos-jsonnet

Conversation

@marcpaterno
Copy link
Member

Jsonnet libraries from Spack have incorrect install_name entries on macOS (bare filenames instead of @rpath-relative paths). This prevents the dynamic linker from finding them at runtime despite correct RPATH settings in executables.

Add logic to Findjsonnet.cmake to detect and fix these libraries during CMake configuration using install_name_tool, ensuring they use @rpath-relative references compatible with macOS dyld.

Jsonnet libraries from Spack have incorrect install_name entries
on macOS (bare filenames instead of @rpath-relative paths). This
prevents the dynamic linker from finding them at runtime despite
correct RPATH settings in executables.

Add logic to Findjsonnet.cmake to detect and fix these libraries
during CMake configuration using install_name_tool, ensuring they
use @rpath-relative references compatible with macOS dyld.
Copilot AI review requested due to automatic review settings December 8, 2025 20:18
Copy link
Contributor

Copilot AI left a comment

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 adds a macOS-specific workaround to Findjsonnet.cmake to fix incorrect install_name entries in Jsonnet libraries installed via Spack. The fix runs during CMake configuration, detecting libraries with bare filename install_names and updating them to use @rpath-relative paths using install_name_tool.

Key changes:

  • Added macOS-specific logic to detect and fix malformed install_name entries in jsonnet libraries
  • Uses otool to inspect library install_names and install_name_tool to correct them
  • Only fixes libraries with install_names that don't start with @ or / (relative paths)

)
endif()
endif()

Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

Remove this trailing blank line. Files must end with exactly one newline character and no trailing blank lines.

Copilot generated this review using guidance from repository custom instructions.
Copy link
Member Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

@codecov
Copy link

codecov bot commented Dec 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #170      +/-   ##
==========================================
- Coverage   81.42%   80.79%   -0.64%     
==========================================
  Files         115      115              
  Lines        2046     2046              
  Branches      330      330              
==========================================
- Hits         1666     1653      -13     
- Misses        252      258       +6     
- Partials      128      135       +7     
Flag Coverage Δ
unittests 80.79% <ø> (-0.64%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b6b96c...5f9434d. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@greenc-FNAL greenc-FNAL left a comment

Choose a reason for hiding this comment

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

I have concerns about using one package's CMake to fix an external package's libraries. I realize this is on MacOS, which is generally single-user and local access, but there is the possibility of failure if the installed package is on CVMFS, and race conditions if the user has local agents running e.g. through VSCode.

Is this being paired with a fix to the Spack recipe?

@marcpaterno
Copy link
Member Author

@phlexbot format

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@marcpaterno
Copy link
Member Author

I have concerns about using one package's CMake to fix an external package's libraries. I realize this is on MacOS, which is generally single-user and local access, but there is the possibility of failure if the installed package is on CVMFS, and race conditions if the user has local agents running e.g. through VSCode.

Is this being paired with a fix to the Spack recipe?

This looks like multiple issues:

  1. have concerns about using one package's CMake to fix an external package's libraries.

It would be great to have the Spack recipe for jsonnet build well-formed libraries, but that is a bigger fix than I wanted to handle now. This is a fix limited to our own code that solves the current problem.

  1. I realize this is on MacOS, which is generally single-user and local access, but there is the possibility of failure if the installed package is on CVMFS, and race conditions if the user has local agents running e.g. through VSCode.

We do not now use CVMFS on macOS, and I do not think we have any reason to do so. Given that, is there still a race condition problem? I do not think so.

  1. Is this being paired with a fix to the Spack recipe?

It does not depend on a fix to the Spack recipe. Fixing the recipe is a larger task.
But I will add a comment to note that this should be removed when the Spack recipe is fixed.

@knoepfel knoepfel merged commit a2fde58 into Framework-R-D:main Dec 8, 2025
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants