Skip to content
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.

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

feat: copy .pdb files to Editable Installation and Wheel for easier debugging on windows #2220

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Next Next commit
feat: copy .pdb files to Editable Installation and Wheel for ea…
…sier debugging on windows
  • Loading branch information
WSH032 committed Sep 14, 2024
commit e3fd1f18436a485223ad97efe438c0049cee3b9c
27 changes: 23 additions & 4 deletions src/build_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ use crate::auditwheel::{PlatformTag, Policy};
use crate::build_options::CargoOptions;
use crate::compile::{warn_missing_py_init, CompileTarget};
use crate::module_writer::{
add_data, write_bin, write_bindings_module, write_cffi_module, write_python_part,
write_uniffi_module, write_wasm_launcher, WheelWriter,
add_data, include_artifact_for_editable_install, write_bin, write_bindings_module,
write_cffi_module, write_python_part, write_uniffi_module, write_wasm_launcher, WheelWriter,
};
use crate::project_layout::ProjectLayout;
use crate::python_interpreter::InterpreterKind;
Expand Down Expand Up @@ -187,6 +187,11 @@ pub struct BuildContext {
pub universal2: bool,
/// Build editable wheels
pub editable: bool,
/// Whether to include debug information(`.pdb` on msvc)
/// in the wheels or editable-installs.
/// Currently only support `.pdb` files with
/// the same name as the binary(`*.exe`/`*.dll`) on `msvc` platform
pub with_debuginfo: bool,
/// Cargo build options
pub cargo_options: CargoOptions,
}
Expand Down Expand Up @@ -692,6 +697,7 @@ impl BuildContext {
&self.target,
self.editable,
self.pyproject_toml.as_ref(),
self.with_debuginfo,
)
.context("Failed to add the files to the wheel")?;

Expand Down Expand Up @@ -770,6 +776,7 @@ impl BuildContext {
&self.target,
self.editable,
self.pyproject_toml.as_ref(),
self.with_debuginfo,
)
.context("Failed to add the files to the wheel")?;

Expand Down Expand Up @@ -861,7 +868,11 @@ impl BuildContext {
let maturin_build = artifact_path.parent().unwrap().join("maturin");
fs::create_dir_all(&maturin_build)?;
let new_artifact_path = maturin_build.join(artifact_path.file_name().unwrap());
fs::copy(artifact_path, &new_artifact_path)?;
include_artifact_for_editable_install(
artifact_path,
&new_artifact_path,
self.with_debuginfo,
)?;
artifact.path = new_artifact_path;
Ok(artifact)
}
Expand Down Expand Up @@ -893,6 +904,7 @@ impl BuildContext {
&self.interpreter[0].executable,
self.editable,
self.pyproject_toml.as_ref(),
self.with_debuginfo,
)?;

self.add_pth(&mut writer)?;
Expand Down Expand Up @@ -959,6 +971,7 @@ impl BuildContext {
self.target.target_os(),
self.editable,
self.pyproject_toml.as_ref(),
self.with_debuginfo,
)?;

self.add_pth(&mut writer)?;
Expand Down Expand Up @@ -1059,7 +1072,13 @@ impl BuildContext {
let mut artifacts_ref = Vec::with_capacity(artifacts.len());
for (artifact, bin_name) in &artifacts_and_files {
artifacts_ref.push(*artifact);
write_bin(&mut writer, &artifact.path, &self.metadata23, bin_name)?;
write_bin(
&mut writer,
&artifact.path,
&self.metadata23,
bin_name,
self.with_debuginfo,
)?;
if self.target.is_wasi() {
write_wasm_launcher(&mut writer, &self.metadata23, bin_name)?;
}
Expand Down
7 changes: 7 additions & 0 deletions src/build_options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,12 @@ pub struct BuildOptions {
#[arg(long)]
pub zig: bool,

/// Include debug information in the Wheel.
/// Currently only support `.pdb` files with
/// the same name as the binary(`*.exe`/`*.dll`) on `msvc` platform
#[arg(long)]
Copy link
Member

Choose a reason for hiding this comment

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

This can't be used together with --strip. (Not sure if clap supports this kind of setup though)

Suggested change
#[arg(long)]
#[arg(long, conflicts_with = "strip")]

Copy link
Author

@WSH032 WSH032 Sep 20, 2024

Choose a reason for hiding this comment

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

On MSVC, even if RUSTFLAGS="-Cstrip=symbols" is specified, rustc(at least with version 1.81 that I used) will still generate a .pdb file, so we can't let it conflict. Not sure how it behaves on macOS.

Copy link
Member

@messense messense Sep 23, 2024

Choose a reason for hiding this comment

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

But does the .pdb file contain useful information?

pub with_debuginfo: bool,

/// Cargo build options
#[command(flatten)]
pub cargo: CargoOptions,
Expand Down Expand Up @@ -719,6 +725,7 @@ impl BuildOptions {
cargo_metadata,
universal2,
editable,
with_debuginfo: self.with_debuginfo,
cargo_options,
})
}
Expand Down
5 changes: 5 additions & 0 deletions src/develop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ pub struct DevelopOptions {
/// Use `uv` to install packages instead of `pip`
#[arg(long)]
pub uv: bool,
/// The same as `--with-debuginfo` in `maturin build`
#[arg(long)]
pub with_debuginfo: bool,
}

#[instrument(skip_all)]
Expand Down Expand Up @@ -301,6 +304,7 @@ pub fn develop(develop_options: DevelopOptions, venv_dir: &Path) -> Result<()> {
pip_path,
cargo_options,
uv,
with_debuginfo,
} = develop_options;
let mut target_triple = cargo_options.target.as_ref().map(|x| x.to_string());
let target = Target::from_target_triple(cargo_options.target)?;
Expand All @@ -326,6 +330,7 @@ pub fn develop(develop_options: DevelopOptions, venv_dir: &Path) -> Result<()> {
skip_auditwheel: false,
#[cfg(feature = "zig")]
zig: false,
with_debuginfo,
cargo: CargoOptions {
target: target_triple,
..cargo_options
Expand Down
152 changes: 122 additions & 30 deletions src/module_writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,103 @@ fn handle_cffi_call_result(
}
}

struct ArtifactDebuginfoPaths {
pub(self) target_path: PathBuf,
pub(self) source_path: PathBuf,
}

fn get_msvc_artifact_debuginfo_path_to_write(
artifact_target_path: &Path,
artifact_source_path: &Path,
) -> Result<ArtifactDebuginfoPaths> {
const DEBUGINFO_EXTENSION: &str = "pdb";

if !cfg!(target_env = "msvc") {
bail!("`--with-debuginfo` is only supported on `MSVC` targets")
}

let artifact_name: &Path = artifact_source_path
.file_name()
.ok_or(anyhow!(
"Failed to get file name of {:?}",
artifact_source_path
))?
.as_ref();
let artifact_debuginfo_name = artifact_name.with_extension(DEBUGINFO_EXTENSION);

let artifact_target_dir = artifact_target_path.parent().ok_or(anyhow!(
"Failed to get parent directory of {:?}",
artifact_target_path
))?;
// On msvc, rustc emits the linker flags `/DEBUG` and `/PDBALTPATH:%_PDB%`,
// so the `.pdb` file should keep the same name as the artifact.
// For example, if the artifact is `foo.dll`, the target path is `/mylib/foo.cp310-win_amd64.pyd`,
// then the debuginfo target path should be `/mylib/foo.pdb`(i.e. not `/mylib/foo.cp310-win_amd64.pdb`).
let artifact_debuginfo_target_path = artifact_target_dir.join(artifact_debuginfo_name);

let artifact_debuginfo_source_path = artifact_source_path.with_extension(DEBUGINFO_EXTENSION);

Ok(ArtifactDebuginfoPaths {
target_path: artifact_debuginfo_target_path,
source_path: artifact_debuginfo_source_path,
})
}

/// Currently, `with_debuginfo` is only supported on `MSVC` targets.
fn write_artifact_to_module_writer(
writer: &mut impl ModuleWriter,
target_path: &Path,
source_path: &Path,
with_debuginfo: bool,
) -> Result<()> {
// We can't use add_file since we need to mark the file as executable
writer.add_file_with_permissions(target_path, source_path, 0o755)?;

if with_debuginfo {
let ArtifactDebuginfoPaths {
target_path: debuginfo_target_path,
source_path: debuginfo_source_path,
} = get_msvc_artifact_debuginfo_path_to_write(target_path, source_path)?;

// `.pdb` file doesn't need to be executable, so `0o644` is enough.
writer.add_file(debuginfo_target_path, debuginfo_source_path)?;
}
Ok(())
}

fn copy_artifact(artifact: &Path, target: &Path) -> Result<()> {
// Remove existing so file to avoid triggering SIGSEV in running process
// See https://github.com/PyO3/maturin/issues/758
debug!("Removing {}", target.display());
let _ = fs::remove_file(target);

debug!("Copying {} to {}", artifact.display(), target.display());
fs::copy(artifact, target).context(format!(
"Failed to copy {} to {}",
artifact.display(),
target.display()
))?;
Ok(())
}

/// Currently, `with_debuginfo` is only supported on `MSVC` targets.
pub(crate) fn include_artifact_for_editable_install(
artifact: &Path,
target: &Path,
with_debuginfo: bool,
) -> Result<()> {
copy_artifact(artifact, target)?;
if with_debuginfo {
let ArtifactDebuginfoPaths {
target_path: debuginfo_target_path,
source_path: debuginfo_source_path,
} = get_msvc_artifact_debuginfo_path_to_write(target, artifact)?;

copy_artifact(&debuginfo_source_path, &debuginfo_target_path)?;
}
Ok(())
}

/// Copies the shared library into the module, which is the only extra file needed with bindings
#[allow(clippy::too_many_arguments)]
#[instrument(skip_all)]
Expand All @@ -803,6 +900,7 @@ pub fn write_bindings_module(
target: &Target,
editable: bool,
pyproject_toml: Option<&PyProjectToml>,
with_debuginfo: bool,
) -> Result<()> {
let ext_name = &project_layout.extension_name;
let so_filename = if is_abi3 {
Expand Down Expand Up @@ -830,23 +928,18 @@ pub fn write_bindings_module(
if let Some(python_module) = &project_layout.python_module {
if editable {
let target = project_layout.rust_module.join(&so_filename);
// Remove existing so file to avoid triggering SIGSEV in running process
// See https://github.com/PyO3/maturin/issues/758
debug!("Removing {}", target.display());
let _ = fs::remove_file(&target);

debug!("Copying {} to {}", artifact.display(), target.display());
fs::copy(artifact, &target).context(format!(
"Failed to copy {} to {}",
artifact.display(),
target.display()
))?;
include_artifact_for_editable_install(artifact, &target, with_debuginfo)?;
} else {
let relative = project_layout
.rust_module
.strip_prefix(python_module.parent().unwrap())
.unwrap();
writer.add_file_with_permissions(relative.join(&so_filename), artifact, 0o755)?;
write_artifact_to_module_writer(
writer,
&relative.join(&so_filename),
artifact,
with_debuginfo,
)?;
}
} else {
let module = PathBuf::from(ext_name);
Expand All @@ -870,7 +963,12 @@ if hasattr({ext_name}, "__all__"):
writer.add_file(module.join("__init__.pyi"), type_stub)?;
writer.add_bytes(module.join("py.typed"), None, b"")?;
}
writer.add_file_with_permissions(module.join(so_filename), artifact, 0o755)?;
write_artifact_to_module_writer(
writer,
&module.join(so_filename),
artifact,
with_debuginfo,
)?;
}

Ok(())
Expand All @@ -888,6 +986,7 @@ pub fn write_cffi_module(
python: &Path,
editable: bool,
pyproject_toml: Option<&PyProjectToml>,
with_debuginfo: bool,
) -> Result<()> {
let cffi_declarations = generate_cffi_declarations(crate_dir, target_dir, python)?;

Expand All @@ -905,11 +1004,7 @@ pub fn write_cffi_module(
"{extension_name}.so",
extension_name = &project_layout.extension_name
));
fs::copy(artifact, &target).context(format!(
"Failed to copy {} to {}",
artifact.display(),
target.display()
))?;
include_artifact_for_editable_install(artifact, &target, with_debuginfo)?;
File::create(base_path.join("__init__.py"))?
.write_all(cffi_init_file(&project_layout.extension_name).as_bytes())?;
File::create(base_path.join("ffi.py"))?.write_all(cffi_declarations.as_bytes())?;
Expand Down Expand Up @@ -943,13 +1038,14 @@ pub fn write_cffi_module(
cffi_init_file(&project_layout.extension_name).as_bytes(),
)?;
writer.add_bytes(module.join("ffi.py"), None, cffi_declarations.as_bytes())?;
writer.add_file_with_permissions(
module.join(format!(
write_artifact_to_module_writer(
writer,
&module.join(format!(
"{extension_name}.so",
extension_name = &project_layout.extension_name
)),
artifact,
0o755,
with_debuginfo,
)?;
}

Expand Down Expand Up @@ -1130,6 +1226,7 @@ pub fn write_uniffi_module(
target_os: Os,
editable: bool,
pyproject_toml: Option<&PyProjectToml>,
with_debuginfo: bool,
) -> Result<()> {
let UniFfiBindings {
name: binding_name,
Expand All @@ -1149,11 +1246,7 @@ pub fn write_uniffi_module(
let base_path = python_module.join(&project_layout.extension_name);
fs::create_dir_all(&base_path)?;
let target = base_path.join(&cdylib);
fs::copy(artifact, &target).context(format!(
"Failed to copy {} to {}",
artifact.display(),
target.display()
))?;
include_artifact_for_editable_install(artifact, &target, with_debuginfo)?;

File::create(base_path.join("__init__.py"))?.write_all(py_init.as_bytes())?;
if let Ok(read_dir) = fs::read_dir(&binding_dir) {
Expand Down Expand Up @@ -1194,7 +1287,7 @@ pub fn write_uniffi_module(
writer.add_file(module.join(binding_file.file_name()), binding_file.path())?;
}
}
writer.add_file_with_permissions(module.join(cdylib), artifact, 0o755)?;
write_artifact_to_module_writer(writer, &module.join(cdylib), artifact, with_debuginfo)?;
}

Ok(())
Expand All @@ -1206,6 +1299,7 @@ pub fn write_bin(
artifact: &Path,
metadata: &Metadata23,
bin_name: &str,
with_debuginfo: bool,
) -> Result<()> {
let data_dir = PathBuf::from(format!(
"{}-{}.data",
Expand All @@ -1216,9 +1310,7 @@ pub fn write_bin(

writer.add_directory(&data_dir)?;

// We can't use add_file since we need to mark the file as executable
writer.add_file_with_permissions(data_dir.join(bin_name), artifact, 0o755)?;
Ok(())
write_artifact_to_module_writer(writer, &data_dir.join(bin_name), artifact, with_debuginfo)
}

/// Adds a wrapper script that start the wasm binary through wasmtime.
Expand Down
4 changes: 4 additions & 0 deletions tests/cmd/build.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ Options:

Make sure you installed zig with `pip install maturin[zig]`

--with-debuginfo
Include debug information in the Wheel. Currently only support `.pdb` files with the same
name as the binary(`*[EXE]`/`*.dll`) on `msvc` platform

-q, --quiet
Do not print cargo log messages

Expand Down
3 changes: 3 additions & 0 deletions tests/cmd/develop.stdout
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ Options:
--uv
Use `uv` to install packages instead of `pip`

--with-debuginfo
The same as `--with-debuginfo` in `maturin build`

-h, --help
Print help (see a summary with '-h')

Expand Down
Loading
Loading