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.

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

fix: use rpath in engine dylib #5031

Merged
merged 2 commits into from
Jul 9, 2024
Merged

Conversation

dandanlen
Copy link
Collaborator

@dandanlen dandanlen commented Jul 9, 2024

This fixes an issue with the engine dylib file that was preventing it from running on mac localnets.

The dylib install path was previously set to a local path on my machine.
Now it's correctly set to a relative path.

The engine lib build script already do this correctly on main, but that change hasn't been ported to the release branch. Hence the release version 1.4.5 libs were compiled with absolute paths instead of relative paths.

Before:

otool -L old-engine-dylib/libchainflip_engine_v1_4_5.dylib | head -2
old-engine-dylib/libchainflip_engine_v1_4_5.dylib:
        /Users/dan/[...]/old-engine-dylib/libchainflip_engine_v1_4_5.dylib (compatibility version 0.0.0, current version 0.0.0)

After:

otool -L old-engine-dylib/libchainflip_engine_v1_4_5.dylib | head -2
old-engine-dylib/libchainflip_engine_v1_4_5.dylib:
        @rpath/old-engine-dylib/libchainflip_engine_v1_4_5.dylib (compatibility version 0.0.0, current version 0.0.0)

@dandanlen dandanlen enabled auto-merge July 9, 2024 10:40
@dandanlen dandanlen requested a review from j4m1ef0rd July 9, 2024 10:40
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72%. Comparing base (1ef2593) to head (f4bacec).
Report is 3 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #5031   +/-   ##
=====================================
- Coverage     72%     72%   -0%     
=====================================
  Files        431     431           
  Lines      74778   74775    -3     
  Branches   74778   74775    -3     
=====================================
- Hits       53536   53487   -49     
- Misses     18396   18431   +35     
- Partials    2846    2857   +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dandanlen dandanlen added this pull request to the merge queue Jul 9, 2024
Merged via the queue into main with commit 7381280 Jul 9, 2024
47 checks passed
@dandanlen dandanlen deleted the fix/use-rpath-in-engine-dylib branch July 9, 2024 12:25
@kylezs
Copy link
Contributor

kylezs commented Jul 16, 2024

It should be @rpath/libchainflip-engine... and not @rpath/old-engine-dylib/libchainflip-engine...

@kylezs
Copy link
Contributor

kylezs commented Jul 16, 2024

FYI - this won't need to be set manually after 1.5 (i.e. when 1.5 is the "old" dylib) - as it's now part of the build. rpath = true in the Cargo.toml now.

@kylezs kylezs mentioned this pull request Jul 16, 2024
2 tasks
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.

3 participants