Skip to content

Enable debug for all release, attach console for windows.[CPP-380][CPP-384] #139

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

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

john-michaelburke
Copy link
Collaborator

@john-michaelburke john-michaelburke commented Sep 30, 2021

Implements

Risks

Since rust is not building them for gnu, I have read you must rely on other tools for running the debug sessions. I tried Windgb but was not able to figure out how to set up the application to run with our symbols; however, the .pdb files are stored so maybe this is sufficient for now.

@john-michaelburke
Copy link
Collaborator Author

I wonder if this is related?
rust-lang/rust#41775

@silverjam
Copy link
Contributor

Hrmm, weird, can we try split-debuginfo? https://doc.rust-lang.org/cargo/reference/profiles.html#split-debuginfo -- this way the debug info isn't embedded in the binary itself.

@john-michaelburke
Copy link
Collaborator Author

Hrmm, weird, can we try split-debuginfo? https://doc.rust-lang.org/cargo/reference/profiles.html#split-debuginfo -- this way the debug info isn't embedded in the binary itself.

Alright false alarm. I was not running thread apply all bt to get the debug info from all threads and it looks like this is working.

That being said we currently globally disable debug info in our CI (unless this flag would supersede RUSTFLAGS?). Should I restrict that to only the benchmarks/code quality check so we have this enabled in built releases? @silverjam

@silverjam
Copy link
Contributor

Hrmm, weird, can we try split-debuginfo? https://doc.rust-lang.org/cargo/reference/profiles.html#split-debuginfo -- this way the debug info isn't embedded in the binary itself.

Alright false alarm. I was not running thread apply all bt to get the debug info from all threads and it looks like this is working.

That being said we currently globally disable debug info in our CI (unless this flag would supersede RUSTFLAGS?). Should I restrict that to only the benchmarks/code quality check so we have this enabled in built releases? @silverjam

Yeah, we can leave it disabled for everything except for the installer/release build, we should still try split-debuginfo too

@john-michaelburke
Copy link
Collaborator Author

Hrmm, weird, can we try split-debuginfo? https://doc.rust-lang.org/cargo/reference/profiles.html#split-debuginfo -- this way the debug info isn't embedded in the binary itself.

Alright false alarm. I was not running thread apply all bt to get the debug info from all threads and it looks like this is working.
That being said we currently globally disable debug info in our CI (unless this flag would supersede RUSTFLAGS?). Should I restrict that to only the benchmarks/code quality check so we have this enabled in built releases? @silverjam

Yeah, we can leave it disabled for everything except for the installer/release build, we should still try split-debuginfo too

split-debuginfo requires nightly it seems at least for linux AFAICT

@silverjam
Copy link
Contributor

Hrmm, weird, can we try split-debuginfo? https://doc.rust-lang.org/cargo/reference/profiles.html#split-debuginfo -- this way the debug info isn't embedded in the binary itself.

Alright false alarm. I was not running thread apply all bt to get the debug info from all threads and it looks like this is working.
That being said we currently globally disable debug info in our CI (unless this flag would supersede RUSTFLAGS?). Should I restrict that to only the benchmarks/code quality check so we have this enabled in built releases? @silverjam

Yeah, we can leave it disabled for everything except for the installer/release build, we should still try split-debuginfo too

split-debuginfo requires nightly it seems at least for linux AFAICT

Ah, bummer-- we'll need to make a copy of the binary then call strip to remove the debug info

Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

We need automation that does two things:

  • Copies the output binary into the artifacts for the release so that we can use this to debug crashes in the future
  • Calls strip on the binary to remove the debug info before it's packaged into the app distribution

I think we can wait to see where @notoriaga's PR #146 lands before finalizing this

@john-michaelburke john-michaelburke force-pushed the john-michaelburke/enable-debugging-release branch from b46efde to 643343c Compare October 29, 2021 23:20
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/enable-debugging-release branch from 643343c to 96895e6 Compare October 30, 2021 00:59
@john-michaelburke
Copy link
Collaborator Author

john-michaelburke commented Nov 1, 2021

Since rust is not building them for gnu, I have read you must rely on other tools for running the debug sessions. I tried Windgb but was not able to figure out how to set up the application to run with our symbols; however, the .pdb files are stored so maybe this is sufficient for now.

@silverjam
Copy link
Contributor

@john-michaelburke can you update the PR summary with your last comment please?

Makefile.toml Outdated
'''

[tasks.build-dist-install-entrypoint.windows]
script = '''
mkdir py39-dist/Scripts
cp target/release/entrypoint.exe py39-dist/Scripts/swiftnav-console.exe
cp target/release/entrypoint.pdb py39-dist/Scripts/swiftnav-console.pdb
Copy link
Contributor

Choose a reason for hiding this comment

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

We should just make the entrypoint program output an exe called "swiftnav-console" rather than renaming it

Makefile.toml Outdated
@@ -388,8 +388,10 @@ end
os = os_family
if eq ${os} windows
mv ./Scripts/swiftnav-console.exe .
mv ./Scripts/swiftnav-console.pdb .
Copy link
Contributor

Choose a reason for hiding this comment

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

We need pdb files for each file that we want to debug, so in addition to entrypoint pdb we'll need the pdb file for the backend

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I also removed the line that was previously removing .pdb files. But I'm not seeing anything related to the backend so I may need to dig around more.
Screen Shot 2021-11-01 at 2 55 13 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

The pdb for the backend is probably going to be the Rust target/release folder, or it might be in the build directory for Python module

Copy link
Contributor

Choose a reason for hiding this comment

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

Or... it could be a issue with PyO3 setuptools integration, maybe it's not reading the Cargo.toml profile? Maybe we need to enable this differently in the setup.py for the backend?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screen Shot 2021-11-01 at 4 44 45 PM

The question now is where should I copy it to? Just the Scripts folder with the swiftnav_console.exe/.pdb?

Copy link
Contributor

Choose a reason for hiding this comment

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

should be copied alongside the console_backend python module in the site-packages directory

@john-michaelburke john-michaelburke force-pushed the john-michaelburke/enable-debugging-release branch from 9116b31 to 4e71e74 Compare November 2, 2021 17:48
@john-michaelburke john-michaelburke changed the title Enable debug for all release. Enable debug for all release, attach console for windows.[CPP-380][CPP-384] Nov 2, 2021
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/enable-debugging-release branch from ce2f52c to b230c97 Compare November 3, 2021 00:23
@john-michaelburke john-michaelburke force-pushed the john-michaelburke/enable-debugging-release branch from b230c97 to f71d8d5 Compare November 3, 2021 00:55
@john-michaelburke john-michaelburke merged commit 0ba533c into main Nov 3, 2021
@john-michaelburke john-michaelburke deleted the john-michaelburke/enable-debugging-release branch November 3, 2021 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants