[go: up one dir, main page]
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.

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

Jump to bottom

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
Open

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

wants to merge 7 commits into from
+673 −38

Conversation

WSH032
Copy link
@WSH032 WSH032 commented Sep 14, 2024 •
edited
Loading

Close #2213

What I Did

I added a --with-debuginfo option to both maturin dev and maturin build. Currently, it only works on msvc. When this option is specified:

  • maturin dev will copy the debug file (e.g., mylib.pdb) with the same name as the artifact (e.g., mylib.dll) to the editable install directory.
  • maturin build will include the debug file in the wheel.
  • For bin bindings, *.pdb will be distributed along with *.exe in the Scripts directory.

Even if developers do not generate debug information, there will be no impact as long as they don't manually specify --with-debuginfo.

Specifically, with the following configuration:

# Cargo.toml

[lib]
name = "lib_name"
# pyproject.toml

python-source = "my_project"
module-name = "my_project._lib_name"

The resulting directory structure would look like this:

my-project
├── pyproject.toml
├── Cargo.toml
├── my_project
│   ├── __init__.py
│   ├── bar.py
│   ├── lib_name.pdb
│   └── _lib_name.cp310-win_amd64.pyd
├── README.md
└── src
    └── lib.rs

Please note that the debug file is lib_name.pdb instead of _lib_name.pdb, due to the linker flag /PDBALTPATH:%_PDB%.

Why Didn't I Implement Support for macOS and Linux?

Linux

On Linux, the default split-debuginfo is off, so the debug information is included in the .so file. Therefore, no additional actions are needed by default.

macOS

Currently, the default value for split-debuginfo on macOS is packed, but it seems recommended to set it to unpacked, see https://internals.rust-lang.org/t/help-test-faster-incremental-debug-macos-builds-on-nightly/14016.

I tried to reference python-build-standalone's approach. I downloaded these three distribution packages:

I found that only the x86_64-pc-windows-msvc-install_only package contained .pdb debug information, while the other two platforms did not have any separate debug information. So, I am unsure how to implement this feature for those platforms.

Most importantly, I do not have experience developing on macOS, nor do I have a macOS machine to perform the necessary testing. Therefore, this will need to be left for others to implement.

Next, I will work on writing tests and documentation, but I would like to get your feedback on the current implementation first.

@netlify Netlify
Copy link
netlify bot commented Sep 14, 2024 •
edited
Loading

✅ Deploy Preview for maturin-guide ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit fe46b4c
🔍 Latest deploy log https://app.netlify.com/sites/maturin-guide/deploys/66e797f4a6c26f0008448d08
😎 Deploy Preview https://deploy-preview-2220--maturin-guide.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

messense
messense reviewed Sep 18, 2024
View reviewed changes
Copy link
Member
@messense messense left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

For Linux, when --with-debuginfo is presented, we can explicitly pass -C split-debuginfo=off to rustc, similarly for macOS we pass -C split-debuginfo=unpacked.

(And it'd be better to check for existing split-debuginfo rustc flag and error out when its value is incompatible with --with-debuginfo.)

messense
messense reviewed Sep 18, 2024
View reviewed changes
src/build_options.rs
/// 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 •
edited
Loading

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 •
edited
Loading

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?

@WSH032
Copy link
Author
WSH032 commented Sep 20, 2024 •
edited
Loading

Thank you for the review!

For Linux, when --with-debuginfo is presented, we can explicitly pass -C split-debuginfo=off to rustc, similarly for macOS we pass -C split-debuginfo=unpacked.

(And it'd be better to check for existing split-debuginfo rustc flag and error out when its value is incompatible with --with-debuginfo.)

I'm concerned that there is no simple and robust way to check the value of split-debuginfo. Developers can set it not only through the RUSTFLAGS environment variable but also via .cargo/config.toml. I'm unsure if Cargo provides an existing method to retrieve this value.

Considering the difficulty of confirming existing configurations and the uncertainty of whether developers are indeed generating the corresponding debug files, I implemented --with-debuginfo in a way that requires developers to manually specify it.

Specifically, when using --with-debuginfo, I prefer to have developers manually specify RUSTFLAGS="-Csplit-debuginfo=off" rather than having maturin automatically set it.

This commit introduced a regression on FreeBSD, which I couldn’t reproduce on Windows. I’m unsure if it’s a sporadic issue. Could we re-run the CI?

@messense
Copy link
Member

I'm concerned that there is no simple and robust way to check the value of split-debuginfo

It's handled by cargo-config2 which is already used in maturin.

Specifically, when using --with-debuginfo, I prefer to have developers manually specify RUSTFLAGS="-Csplit-debuginfo=off" rather than having maturin automatically set it.

I disagree, it should provide a seamless user experience by default w/o asking user to look up how to set the right RUSTFLAGS.

@WSH032 WSH032 mentioned this pull request Nov 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@messense messense messense left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Copy .pdb files to Editable Installations and Wheels for easier debugging on windows
2 participants
@WSH032 @messense