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

Fix Triple Slash Porter MSBuild loading #124

Merged
merged 6 commits into from
Oct 27, 2022

Conversation

carlossanlop
Copy link
Member

Some fixes to get the tool working back again, @RussKie @jeffhandley.

It works if you execute it manually. It doesn't work when executed from the unit tests due to dotnet/roslyn#61454.

Changes in this PR:

  • Updated the projects to net7.0, otherwise we are unable to load the target projects in dotnet/runtime.
  • Separated the code that loads the VS instance and the code that loads MSBuild, because as described in the documentation, you must not load Microsoft.Build APIs in the same context where you're loading MSBuildLocator APIs. This includes BinLogger APIs. https://docs.microsoft.com/en-us/visualstudio/msbuild/updating-an-existing-application
  • Made the project loading code much faster by loading the workspaces, projects and compilations only once.
  • Simplified the code that retrieves INamedTypeSymbol locations for a specified DocsType, and made sure to run this porting code in a single loop. Tests are different, they get executed in two loops because I want to show failures separately.
  • Adapted the tests to the new API surface. Will address the failures later when the related roslyn issue gets fixed.
  • Various small analyzer fixes (info and warning).

Once this is fixed, we can proceed to improve the code that loads the triple slash comments using the Roslyn APIs (we can try to apply @jeffhandley's refactoring).

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

LGTM

@carlossanlop
Copy link
Member Author

Need to re-enable the workflow job in this PR before merging: #131

@carlossanlop
Copy link
Member Author

Update: Someone posted a workaround in the roslyn issue and it worked for this, so I am now unblocked: dotnet/roslyn#61454

cc @smasher164

@carlossanlop
Copy link
Member Author

I have unblocked this PR by solving the building problem, locally, both for dotnet test and for the VS Test Explorer. It has uncovered two new, unrelated issues that I plan on fixing in separate PRs:

  • The CI is unable to run with .NET 7.0 (once again, this is not happening locally):
Error: /usr/share/dotnet/sdk/6.0.402/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.TargetFrameworkInference.targets(144,5):
 error NETSDK1045: The current .NET SDK does not support targeting .NET 7.0. 
Either target .NET 6.0 or lower, or use a version of the .NET SDK that supports .NET 7.0.
[/home/runner/work/api-docs-sync/api-docs-sync/src/PortToTripleSlash/src/app/PortToTripleSlash.csproj]
  • A newly uncovered test failure that happens because the symbol of the delegate can't be found, because we only look for types at the top level, and this delegate is a nested type.
 System.Exception : Symbol locations null for 'MyNamespace.MyType.MyDelegate'.

@smasher164 do you mind reviewing my PR?

{
throw new Exception("You must specify a *.binlog path.");
}
else if (Path.GetExtension(arg).ToUpperInvariant() != ".BINLOG")
Copy link
Member

Choose a reason for hiding this comment

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

so we're okay with .bInLoG?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's no big deal.

cancellationToken.ThrowIfCancellationRequested();

Debug.Assert(projectPath != null);
if (!TryGetResolvedWorkspace(projectPath, isMono, out ResolvedWorkspace? resolvedWorkspace))
Copy link
Member

Choose a reason for hiding this comment

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

newline before after if. same for other cases

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem. I can fix them as part of the next PR.

@carlossanlop carlossanlop merged commit 07cc619 into dotnet:main Oct 27, 2022
@carlossanlop carlossanlop deleted the FixBugsToTripleSlashPorter branch October 27, 2022 22:15
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.

3 participants