Skip to content

Restrict MacOS symbol exports to the Python module init#3172

Merged
G-D-Petrov merged 3 commits into
masterfrom
gpetrov/macos_symbol_export
Jun 17, 2026
Merged

Restrict MacOS symbol exports to the Python module init#3172
G-D-Petrov merged 3 commits into
masterfrom
gpetrov/macos_symbol_export

Conversation

@G-D-Petrov

@G-D-Petrov G-D-Petrov commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Reference Issues/PRs

What does this implement or fix?

ld64 on MacOS lacks --exclude-libs,ALL, which leads to symbols from statically linked libraries"leaking" in our wheel.
This PR makes sure that exports are restricted to the module init symbol to stop dyld weak-coalescing of statically-linked deps (e.g. pyarrow's bundled AWS SDK in libarrow) from aborting in aws_fatal_assert.

Any other comments?

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

@G-D-Petrov G-D-Petrov added patch Small change, should increase patch version no-release-notes This PR shouldn't be added to release notes. labels Jun 16, 2026
@G-D-Petrov G-D-Petrov marked this pull request as ready for review June 16, 2026 10:01
@claude

claude Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

ArcticDB Code Review Summary

Build-only change restricting the macOS extension's exported symbols to _PyInit_arcticdb_ext, with packaging and runtime regression tests. The change is sound: the symbol name correctly accounts for the macOS leading-underscore ABI, the export restriction only fires on the non-conda (wheel) AppleClang path where HIDE_LINKED_SYMBOLS is ON, which exactly matches the MACOS_WHEEL_BUILD test skip guard, and pybind's default-visibility PyInit survives the restriction. Both a nm-based packaging check and a pyarrow-before-arcticdb runtime regression are included.

The latest commits add unrelated test/CI cleanups: pinning the conda Windows runner to windows-2022, introducing an unparametrized latest_venv session fixture (used by test_comp_venv_loading_path), and removing stray trailing commas in two @pytest.mark.parametrize argname strings. All are benign — no correctness, safety, or behavioral impact.

PR Title and Description

  • Description is empty (template only). Per the review guidelines the description should explain what changed and why — here, that ld64 lacks --exclude-libs,ALL, so exports are restricted to the module init symbol to stop dyld weak-coalescing of statically-linked deps (e.g. pyarrow's bundled AWS SDK in libarrow) from aborting in aws_fatal_assert. The context already lives in the code comments and test docstrings; please lift a sentence or two into the description.

No correctness, memory-safety, API, on-disk-format, or build-configuration issues found. Labels (no-release-notes, patch) are appropriate for a build/test-only change.

@G-D-Petrov G-D-Petrov merged commit 73dac8e into master Jun 17, 2026
410 of 414 checks passed
@G-D-Petrov G-D-Petrov deleted the gpetrov/macos_symbol_export branch June 17, 2026 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-release-notes This PR shouldn't be added to release notes. patch Small change, should increase patch version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants