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

Add Natvis visualizations for the Url type #783

Merged
merged 2 commits into from
Aug 18, 2022

Conversation

ridwanabdillahi
Copy link
Contributor

This change adds Natvis visualizations for the Url type in the url crate to help improve the debugging experience on Windows.

Debugging types such as the Url type under a Windows debugger, WinDbg or the Visual Studio debugger, can be a bit difficult to dissect exactly what data is contained in each type. The Rust compiler does have Natvis support for some types, but this is limited to some of the core libraries and not supported for external crates.

RFC 3191 proposes adding support for embedding debugging visualizations such as Natvis in a Rust crate. This RFC has been approved, merged and implemented.

Natvis is a framework that can be used to specify how types should be viewed under a supported debugger, such as the Windows debugger (WinDbg) and the Visual Studio debugger.

This PR adds:

  • Natvis visualizations for the Url type in the url module.
  • Tests for testing visualizers embedded in the url crate.
  • Updates to the CI pipeline to ensure tests for visualizers are run so they do not break silently.
  • Cleanups to the CI pipeline to manually select features to enable for testing, in addition to the default features. This allows unstable features to be enabled and tested separately from stable features.
  • A new debugger_visualizer feature for the url crate to enable the unstable debugger_visualizer feature.

@codecov-commenter
Copy link

Codecov Report

Merging #783 (46ec473) into master (a72f83d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #783   +/-   ##
=======================================
  Coverage   85.23%   85.23%           
=======================================
  Files          22       22           
  Lines        3894     3894           
=======================================
  Hits         3319     3319           
  Misses        575      575           
Impacted Files Coverage Δ
url/src/lib.rs 75.76% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Collaborator

@valenting valenting left a comment

Choose a reason for hiding this comment

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

This is great stuff, thank you. Please fix the warnings if possible.

debug_metadata/url.natvis Outdated Show resolved Hide resolved
@ridwanabdillahi
Copy link
Contributor Author

This is great stuff, thank you. Please fix the warnings if possible.

Great, I'll fix the warnings and update the PR. Thanks for the review!

Copy link
Collaborator

@valenting valenting left a comment

Choose a reason for hiding this comment

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

Please fix the CI / Test (ubuntu-latest, 1.45.0) and cargo fmt lint.
Thanks!

@ridwanabdillahi
Copy link
Contributor Author

ridwanabdillahi commented Aug 10, 2022

Please fix the CI / Test (ubuntu-latest, 1.45.0) and cargo fmt lint.
Thanks!

Sorry about the lint failures @valenting

Looking at the CI/Test failure, this is actually caused by a new version of the serde_derive crate that was published yesterday and was not caused by this change. The error shows that version 1.0.143 of the serde or serde_derive crates require the resolver Cargo feature which was not stable in the version of Cargo used by the Rust 1.45.0 toolchain.

I'm not entirely sure why this new version of these crates requires the resolver Cargo feature when the prior versions did not. I was able to reproduce this locally on the master branch by running a cargo update followed by a cargo build --all-targets.

One solution would be to explicitly depend on version 1.0.142 or lower of these crates but I'm not sure if that's what we want to do here. Thoughts?

@valenting
Copy link
Collaborator

One solution would be to explicitly depend on version 1.0.142 or lower of these crates but I'm not sure if that's what we want to do here. Thoughts?

How about we don't run the serde CI task on 1.45.0?

@ridwanabdillahi
Copy link
Contributor Author

How about we don't run the serde CI task on 1.45.0?

From what I can tell there isn't a specific serde CI task. If you mean disabling the entire 1.45.0 CI job then that should probably be a separate change from this PR with a relevant issue opened and linked.

I'm not sure if the url crate has a minimum supported Rust version (MSRV) that's dependent on the 1.45.0 Rust toolchain but the crate maintainers would probably be better served to answer that question.

@ridwanabdillahi
Copy link
Contributor Author

@valenting

I opened up an issue for this CI failure #785 and a corresponding PR #786 to update the minimum rust-version to 1.51.0 which is when the resolver feature was stabilized

@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #786) made this pull request unmergeable. Please resolve the merge conflicts.

…lizations do not become stale or broken.

Add a README for documenting how the debugger visualizers and how to embed/ test them.
Update the github actions CI workflow to manually enable features, in addition to the default features, to allow for testing unstable features separately.

Cleanup running tests with the serde feature enabled.

Update documentation noting the debugger_visualizer crate feature is an unstable feature.

Respond to PR comments. Fix unused variables warnings.

Fix lint errors

Enable features via `member:feature` syntax.
@valenting
Copy link
Collaborator

It seems the test failed on Windows.

@ridwanabdillahi
Copy link
Contributor Author

It seems the test failed on Windows.

The latest nightly compiler contained changes to the debuginfo type layout for enums. The Natvis definitions have been updated and the test is passing.

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.

4 participants