Skip to content

Use .rcdata to store trampoline type + path to python binary #15068

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.

Sign up for GitHub

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

Merged
zanieb merged 15 commits into from
Nov 9, 2025
Merged

Use .rcdata to store trampoline type + path to python binary #15068

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
e0b9c31
refactor(uv-trampoline): use PE resources to store trampoline type + …
paveldikov Aug 4, 2025
ee7e578
tmp: aarch64 trampolines
samypr100 Aug 8, 2025
185bc3f
chore(deps): move `windows` version to workspace
paveldikov Aug 11, 2025
cc9c2e5
chore(deps): regen lockfile post rebase
paveldikov Aug 15, 2025
97a47f7
refactor(uv-trampoline,uv-trampoline-builder): address feedback
paveldikov Aug 16, 2025
509165f
Review
zanieb Aug 28, 2025
fd2a9ce
chore(deps): reconcile lockfile
paveldikov Aug 28, 2025
3279712
Merge branch 'main' into trampoline
zanieb Sep 12, 2025
d4303fd
Restore handle API
zanieb Sep 12, 2025
5687577
Merge branch 'main' into trampoline
zanieb Oct 13, 2025
a5e08c8
Merge branch 'main' into trampoline
zanieb Nov 9, 2025
d8f67c5
Regenerate lockfile
zanieb Nov 9, 2025
d4131f2
Regenerate trampolines
zanieb Nov 9, 2025
153d854
fix: remove legacy p12 dependency by using modern approach
samypr100 Nov 9, 2025
f7e549d
fix: avoid updating windows
samypr100 Nov 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 132 additions & 64 deletions Cargo.lock
View file
Open in desktop

Large diffs are not rendered by default.

4 changes: 3 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ uuid = { version = "1.16.0" }
version-ranges = { git = "https://github.com/astral-sh/pubgrub", rev = "d8efd77673c9a90792da9da31b6c0da7ea8a324b" }
walkdir = { version = "2.5.0" }
which = { version = "8.0.0", features = ["regex"] }
windows = { version = "0.59.0", features = ["Win32_Globalization", "Win32_Security", "Win32_System_Console", "Win32_System_Kernel", "Win32_System_Diagnostics_Debug", "Win32_Storage_FileSystem", "Win32_System_Registry", "Win32_System_IO", "Win32_System_Ioctl"] }
windows = { version = "0.61.0", features = ["std", "Win32_Globalization", "Win32_System_LibraryLoader", "Win32_System_Console", "Win32_System_Kernel", "Win32_System_Diagnostics_Debug", "Win32_Storage_FileSystem", "Win32_Security", "Win32_System_Registry", "Win32_System_IO", "Win32_System_Ioctl"] }
Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zanieb I also don't think we need to change the windows version as part of this PR either. Everything should work fine if we keep the previous windows versions intact. That will minimize the changes to the lock file to be just isolated to rcgen.

Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After f7e549d windows version is now the same as main. It also removed small unrelated changes to windows_exception in this PR as a result of the upgrade.

Copy link
Member

@zanieb zanieb Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

windows-registry = { version = "0.5.0" }
wiremock = { version = "0.6.4" }
wmi = { version = "0.16.0", default-features = false }
Expand All @@ -215,7 +215,9 @@ hyper = { version = "1.4.1", features = ["server", "http1"] }
hyper-util = { version = "0.1.8", features = ["tokio"] }
ignore = { version = "0.4.23" }
insta = { version = "1.40.0", features = ["json", "filters", "redactions"] }
p12 = { version = "0.6.3" }
Copy link
Member

@zanieb zanieb Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should be using https://docs.rs/pkcs12/latest/pkcs12/ instead which is maintained?

Copy link
Member

@zanieb zanieb Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It actually looks like that doesn't support writing archives)

Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason P12 format was needed in particular?

Copy link
Member

@zanieb zanieb Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure.

I also briefly looked at https://github.com/ancwrd1/p12-keystore which seems fine too.

It'd be nice to use a dependency that comes from a higher trust source, even if it's just for a test.

Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the test and the Set-AuthenticodeSignature docs, I wonder if its possible to avoid P12 entirely by manually instantiating a System.Security.Cryptography.X509Certificates.X509Certificate2 bundle. I believe that would allows us to only rely on rcgen and not p12 (if it's possible). I'll give it a shot.

Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Success, although it will require use of pwsh rather than powershell because System.Security.Cryptography.X509Certificates.X509Certificate2::CreateFromPemFile is .NET 5+

Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CI should have pwsh already so I don't think its a problem and I don't think we truly need to test in legacy powershell in this particular case as we've proven it works with legacy powershell already using pfx. I'll push my changes soon in case you're interested.

Copy link
Collaborator

@samypr100 samypr100 Nov 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 153d854

Copy link
Contributor Author

@paveldikov paveldikov Nov 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! Re the 'why' question: I did it because of a (possibly incorrect) inference that pwsh on CI did not have the ability to modify the credential store

predicates = { version = "3.1.2" }
rcgen = { version = "0.14.3" }
similar = { version = "2.6.0" }
temp-env = { version = "0.3.6" }
test-case = { version = "3.3.1" }
Expand Down
11 changes: 6 additions & 5 deletions crates/uv-python/src/managed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use std::str::FromStr;

use fs_err as fs;
use itertools::Itertools;
use same_file::is_same_file;
use thiserror::Error;
use tracing::{debug, warn};
use uv_preview::{Preview, PreviewFeatures};
Expand All @@ -22,7 +21,7 @@ use uv_platform::{Error as PlatformError, Os};
use uv_platform::{LibcDetectionError, Platform};
use uv_state::{StateBucket, StateStore};
use uv_static::EnvVars;
use uv_trampoline_builder::{Launcher, windows_python_launcher};
use uv_trampoline_builder::{Launcher, LauncherKind};

use crate::downloads::{Error as DownloadError, ManagedPythonDownload};
use crate::implementation::{
Expand Down Expand Up @@ -649,12 +648,12 @@ impl ManagedPythonInstallation {
/// [`create_bin_link`].
pub fn is_bin_link(&self, path: &Path) -> bool {
if cfg!(unix) {
is_same_file(path, self.executable(false)).unwrap_or_default()
same_file::is_same_file(path, self.executable(false)).unwrap_or_default()
} else if cfg!(windows) {
let Some(launcher) = Launcher::try_from_path(path).unwrap_or_default() else {
return false;
};
if !matches!(launcher.kind, uv_trampoline_builder::LauncherKind::Python) {
if !matches!(launcher.kind, LauncherKind::Python) {
return false;
}
// We canonicalize the target path of the launcher in case it includes a minor version
Expand Down Expand Up @@ -922,6 +921,8 @@ pub fn create_link_to_executable(link: &Path, executable: &Path) -> Result<(), E
}),
}
} else if cfg!(windows) {
use uv_trampoline_builder::windows_python_launcher;

// TODO(zanieb): Install GUI launchers as well
let launcher = windows_python_launcher(executable, false)?;

Expand All @@ -938,7 +939,7 @@ pub fn create_link_to_executable(link: &Path, executable: &Path) -> Result<(), E
})
}
} else {
unimplemented!("Only Windows and Unix systems are supported.")
unimplemented!("Only Windows and Unix are supported.")
}
}

Expand Down
7 changes: 6 additions & 1 deletion crates/uv-trampoline-builder/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,19 @@ workspace = true

[dependencies]
uv-fs = { workspace = true }

fs-err = {workspace = true }
tempfile = { workspace = true }
thiserror = { workspace = true }
zip = { workspace = true }

[target.'cfg(target_os = "windows")'.dependencies]
windows = { workspace = true }

[dev-dependencies]
assert_cmd = { workspace = true }
assert_fs = { workspace = true }
anyhow = { workspace = true }
fs-err = { workspace = true }
p12 = { workspace = true }
rcgen = { workspace = true }
which = { workspace = true }
Loading
Loading