macOS support (for the fancy peeps with nice furniture)#406
Conversation
goodboy
added
non-linux
goodboy
changed the title
platformdirs for .config.get_rt_dir()There was a problem hiding this comment.
Pull request overview
Updates Tractor’s runtime-directory resolution to be cross-platform (including macOS) by switching from direct XDG_RUNTIME_DIR usage to platformdirs.
Changes:
- Add
platformdirsas a runtime dependency (and lock it inuv.lock). - Rework
tractor._state.get_rt_dir()to useplatformdirs.user_runtime_dir()instead ofXDG_RUNTIME_DIR. - Update the
get_rt_dir()docstring to reflect the new behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tractor/_state.py |
Switch runtime-dir selection to platformdirs and adjust get_rt_dir() API/behavior. |
pyproject.toml |
Add platformdirs>=4.4.0 to project dependencies. |
uv.lock |
Lock platformdirs and add it to the resolved dependency set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not rt_dir.is_dir(): | ||
| rt_dir.mkdir() |
There was a problem hiding this comment.
rt_dir.mkdir() can fail on macOS if platformdirs.user_runtime_dir() returns a nested path whose parents don’t exist, and it can also race between processes (TOCTOU) causing FileExistsError. Use mkdir(parents=True, exist_ok=True) (or platformdirs.user_runtime_dir(..., ensure_exists=True) if available) so directory creation is robust.
| if not rt_dir.is_dir(): | |
| rt_dir.mkdir() | |
| rt_dir.mkdir(parents=True, exist_ok=True) |
| On linux we take use a `'${XDG_RUNTIME_DIR}/tractor/` subdir by | ||
| default but equivalents are mapped for each platform using | ||
| the lovely `platformdirs`. |
There was a problem hiding this comment.
The docstring has a couple of issues: “take use” reads like a typo, and the quoting/backtick around ${XDG_RUNTIME_DIR}/tractor/ is unbalanced (opening ' without a closing one). Please fix the wording/markup so the documentation renders correctly and is unambiguous.
| On linux we take use a `'${XDG_RUNTIME_DIR}/tractor/` subdir by | |
| default but equivalents are mapped for each platform using | |
| the lovely `platformdirs`. | |
| On linux we use the `${XDG_RUNTIME_DIR}/tractor/` subdir by | |
| default, but equivalents are mapped for each platform using | |
| the `platformdirs` library. |
| ), | ||
| ) | ||
| if subdir: | ||
| rt_dir: Path = rt_dir / subdir |
There was a problem hiding this comment.
Allowing subdir to be a Path means callers can pass an absolute path (or .. segments) and cause get_rt_dir() to return a location outside the intended runtime directory (rt_dir / subdir discards the base if subdir is absolute). If subdir can ever be influenced by external input, validate it is a relative path (and consider rejecting ..), or keep the parameter as a simple relative str.
| rt_dir: Path = rt_dir / subdir | |
| # Normalize and validate that `subdir` is a relative path | |
| # without any parent-directory ("..") components, to prevent | |
| # escaping the runtime directory. | |
| subdir_path = subdir if isinstance(subdir, Path) else Path(subdir) | |
| if subdir_path.is_absolute(): | |
| raise ValueError('subdir must be a relative path') | |
| if any(part == '..' for part in subdir_path.parts): | |
| raise ValueError("subdir must not contain '..' components") | |
| rt_dir = rt_dir / subdir_path |
There was a problem hiding this comment.
Yup good idea, adding this.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(key) <= max_len: | ||
| return key | ||
|
|
||
| _hash: str = hashlib.sha256( | ||
| key.encode() | ||
| ).hexdigest() | ||
|
|
||
| hash_len: int = ( | ||
| (max_len - 1) |
There was a problem hiding this comment.
_shorten_key_for_macos() ignores the provided prefix/suffix when len(key) <= max_len (it returns key unchanged). This causes collisions on macOS in _make_token() because shm_name, shm_first_index_name, and shm_last_index_name can become identical for “short” keys, and it also drops the prefix for open_shm_list(). Adjust the early-return logic to include prefix + key + suffix and ensure the length check accounts for the leading '/' added by SharedMemory on POSIX as well as the prefix/suffix lengths.
| if len(key) <= max_len: | |
| return key | |
| _hash: str = hashlib.sha256( | |
| key.encode() | |
| ).hexdigest() | |
| hash_len: int = ( | |
| (max_len - 1) | |
| # Account for the leading '/' added by POSIX SharedMemory names | |
| usable_len: int = max_len - 1 | |
| full_key: str = f"{prefix}{key}{suffix}" | |
| if len(full_key) <= usable_len: | |
| return full_key | |
| _hash: str = hashlib.sha256( | |
| key.encode() | |
| ).hexdigest() | |
| hash_len: int = ( | |
| usable_len |
| if platform.system() == 'Darwin': | ||
| key: str = _shorten_key_for_macos( | ||
| key=key, | ||
| prefix=prefix, | ||
| ) | ||
|
|
There was a problem hiding this comment.
On macOS open_shm_list() may shorten the provided key, so shml.key (and the value passed to other processes) can differ from the original input. Existing tests/callers that assert equality with the original key will fail on Darwin for long keys (e.g. UUID-based keys). Consider documenting this behavior and/or preserving the original key separately so API consumers don’t assume ShmList.key == input key across platforms.
|
|
||
| if not rt_dir.is_dir(): | ||
| rt_dir.mkdir( | ||
| parents=True, |
There was a problem hiding this comment.
get_rt_dir() uses a check-then-create pattern (if not rt_dir.is_dir(): rt_dir.mkdir(...)) without exist_ok=True. In concurrent startup (multiple tasks/processes) this can raise FileExistsError if another process creates the directory between the check and the mkdir. Use mkdir(parents=True, exist_ok=True) (or catch FileExistsError) to make this race-safe.
| parents=True, | |
| parents=True, | |
| exist_ok=True, |
| # NOTE known to at least works on, | ||
| # - macos | ||
| else: | ||
| peer_pid: int = get_peer_pid(sock) |
There was a problem hiding this comment.
get_peer_pid() returns int | None, but in the non-Linux branch peer_pid is annotated as int. If get_peer_pid() fails, peer_pid will be None and the annotation becomes inaccurate (and will trip static type-checking). Consider annotating peer_pid as int|None here (or handling the None case explicitly) since UDSAddress.maybe_pid already supports None.
| peer_pid: int = get_peer_pid(sock) | |
| peer_pid: int|None = get_peer_pid(sock) |
| version = "0.1.0a6dev0" | ||
| description = 'structured concurrent `trio`-"actors"' | ||
| authors = [{ name = "Tyler Goodlet", email = "goodboy_foss@protonmail.com" }] | ||
| requires-python = ">= 3.11" | ||
| requires-python = ">=3.12, <3.14" | ||
| readme = "docs/README.rst" |
There was a problem hiding this comment.
requires-python is now >=3.12, <3.14, but the project classifiers still include Programming Language :: Python :: 3.11. This metadata mismatch can confuse package index consumers and tooling; please update the classifiers to reflect the supported Python versions (or adjust requires-python if 3.11 is still intended).
Per the `copilot` review, #406 (review) now we also, - pass `exists_ok=True` to `.mkdir()` to avoid conc races. - expose `appname: str` param for caller override. - normalize `subdir` to avoid escaping the base rt-dir location.
Per the `copilot` review, #406 (review) now we also, - pass `exists_ok=True` to `.mkdir()` to avoid conc races. - expose `appname: str` param for caller override. - normalize `subdir` to avoid escaping the base rt-dir location.
Thanks to the `tox`-dev community for such a lovely pkg which seems to solves all the current cross-platform user-dir problems B) Also this, - now passes `platformdirs.user_runtime_dir(appname='tractor')` and allows caller to pass an optional `subdir` under `tractor/` if desired. - drops the `.config._rtdir: Path` mod var. - bumps the lock file with the new dep.
Make socket credential imports platform-conditional in `.ipc._uds`. - Linux: use `SO_PASSCRED`/`SO_PEERCRED` from socket module - macOS: use `LOCAL_PEERCRED` (0x0001) instead, no need for `SO_PASSCRED` - Conditionally call `setsockopt(SO_PASSCRED)` only on Linux Fixes AttributeError on macOS where SO_PASSCRED doesn't exist. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Move the multi-platorm-supporting conditional/dynamic `socket` constant imports to *after* the main cross-platform ones. Also add constant typing and reformat comments a bit for the macOS case.
Adapt the `PSHMNAMLEN` fix from `piker.data._sharedmem` (orig commit 96fb79ec thx @Dnks!) to `tractor.ipc._shm` accounting for the module-local differences: - Add `hashlib` import for sha256 key hashing - Add `key: str|None` field to `NDToken` for storing the original descriptive key separate from the (possibly shortened) OS-level `shm_name` - Add `__eq__()`/`__hash__()` to `NDToken` excluding the `key` field from identity comparison - Add `_shorten_key_for_macos()` using `t_` prefix (vs piker's `p_`) with 16 hex chars of sha256 - Use `platform.system() == 'Darwin'` in `_make_token()` (tractor already imports the `platform` module vs piker's `sys.platform`) - Wrap `shm_unlink()` in `ShmArray.destroy()` with `try/except FileNotFoundError` for teardown races (was already done in `SharedInt.destroy()`) - Move token creation before `SharedMemory()` alloc in `open_shm_ndarray()` so `token.shm_name` is used as the OS-level name - Use `lookup_key` pattern in `attach_shm_ndarray()` to decouple `_known_tokens` dict key from OS name (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Same problem as for the `ShmArray` tokens, so tweak and reuse the `_shorten_key_for_macos()` helper and call it from `open_shm_list()` similarly. Some tweaks/updates to the various helpers, - support `prefix/suffix` inputs and if provided take their lengths and subtract them from the known *macOS shm_open() has a 31 char limit (PSHMNAMLEN)* when generating and using the `hashlib.sha256()` value which overrides (for now..) wtv `key` is passed by the caller. - pass the appropriate `suffix='_first/_last'` values for the `ShmArray` token generators case. - add a `prefix: str = 'shml_'` param to `open_shm_list()`. - better log formatting with `!r` to report any key shortening.
Though it was a good (vibed) try by @Dnks, the previous "fix" was not actually adding unix socket support but merely sidestepping a crash due to `get_peer_info()`'s impl never going to work on MacOS (and it was never intended to). This patch instead solves the underlying issue by implementing a new `get_peer_pid()` helper which does in fact retrieve the peer's PID in a more generic/cross-platform way (:fingers_crossed:); much thanks to the linked SO answer for this solution! Impl deats, - add `get_peer_pid()` and call it from `MsgpackUDSStream.get_stream_addrs()` when we detect a non-'linux' platform, OW use the original soln: `get_stream_addrs()`. - add a new case for the `match (peername, sockname)` with a `case (str(), str()):` which seems to at least work on macos. - drop all the `LOCAL_PEERCRED` dynamic import branching since it was never needed and was never going to work.
Per the questionable `copilot` review which is detailed for follow up in #418. These constants are directly linked from the kernel sources fwiw.
Per the `copilot` review, #406 (review) now we also, - pass `exists_ok=True` to `.mkdir()` to avoid conc races. - expose `appname: str` param for caller override. - normalize `subdir` to avoid escaping the base rt-dir location.
There's a very sloppy registrar-actor-bootup syncing approach used in
this fixture (basically just guessing how long to sleep to wait for it
to init and bind the registry socket) using a `global _PROC_SPAWN_WAIT`
that needs to be made more reliable. But, for now i'm just playing along
with what's there to try and make less CI runs flaky by,
- sleeping *another* 1s when run from non-linux CI.
- reporting stdout (if any) alongside stderr on teardown.
- not strictly requiring a `proc.returncode == -2` indicating successful
graceful cancellation via SIGINT; instead we now error-log and only
raise the RTE on `< 0` exit code.
* though i can't think of why this would happen other then an
underlying crash which should propagate.. but i don't think any test
suite does this intentionally rn?
* though i don't think it should ever happen, having a CI run
"error"-fail bc of this isn't all that illuminating, if there is
some weird `.returncode == 0` termination case it's likely not
a failure?
For later, see the new todo list; we should sync to some kind of "ping"
polling of the tpt address if possible which is already easy enough for
TCP reusing an internal closure from `._root.open_root_actor()`.
Via ensuring `all(mark.args)` on wtv expressions are arg-passed to the mark decorator; use it to skip the `test_subactor_breakpoint` suite when `ctlc=True` since it seems too unreliable in CI.
- convert all doc-strings to `'''` multiline style. - rename `nursery` -> `an`, `n` -> `tn` to match project-wide conventions. - add type annotations to fn params (fixtures, test helpers). - break long lines into multiline style for fn calls, assertions, and `parametrize` decorator lists. - add `ids=` to `@pytest.mark.parametrize`. - use `'` over `"` for string literals. - add `from typing import Callable` import. - drop spurious blank lines inside generators. (this commit msg was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
It seems something is up with their VM-img or wtv bc i keep increasing the subproc timeout and nothing is changing. Since i can't try a `-xlarge` one without paying i'm just muting this test for now.
Macos in ci
- add `"Operating System :: MacOS"` classifier. - add macOS bullet to README's TODO/status section. (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
Drop the separate `testing-macos` job and add `macos-latest` to the existing OS matrix; bump timeout to 16 min to accommodate macOS runs. (this patch was generated in some part by [`claude-code`][claude-code-gh]) [claude-code-gh]: https://github.com/anthropics/claude-code
A more series effort toward #404 and getting macOS running as part of
our CI!
There's been some new collabs entering the scene who are helping with
this so I will be piecewise making updates here as we work through
various fixes on other git-service-platforms.
Set of patches which make #342 green
platformdirsfor.config.get_rt_dir()._state.get_rt_dir()use.pikerShmListinternal keying.darwin kernel sources.
bashon macos similar to whatwe have already for
xonsh; makes the debugger test suites pass.Todo pre-Landing of #342 history on top of this,
cleanup history in Macos in ci #342.
which are unclearly mixed together.
via PR, merge Macos in ci #342 history into this.
ensure the GH
ci.ymlis in best shape it can be.CI entries B)
all green on CI runs for this branch 🙏