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: change DebugSession::source_by_path() to return SourceCode enum #758

Merged
merged 19 commits into from
Feb 17, 2023

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Feb 10, 2023

closes #735 - this changes DebugSession::source_by_path() to return one of two options, a real source code, or an http source link.

This is a breaking change so will require a new major version.

@github-actions
Copy link

github-actions bot commented Feb 10, 2023

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against b947346

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #758 (4ed61c5) into master (9f7ceef) will increase coverage by 0.28%.
The diff coverage is 92.06%.

❗ Current head 4ed61c5 differs from pull request most recent head b947346. Consider uploading reports for the commit b947346 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #758      +/-   ##
==========================================
+ Coverage   73.90%   74.19%   +0.28%     
==========================================
  Files          69       70       +1     
  Lines       14952    15112     +160     
==========================================
+ Hits        11051    11213     +162     
+ Misses       3901     3899       -2     

@vaind vaind marked this pull request as ready for review February 10, 2023 15:22
@vaind vaind requested a review from a team February 10, 2023 15:22
@vaind vaind changed the title feat: change source_by_path to return SourceCode enum feat: change DebugSession::source_by_path() to return SourceCode enum Feb 10, 2023
Copy link
Contributor

@loewenheim loewenheim 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 doing this!

symbolic-debuginfo/src/ppdb.rs Outdated Show resolved Hide resolved
symbolic-debuginfo/src/ppdb.rs Show resolved Hide resolved
symbolic-debuginfo/src/sourcebundle.rs Outdated Show resolved Hide resolved
symbolic-ppdb/src/format/sourcelinks.rs Outdated Show resolved Hide resolved
symbolic-ppdb/src/format/mod.rs Outdated Show resolved Hide resolved
symbolic-ppdb/src/format/sourcelinks.rs Outdated Show resolved Hide resolved
symbolic-ppdb/tests/test_ppdb.rs Outdated Show resolved Hide resolved
@vaind vaind force-pushed the feat/ppdb-sourcelink branch from 61ea71c to 524ebfd Compare February 11, 2023 12:51
symbolic-ppdb/tests/test_ppdb.rs Outdated Show resolved Hide resolved
symbolic-ppdb/tests/test_ppdb.rs Outdated Show resolved Hide resolved
let mut source_link_mappings = Vec::new();
for cdi in CustomDebugInformationIterator::new(&result, SOURCE_LINK_KIND)? {
let cdi = cdi?;
// Note: only handle module #1 (do we actually handle multiple modules in any way??)
Copy link
Member

Choose a reason for hiding this comment

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

I believe there is only ever one module as defined in the specification.

symbolic-ppdb/src/format/mod.rs Outdated Show resolved Hide resolved
symbolic-ppdb/src/format/sourcelinks.rs Outdated Show resolved Hide resolved
symbolic-ppdb/src/format/sourcelinks.rs Outdated Show resolved Hide resolved
symbolic-ppdb/src/format/sourcelinks.rs Show resolved Hide resolved
@vaind vaind mentioned this pull request Feb 16, 2023
@mitsuhiko
Copy link
Member

mitsuhiko commented Feb 17, 2023

Don't take this as blocking the merge but I think it might be sensible to think about this a bit more. These thoughts came to mind:

  • DWARF5 already has a way to refer to source code by MD5 (DW_LNCT_MD5) which could be used for a symbol server to look up sources
  • There are extensions to DWARF5 to embed source directly (Support DW_LNCT_LLVM_source gimli-rs/gimli#431)

This to me implied there are multiple ways to refer to source, some of which might exist concurrencly. I wonder if it makes sense to revisit this abstraction. If we do decide that the return value is an enum and you gotta deal with it there, then we should make this #[non_exhaustive] since we probably expect more things to be added in the future.

@vaind vaind force-pushed the feat/ppdb-sourcelink branch from cf65c78 to 647df89 Compare February 17, 2023 10:02
@vaind
Copy link
Collaborator Author

vaind commented Feb 17, 2023

Thanks @mitsuhiko, I've added the attribute.

@vaind vaind force-pushed the feat/ppdb-sourcelink branch from 647df89 to 1bde15b Compare February 17, 2023 10:04
@vaind vaind merged commit 6394b02 into master Feb 17, 2023
@vaind vaind deleted the feat/ppdb-sourcelink branch February 17, 2023 12:55
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.

Support .NET SourceLink
4 participants