delocate/delocating.py: Sanitize each file only once, parallelize #257

mkoeppe · 2025-10-31T03:01:10Z

Performance improvement based in part on a change in #256. PR is on top of #260, for fixing the handling of duplicate rpaths.

_sanitize_rpaths now calls _remove_absolute_rpaths only once for the same file. This saves calls to otool to retrieve rpaths.
Parallelize the calls of _remove_absolute_rpaths using concurrent.futures.ThreadPoolExecutor.

Pull Request Checklist

Read and follow the CONTRIBUTING.md guide
Mentioned relevant issues
Append public facing changes to Changelog.md
Ensure new features are covered by tests
Ensure fixes are verified by tests

HexDecimal · 2025-10-31T03:06:34Z

Funny thing about this. If a library has a duplicate rpath than the CLI tool used to remove paths will only remove one of the duplicates.

mkoeppe · 2025-10-31T03:19:15Z

Right, but _delete_rpaths already takes care of calling install_name_tool the right number of times (once for every bad rpath)

HexDecimal · 2025-10-31T03:51:25Z

Right, but _delete_rpaths already takes care of calling install_name_tool the right number of times (once for every bad rpath)

"once for every bad rpath" is not the right number of times when duplicate paths are not accounted for. This has merged rpaths into a set via dictionary keys, removing duplicates which could cause them to be missed.

I'll only approve this if a test exists for sanitizing a library with duplicate absolute paths and duplicate relative paths.

I would agree that handling rpath duplicates should be accounted for by _remove_absolute_rpaths instead of its caller _sanitize_rpaths.

If this is for performance: The performance issues from this are caused by replace_signature. It would be better to properly defer signature generation instead of scrutinizing the number of times _remove_absolute_rpaths is called. Functions which invalidate a files signature could return the invalided files to the caller.

HexDecimal · 2025-10-31T03:54:17Z

I could also be misreading this. You might need to walk be through this if it's correct. Or else I might need to come back to it later.

mkoeppe · 2025-10-31T03:58:34Z

You are right, there's something fishy there, related to _remove_absolute_rpaths still using the deprecated function get_rpaths, which uniquifies the rpaths. This should probably be replaced by using _get_rpaths.

HexDecimal

My suggestion is to instead add ad_hoc_sign=False to the _remove_absolute_rpaths call and then call replace_signature on the modified files once. Assuming this was for performance.

HexDecimal · 2025-10-31T04:20:06Z

You are right, there's something fishy there, related to _remove_absolute_rpaths still using the deprecated function get_rpaths, which uniquifies the rpaths. This should probably be replaced by using _get_rpaths.

I would believe that _remove_absolute_rpaths code is actually wrong here. It looks like in its current state it would miss the duplicate paths that I mentioned. Specifically it looks like a regression caused by the get_rpaths function. #208 never added a test for the issue it fixed.

delocate/tools.py

codecov · 2025-10-31T04:54:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.47%. Comparing base (b6886bf) to head (4844ef3).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #257      +/-   ##
==========================================
+ Coverage   97.42%   97.47%   +0.04%     
==========================================
  Files          16       16              
  Lines        1242     1265      +23     
==========================================
+ Hits         1210     1233      +23     
  Misses         32       32

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mkoeppe · 2025-10-31T05:30:47Z

The existing suite of test wheels seems much too limited to be sure about the correctness of the code regarding the rpath situation

HexDecimal · 2025-11-01T19:52:35Z

I think this method of conditional signage is wrong tactic. Files to be signed should to be propagated to the caller to be signed all at once. There are later steps of changes which will invalid the signatures of the currently touched code.

Note that there is also the validate_signature function which only signs files which were invalidated. Signatures can be disabled until a final step which validates all binary files at once.

delocate/tools.py

Refactor code to reduce redundant codesigns

mkoeppe · 2025-11-02T06:29:19Z

Rebased on top of #260

…te#257

mkoeppe · 2025-11-02T06:34:55Z

Testing at https://github.com/passagemath/passagemath/actions/runs/19008534669?pr=1755

mkoeppe · 2025-11-02T17:16:34Z

This speeds up the sanitizing phase to 2 minutes, bringing the total down to 16.5 minutes.

https://github.com/passagemath/passagemath/actions/runs/19008534669/job/54293344668?pr=1755#step:25:3332

Sun, 02 Nov 2025 14:47:16 GMT
  + delocate-wheel --require-archs x86_64 -w /private/var/folders/5k/fq5ptf2906bgfn1krb78bmj00000gn/T/cibw-run-saub2489/cp310-macosx_x86_64/repaired_wheel -v /private/var/folders/5k/fq5ptf2906bgfn1krb78bmj00000gn/T/cibw-run-saub2489/cp310-macosx_x86_64/built_wheel/passagemath_fricas-10.6.31-cp310-cp310-macosx_13_0_x86_64.whl
Sun, 02 Nov 2025 14:51:50 GMT
  INFO:delocate.delocating:Copying library /Users/runner/sage-local/lib/libecl.24.5.10.dylib to passagemath_fricas.dylibs/libecl.24.5.10.dylib
Sun, 02 Nov 2025 14:51:50 GMT
  INFO:delocate.delocating:Copying library /Users/runner/sage-local/lib/libgmp.10.dylib to passagemath_fricas.dylibs/libgmp.10.dylib
Sun, 02 Nov 2025 14:51:50 GMT
  INFO:delocate.delocating:Copying library /Users/runner/sage-local/lib/libgc.1.dylib to passagemath_fricas.dylibs/libgc.1.dylib
Sun, 02 Nov 2025 14:51:50 GMT
  INFO:delocate.delocating:Copying library /Users/runner/sage-local/lib/libffi.8.dylib to passagemath_fricas.dylibs/libffi.8.dylib
Sun, 02 Nov 2025 14:51:50 GMT
  INFO:delocate.delocating:Modifying install name in sage_wheels/lib/fricas/target/x86_64-apple-darwin24.6.0/bin/FRICASsys from @rpath/libecl.24.5.dylib to @loader_path/../../../../../../passagemath_fricas.dylibs/libecl.24.5.10.dylib
...
Sun, 02 Nov 2025 14:59:16 GMT
  INFO:delocate.delocating:Modifying install name in passagemath_fricas.dylibs/libecl.24.5.10.dylib from /Users/runner/sage-local/lib/libffi.8.dylib to @loader_path/libffi.8.dylib
Sun, 02 Nov 2025 14:59:16 GMT
  INFO:delocate.tools:Sanitize: Deleting rpath '/Users/runner/sage-local/lib/' from '/private/var/folders/5k/fq5ptf2906bgfn1krb78bmj00000gn/T/tmpx5v9ah32/wheel/sage_wheels/lib/fricas/target/x86_64-apple-darwin24.6.0/bin/FRICASsys'
...
Sun, 02 Nov 2025 15:01:11 GMT
  INFO:delocate.tools:Sanitize: Deleting rpath '/Users/runner/sage-local/lib/' from '/private/var/folders/5k/fq5ptf2906bgfn1krb78bmj00000gn/T/tmpx5v9ah32/wheel/passagemath_fricas.dylibs/libecl.24.5.10.dylib'
Sun, 02 Nov 2025 15:03:56 GMT
  INFO:delocate.delocating:Output:/private/var/folders/5k/fq5ptf2906bgfn1krb78bmj00000gn/T/cibw-run-saub2489/cp310-macosx_x86_64/repaired_wheel/passagemath_fricas-10.6.31-cp310-cp310-macosx_13_0_x86_64.whl
Sun, 02 Nov 2025 15:03:56 GMT
  Fixing: /private/var/folders/5k/fq5ptf2906bgfn1krb78bmj00000gn/T/cibw-run-saub2489/cp310-macosx_x86_64/built_wheel/passagemath_fricas-10.6.31-cp310-cp310-macosx_13_0_x86_64.whl
Sun, 02 Nov 2025 15:03:56 GMT
  Copied to package .dylibs directory:
Sun, 02 Nov 2025 15:03:56 GMT
    /Users/runner/sage-local/lib/libecl.24.5.10.dylib
Sun, 02 Nov 2025 15:03:56 GMT
    /Users/runner/sage-local/lib/libffi.8.dylib
Sun, 02 Nov 2025 15:03:56 GMT
    /Users/runner/sage-local/lib/libgc.1.dylib
Sun, 02 Nov 2025 15:03:56 GMT
    /Users/runner/sage-local/lib/libgmp.10.dylib

…olExecutor

…te#257

mkoeppe · 2025-11-04T02:25:31Z

Testing at https://github.com/passagemath/passagemath/actions/runs/19055757176?pr=1755

delocate/delocating.py

… of bool

Co-authored-by: Kyle Benesch <[email protected]>

mkoeppe requested review from HexDecimal and matthew-brett as code owners October 31, 2025 03:01

mkoeppe marked this pull request as draft October 31, 2025 03:59

HexDecimal requested changes Oct 31, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

HexDecimal reviewed Oct 31, 2025

View reviewed changes

delocate/tools.py Outdated Show resolved Hide resolved

mkoeppe force-pushed the sanitize_once_only branch 3 times, most recently from c96a9a3 to 5a0d0d9 Compare November 1, 2025 19:46

This comment was marked as outdated.

Sign in to view

mkoeppe marked this pull request as ready for review November 1, 2025 19:59

HexDecimal reviewed Nov 1, 2025

View reviewed changes

delocate/tools.py Outdated Show resolved Hide resolved

Codesign libraries in parallel

cf70af3

Refactor code to reduce redundant codesigns

HexDecimal mentioned this pull request Nov 1, 2025

Improve codesign performance #260

Draft

5 tasks

mkoeppe force-pushed the sanitize_once_only branch from 5a0d0d9 to 26f28be Compare November 2, 2025 06:28

mkoeppe added a commit to passagemath/passagemath that referenced this pull request Nov 2, 2025

.github/workflows/dist.yml (wheels) [macos]: Use matthew-brett/deloca…

7f33df4

…te#257

delocate/delocating.py: Sanitize each file only once

a81c29c

mkoeppe force-pushed the sanitize_once_only branch from 26f28be to a81c29c Compare November 2, 2025 06:58

delocate/delocating.py (_sanitize_rpaths): Parallelize using ThreadPo…

a8bc94c

…olExecutor

mkoeppe changed the title ~~delocate/delocating.py: Sanitize each file only once~~ delocate/delocating.py: Sanitize each file only once, parallelize Nov 4, 2025

mkoeppe added a commit to passagemath/passagemath that referenced this pull request Nov 4, 2025

.github/workflows/dist.yml (wheels) [macos]: Use matthew-brett/deloca…

33300f6

…te#257

mkoeppe mentioned this pull request Nov 4, 2025

Optionally use joblib to parallelize delocating #256

Closed

5 tasks

HexDecimal reviewed Nov 4, 2025

View reviewed changes

delocate/delocating.py Outdated Show resolved Hide resolved

mkoeppe and others added 4 commits November 3, 2025 18:42

delocate/tools.py (_remove_absolute_rpaths): Return set[Path] instead…

13d5bae

… of bool

delocate/delocating.py: Update use of _remove_absolute_rpaths

7ebe5ee

Co-authored-by: Kyle Benesch <[email protected]>

delocate/delocating.py: Codestyle fix

493289b

Add type annotations

4844ef3

delocate/delocating.py: Sanitize each file only once, parallelize #257

Are you sure you want to change the base?

delocate/delocating.py: Sanitize each file only once, parallelize #257

Conversation

mkoeppe commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

HexDecimal commented Oct 31, 2025

Uh oh!

mkoeppe commented Oct 31, 2025

Uh oh!

HexDecimal commented Oct 31, 2025

Uh oh!

HexDecimal commented Oct 31, 2025

Uh oh!

mkoeppe commented Oct 31, 2025

Uh oh!

HexDecimal left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HexDecimal commented Oct 31, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

codecov bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mkoeppe commented Oct 31, 2025

Uh oh!

HexDecimal commented Nov 1, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

mkoeppe commented Nov 2, 2025

Uh oh!

mkoeppe commented Nov 2, 2025

Uh oh!

mkoeppe commented Nov 2, 2025

Uh oh!

mkoeppe commented Nov 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mkoeppe commented Oct 31, 2025 •

edited

Loading

HexDecimal left a comment •

edited

Loading

codecov bot commented Oct 31, 2025 •

edited

Loading