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

Change folder path to avoid implicit NuGet inclusions #1222

Merged
merged 1 commit into from
Aug 24, 2021

Conversation

sharwell
Copy link
Contributor

@sharwell sharwell commented Aug 19, 2021

@Mike-E-angelo I'm hoping we can use something like this PR to avoid needing to move these files completely outside the analyzers folder.

Fixes #1216 (comment)

@Mike-E-angelo
Copy link

Mike-E-angelo commented Aug 19, 2021

Yeah it appears that any reference to this:

	<ItemGroup>
        <Analyzer Include="$(MSBuildThisFileDirectory)../../analyzers/cs-roslyn40/*.dll" />
    </ItemGroup>

Generates exceptions at build time. I am curious on why removing it still loads it into ServiceHub.RoslynCodeAnalysisService.exe. If NuGet is auto-loading this into ServiceHub.RoslynCodeAnalysisService.exe shouldn't we remove this ItemGroup altogether?

That is, based on my understanding, the source generator generates the class files, which are then picked up by the traditional MSBuild process during build. The MSBuild process does not need to reference the above, correct? Still trying to understand things here. :)

@sharwell
Copy link
Contributor Author

sharwell commented Aug 19, 2021

OK this is going to be more annoying than I expected. I have to move these assemblies completely outside the analyzers folder. I'll update this PR.

https://github.com/dotnet/NuGet.BuildTasks/blob/b2f0c162994237c4a9c23af79d95f79847499ebd/src/Microsoft.NuGet.Build.Tasks/ResolveNuGetPackageAssets.cs#L468-L477

GitHub
The build tasks used to pick up package content from project.lock.json. - NuGet.BuildTasks/ResolveNuGetPackageAssets.cs at b2f0c162994237c4a9c23af79d95f79847499ebd · dotnet/NuGet.BuildTasks

@sharwell sharwell marked this pull request as ready for review August 19, 2021 21:28
@sharwell sharwell closed this Aug 19, 2021
@sharwell sharwell reopened this Aug 19, 2021
@clairernovotny
Copy link
Member

Should this be merged?

@clairernovotny clairernovotny merged commit cde1c90 into reactiveui:main Aug 24, 2021
@clairernovotny
Copy link
Member

I'm going to go with yes :) I was confused by the close/reopen.

@sharwell
Copy link
Contributor Author

I was confused by the close/reopen.

Build had issues the first time; close/reopen triggered a new one.

@sharwell sharwell deleted the repackage branch August 24, 2021 13:42
@Mike-E-angelo
Copy link

Mike-E-angelo commented Aug 24, 2021

Woohoo... I can confirm that this works like a champ now with 6.1.5-preview. Thank you so much for doing this! It really means a lot to me personally as there are a lot of fires burning in my world and this it makes one less of them. :)

I appreciate the dedication to ensuring a high level of quality to your work out there. 👍🙏

@clairernovotny
Copy link
Member

@sharwell should I release this as stable?

@sharwell
Copy link
Contributor Author

sharwell commented Aug 24, 2021

@clairernovotny the specific risk is as follows:

  • If a consumer uses this package outside of NuGet tooling, or in a manner that doesn't add a source generator assembly from the customized location, InterfaceStubGenerator will not execute. If the build is likely to still succeed in this scenario (i.e. no compiler errors due to missing generated code), it's possible this user would experience a silent breaking change. This is a risk area for Refit authors to review.
  • For a while, the incremental source generator APIs were considered experimental and were only enabled if a consumer manually enabled <LangVersion>preview</LangVersion> in their project. If this behavior has not changed, it's possible for a user to experience a silent break inside the IDE for Visual Studio 2022. This is a risk area for @chsienki to review.

All other potentially problematic cases should consistently use the old implementation (Roslyn 3.8 APIs) during builds, so even if there are bugs in the Roslyn 4.0 portion it should not manifest in the build outputs. Once confidence is built in the Roslyn 4.0 implementation, we can update the integration to leverage upcoming VBCSCompiler support for incrementally running source generators during builds.

@sharwell
Copy link
Contributor Author

sharwell commented Aug 24, 2021

I think we need to update this to gate the new incremental implementation on <LangVersion> in addition to IDE version.

https://github.com/dotnet/roslyn/blob/c083dae9fcb112d82c18b89687b9af65e7ef77b7/src/Compilers/CSharp/Portable/SourceGeneration/CSharpGeneratorDriver.cs#L27

GitHub
The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs. - roslyn/CSharpGeneratorDriver.cs at c083dae9fcb112d82c18b89687b9af65e7ef77b7 · dotnet/roslyn

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants