feat: add Python 3.13 compatibility#1251
Closed
vegerot wants to merge 3 commits into
Closed
Conversation
Commit f54b293 moved build_eden_command into the edenfs-client crate and added it as a dependency of clone. edenfs-client pulls in thrift, which requires the thrift1 compiler — unavailable in OSS builds. Fix by restoring the function locally in clone and dropping the edenfs-client dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
python3-sys 0.7.2 (unmaintained, final release) is missing two fields added in CPython 3.13's PyConfig struct: cpu_count and sys_path_0. This makes the Rust struct ~12 bytes too small, so PyConfig_InitPythonConfig overflows the buffer and corrupts the stack. Fix by over-allocating PyConfig with a repr(C) overflow guard, and avoiding direct access to any field whose offset is shifted by the missing cpu_count (executable, prefix, module_search_paths are either auto-computed by PyConfig_Read or only used on older Python / static builds). Also detect .git directories in infer_python_home() so the Python module path is found in git clones. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 13, 2026
|
@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D96429855. (Because this pull request was imported automatically, there will not be any future comments.) |
There was a problem hiding this comment.
Pull request overview
Updates Sapling’s OSS build/install and Rust components to improve installation behavior, work around Python 3.13 PyConfig layout changes, and decouple the clone crate from sapling-edenfs-client.
Changes:
- Add pre-install steps to stop
sl/chg-related processes duringinstall-oss. - Add a Rust-side workaround for Python 3.13
PyConfigstruct size/layout mismatch incmdpyinitialization. - Inline EdenFS command construction in
sapling-cloneand drop thesapling-edenfs-clientdependency.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
eden/scm/Makefile |
Adds daemon/process termination steps before installing OSS binaries. |
eden/scm/lib/commands/cmdpy/src/python.rs |
Introduces a workaround to avoid PyConfig overflow/layout issues with Python 3.13 and python3-sys 0.7.2. |
eden/scm/lib/clone/src/lib.rs |
Replaces edenfs_client::build_eden_command with a local helper for constructing the EdenFS command. |
eden/scm/lib/clone/Cargo.toml |
Removes the sapling-edenfs-client dependency after inlining logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| install-oss: oss | ||
| $(SL_NAME) --kill-chg-daemon | ||
| killall $(SL_NAME) || true |
Comment on lines
+150
to
+154
| #[repr(C, align(8))] | ||
| struct PyConfigStorage { | ||
| inner: ffi::PyConfig, | ||
| _overflow_guard: [u64; 8], | ||
| } |
Comment on lines
+203
to
212
| // | ||
| // WARNING: On Python 3.13+, config.prefix has the wrong Rust offset | ||
| // due to the missing cpu_count field in python3-sys 0.7.2. This code | ||
| // path (static_libpython) is not used on standard OSS builds, so this | ||
| // is acceptable for now. A proper fix requires vendoring python3-sys | ||
| // or migrating to PyO3. | ||
| check_status!( | ||
| ffi::PyConfig_SetString(&mut config, &mut config.prefix, to_wide(exe_dir)), | ||
| Some(config) | ||
| None | ||
| ); |
Comment on lines
+116
to
+135
| fn get_eden_clone_command(config: &dyn Config) -> Result<Command> { | ||
| let eden_command = config.get_opt::<String>("edenfs", "command")?; | ||
| let mut cmd = match eden_command { | ||
| Some(cmd) => Command::new(cmd), | ||
| None => return Err(EdenCloneError::MissingCommandConfig().into()), | ||
| }; | ||
|
|
||
| // allow tests to specify different configuration directories from prod defaults | ||
| if let Some(base_dir) = config.get_opt::<PathBuf>("edenfs", "basepath")? { | ||
| cmd.args([ | ||
| "--config-dir".into(), | ||
| base_dir.join("eden"), | ||
| "--etc-eden-dir".into(), | ||
| base_dir.join("etc_eden"), | ||
| "--home-dir".into(), | ||
| base_dir.join("home"), | ||
| ]); | ||
| } | ||
| Ok(cmd) | ||
| } |
7 tasks
|
This pull request has been merged in 35a82da. |
meta-codesync Bot
pushed a commit
that referenced
this pull request
Apr 20, 2026
Summary: Reinstalling sapling while a chg daemon (the persistent chg server forked from a prior `sl` invocation) is still alive can leave the daemon connected to the just-overwritten binary and cause subsequent `sl` invocations to misbehave or hang. Ask the daemon to exit cleanly before copying the new binary into place. The `|| true` keeps `make install-oss` working when no daemon is running, and the `sleep 1` gives the daemon a moment to release its socket before the new binary is copied over. This avoids relying on a broad `killall sl` (which would also terminate unrelated user `sl` processes); `--kill-chg-daemon` only targets the chg server. Pull Request resolved: #1251 Original GitHub author: Max Coplan <mchcopl@gmail.com> Reviewed By: quark-zju Differential Revision: D101298960 fbshipit-source-id: 9520aa8246f2e59521c45173a2df449b72ea23c1
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
python3-sys 0.7.2 (unmaintained, final release) is missing two fields added in
CPython 3.13's PyConfig struct: cpu_count and sys_path_0. This makes the Rust
struct ~12 bytes too small, so PyConfig_InitPythonConfig overflows the buffer
and corrupts the stack. Fix by over-allocating PyConfig with a repr(C) overflow
guard, and avoiding direct access to any field whose offset is shifted by the
missing cpu_count (executable, prefix, module_search_paths are either
auto-computed by PyConfig_Read or only used on older Python / static builds).
Also detect .git directories in infer_python_home() so the Python module path
is found in git clones.
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
Stack created with Sapling. Best reviewed with ReviewStack.