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

Read SourceLink info and call service to retrieve source from there #57978

Merged
merged 14 commits into from
Dec 1, 2021
Merged

Read SourceLink info and call service to retrieve source from there #57978

merged 14 commits into from
Dec 1, 2021

Conversation

davidwengier
Copy link
Contributor

@davidwengier davidwengier commented Nov 24, 2021 •
edited
Loading

Part of #55834

This creates a source link service and uses it to download files, but there is no source link service implementation in here. That will come in a separate PR to main-vs-deps because it needs new debugger contracts. There is also a new logger interface that will be filled in later, in another PR, but better to plumb it through now so that the main-vs-deps change can be singular in nature

This also changes things so we only write a temp file for embedded source, and just use the existing source file if possible, or the source link source file, which the debugger will write to its temp folder. The latter is especially important so that breakpoints work.

@davidwengier davidwengier requested review from a team as code owners November 24, 2021 23:02
davidwengier
davidwengier commented Nov 24, 2021
View reviewed changes
src/EditorFeatures/CSharpTest/PdbSourceDocument/AbstractPdbSourceDocumentTests.cs
@@ -0,0 +1,303 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this is just a move from PdbSourceDocumentTests

davidwengier
davidwengier commented Nov 24, 2021
View reviewed changes
src/EditorFeatures/CSharpTest/PdbSourceDocument/NullResultMetadataAsSourceFileProvider.cs
@@ -0,0 +1,58 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a just move and rename

This was referenced Nov 26, 2021
Closed
Closed
ryzngard
ryzngard reviewed Nov 30, 2021
View reviewed changes
src/Setup/DevDivInsertionFiles/DevDivInsertionFiles.csproj
@@ -134,6 +134,8 @@
<_Dependency Remove="System.ValueTuple"/>
<_Dependency Remove="System.Security.AccessControl"/>
<_Dependency Remove="System.Security.Principal.Windows"/>
<_Dependency Remove="System.Text.Encodings.Web" />
<_Dependency Remove="System.Text.Json" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how these are part of this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not entirely sure either, but the story I'm telling myself is that System.Text.Json is a transitive dependencies from the SourceLink.Tools package, and Text.Encodings.Web is transitive of that, and I'm the lucky winner of being the first to reference System.Text.Json in Roslyn.

Basically I added the package reference, built on the command line, and an error told me to modify this file :)

ryzngard
ryzngard approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

This looks as good as I could guess for the change. Some open questions that I'd like answered but not worth blocking the PR for.

@runfoapp runfoapp bot mentioned this pull request Nov 30, 2021
Closed
@davidwengier
Copy link
Contributor Author

@dotnet/roslyn-infrastructure is there any issues with referencing System.Text.Json here, or the updates to DevDivInsertion.csproj? Would this need to target vs-deps?

@davidwengier davidwengier changed the base branch from main to release/dev17.1 December 1, 2021 00:31
@davidwengier
Copy link
Contributor Author

davidwengier commented Dec 1, 2021 •
edited
Loading

@dotnet/roslyn-infrastructure is there any issues with referencing System.Text.Json here, or the updates to DevDivInsertion.csproj? Would this need to target vs-deps?

Answer from @dibarbet: As long as the integration tests work and roslyn loads / F5 in public previews it should be fine (I think we're forcing public previews now)

VS does F5 successfully :)

@davidwengier
Copy link
Contributor Author

davidwengier commented Dec 1, 2021 •
edited
Loading

@jinujoseph for M2 approval. This is the first part of SourceLink, and once merged I'll rebase #58051 which adds the actual debugger dependency to -vs-deps. Debugger bits were merged to VS main today.

This was referenced Dec 1, 2021
Closed
Closed
Closed
@davidwengier davidwengier merged commit f17419c into dotnet:release/dev17.1 Dec 1, 2021
@davidwengier davidwengier deleted the SourceLinkServices branch December 1, 2021 20:40
@davidwengier davidwengier mentioned this pull request Dec 1, 2021
Merged
333fred added a commit to 333fred/roslyn that referenced this pull request Dec 3, 2021
@333fred
Merge remote-tracking branch 'upstream/main' into interp-nullable-imp…
7663d94 
…rovements

* upstream/main: (310 commits)
  Read SourceLink info and call service to retrieve source from there (dotnet#57978)
  Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598) (dotnet#58050)
  Snap 17.1 P2 (dotnet#58041)
  Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (dotnet#57576)
  Shorten paths in VS installation (dotnet#57726)
  Add comments
  Add new parser/lexer to the StackTraceAnalyzer (dotnet#57598)
  Fix await completion for expression body lambda
  Add tests
  Fix comment
  Honor option, and also improve formatting with comment
  Skip TestLargeStringConcatenation (dotnet#58035)
  Log runtime framework of remote host
  Mark EqualityContract property accessor as not auto-implemented (dotnet#57917)
  Fix typo in XML doc for GeneratorExtensions (dotnet#58020)
  Hold Receiver directly in bound node for implicit indexer access (dotnet#58009)
  Pass AnalysisKind instead of int
  Enable nullable reference types for TableDataSource
  Simplify 'interpolation' data, and move to an easier to consume System.Range approach for it (dotnet#57966)
  Add missing test for CallerArgumentExpression (dotnet#57805)
  ...
allisonchou added a commit to allisonchou/roslyn that referenced this pull request Dec 3, 2021
@allisonchou
Revert "Read SourceLink info and call service to retrieve source from…
d8e7ae6 
… there (dotnet#57978)"

This reverts commit f17419c.
@allisonchou allisonchou mentioned this pull request Dec 3, 2021
Closed
JoeRobich added a commit that referenced this pull request Dec 6, 2021
@JoeRobich @Youssef1313
@sharwell @bradselw @mavasani @genlu @CyrusNajmabadi @ryzngard @bernd5 @allisonchou @davidwengier
Merge main to main-vs-deps (#58066)
3e5ced3 
* Remove IDE0051 suppression

* Add LayoutKind

* LayoutKind.Sequential

Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>

* Create azure-pipelines-integration-dartlab.yml

* Update azure-pipelines-integration-dartlab.yml

* [Async lightbulb] Move suppression fixes to 'Low' priority bucket

Addresses second item in #56843

Prior to this change, all the analyzers that are not high-pri are added to the normal priority bucket and executed at once. This PR further breaks down the normal priority bucket into two buckets as follow:

1. `Normal`: Analyzers that produce at least one fixable diagnostic, i.e. have a corresponding code fixer.
2. `Low`: Analyzers without any fixable diagnostics, i.e. have no corresponding code fixer. These diagnostics will only have Suppression/Configuration quick actions, which are lower priority and always show up at the bottom of the light bulb.

This new bucketing allows us to reduce the number of analyzers that are executed in the Normal priority bucket and hence generate all the non-suppression/configuration code fixes faster in async light bulb.

* Cache last project and compilationWithAnalyzers

* Update test as normal priority bucket has one lesser code action

* Make shallow checkout optional for integration test CI

* Move pipeline checkout skips up a layer.

* Look up VS bootstrapper from build

* Revert skipPipelinesCheckout change

* Comment out checkout: none for now

* Added matrix

* Target topic branch in DartLab.Templates

* Update number of machines to 4

* Run test agent elevated

* Update pipeline description

* Default VS branch to main

* Address feedback

* Exclude unnecessary sqlite assemblies

* Convert namespace to file scoped when typing semicolon

* Working on tests

* Add tests

* Add comments

* Add comments

* Revert

* Revert

* Make static

* Add tests

* Pass AnalysisKind instead of int

* Honor option, and also improve formatting with comment

* Fix comment

* Add tests

* Fix await completion for expression body lambda

* Add new parser/lexer to the StackTraceAnalyzer (#57598)

No functional changes, just moving to the new API and cleaning up unused code

* Add comments

* Make it possible to analyze the dataflow of `ConstructorInitializerSyntax` and `PrimaryConstructorBaseTypeSyntax` (#57576)

Fixes #57577.

* Snap 17.1 P2 (#58041)

* Add new parser/lexer to the StackTraceAnalyzer (#57598) (#58050)

No functional changes, just moving to the new API and cleaning up unused code

* Read SourceLink info and call service to retrieve source from there (#57978)

* Merge pull request #58100 from dotnet/dev/jorobich/skip-test

Skip flaky CommandLine.ArgumentParsing test

See #58077 for more details

Co-authored-by: Youssef Victor <31348972+Youssef1313@users.noreply.github.com>
Co-authored-by: Sam Harwell <sam@tunnelvisionlabs.com>
Co-authored-by: Brad White <4261702+bradselw@users.noreply.github.com>
Co-authored-by: Manish Vasani <mavasani@microsoft.com>
Co-authored-by: Joey Robichaud <jorobich@microsoft.com>
Co-authored-by: Brad White <bradwhit@microsoft.com>
Co-authored-by: gel@microsoft.com <gel@microsoft.com>
Co-authored-by: Cyrus Najmabadi <cyrusn@microsoft.com>
Co-authored-by: Sam Harwell <Sam.Harwell@microsoft.com>
Co-authored-by: Andrew Hall <andrha@microsoft.com>
Co-authored-by: Bernd Baumanns <baumannsbernd@gmail.com>
Co-authored-by: Allison Chou <allichou@microsoft.com>
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
Co-authored-by: David Wengier <david.wengier@microsoft.com>
Co-authored-by: Gen Lu <genlu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzngard ryzngard ryzngard approved these changes

Assignees
No one assigned
Labels
Approved to merge Area-IDE
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davidwengier @ryzngard @jinujoseph