-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 analayzer to nuget #999
Conversation
</PropertyGroup> | ||
|
||
<Target Name="PrepareAnalyzerPackageAssets" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking a bit more on this, we could integrate this into the existing PreparePackageAssets
target. I think it would help eliminate a little bit of duplication, and ensure one usage of the PackageAsset
item.
Basically just change
machinelearning/src/Directory.Build.targets
Lines 5 to 21 in eb26489
<Target Name="PreparePackageAssets" | |
AfterTargets="Build" | |
Condition="'$(IncludeInPackage)' != ''"> | |
<ItemGroup> | |
<PackageAsset Include="$(TargetPath)" | |
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" /> | |
<PackageAsset Include="@(DebugSymbolsProjectOutputGroupOutput)" | |
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" /> | |
<PackageAsset Include="@(DocumentationProjectOutputGroupOutput)" | |
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" /> | |
</ItemGroup> | |
<Copy SourceFiles="@(PackageAsset)" | |
DestinationFolder="$(PackageAssetsPath)%(PackageAsset.RelativePath)" /> | |
</Target> |
to be:
<Target Name="PreparePackageAssets"
AfterTargets="Build">
<ItemGroup Condition="'$(IncludeInPackage)' != ''">
<PackageAsset Include="$(TargetPath)"
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" />
<PackageAsset Include="@(DebugSymbolsProjectOutputGroupOutput)"
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" />
<PackageAsset Include="@(DocumentationProjectOutputGroupOutput)"
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" />
</ItemGroup>
<ItemGroup Condition="'$(IncludeAnalyzerInPackage)' != ''">
<PackageAsset Include="$(TargetPath)"
RelativePath="$(IncludeAnalyzerInPackage)\analyzers\dotnet\cs" />
<PackageAsset Include="@(DebugSymbolsProjectOutputGroupOutput)"
RelativePath="$(IncludeAnalyzerInPackage)\analyzers\dotnet\cs" />
</ItemGroup>
<Copy SourceFiles="@(PackageAsset)"
DestinationFolder="$(PackageAssetsPath)%(PackageAsset.RelativePath)" />
</Target>
Then all you need to do in this file is set <IncludeAnalyzerInPackage>Microsoft.ML</IncludeAnalyzerInPackage>
like you are above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9e752f9
to
d8b4f9e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -15,6 +14,13 @@ | |||
RelativePath="$(IncludeInPackage)\lib\$(TargetFramework)" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition="'$(IncludeAnalyzerInPackage)' != ''"> | |||
<PackageAsset Include="$(TargetPath)" | |||
RelativePath="$(IncludeAnalyzerInPackage)\analyzers\dotnet\cs" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to double check that we don't need this dotnet
to be parallel with other tfms in the package to avoid the package installing and only supplying an analyzer on a unsupported framework.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, don't reintroduce this issue: #357.
I think this should be simple to test, I'm not familiar enough with NuGet conventions for analyzers to know if it would have this problem without testing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be an unfounded concern. I looked into how NuGet resolves these and its actually different than the rest of the content. Essentially these appear to be ignored by restore / package applicability checks, and just resolved via a separate content selection algo at build time (not restore time https://github.com/NuGet/NuGet.BuildTasks/blob/640c8e13a9b7ab6e86264a296638fbf3cc016ad1/src/Microsoft.NuGet.Build.Tasks/ResolveNuGetPackageAssets.cs#L371).
Net result, ignore this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!! Thanks for your attention though @ericstj
More completion for #778, where I add the code analyzer to the nuget.
Thanks for help from @eerhardt !