-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 Nullable Warnings & projectcacheplugin.csproj now targets net5.0 #6048
Fix Nullable Warnings & projectcacheplugin.csproj now targets net5.0 #6048
Conversation
…m intrinsic function. ProjectCachePlugin project uses net5.0
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.
LGTM
The It gets passed pipeline build just fine. This should be good to merge. |
<ProjectReference Include="..\Samples\ProjectCachePlugin\ProjectCachePlugin.csproj" Private="false" ReferenceOutputAssembly="false"> | ||
<SetTargetFramework Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">TargetFramework=$(FullFrameworkTFM)</SetTargetFramework> | ||
<SetTargetFramework Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and '$(MonoBuild)' == 'true'">TargetFramework=$(FullFrameworkTFM)</SetTargetFramework> | ||
<SetTargetFramework Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">TargetFramework=netcoreapp2.1</SetTargetFramework> | ||
<SetTargetFramework Condition="'$(TargetFrameworkIdentifier)' != '.NETFramework'">TargetFramework=net5.0</SetTargetFramework> | ||
</ProjectReference> |
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.
Is this project reference really needed? It appears to build and run fine without 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.
Somehow you want to specify that the sample project should be built before the test project. The test project does not have a compile time dependency on it, but a runtime dependency, because the tests are testing whether an arbitrary, previously unknown plugin assembly can be found and its types loaded and instantiated. I had hit issues with this dependency ordering while changing both test and sample, and this ProjectReference is the only way I found to make it work. I think it just happens to work without the ProjectReference because it's also declared in the solution. But that does not guarantee the correct ordering (and I'd rather not specify project ordering in solutions 😇).
…otnet#6048) Fixes official builds after the arcade update. This failed because arcade CI had merged with a successful (but stale) CI build.
…otnet#6048) Fixes official builds after the arcade update. This failed because arcade CI had merged with a successful (but stale) CI build.
Fixes CI
Context
Arcade PR passed CI -> projectcache was merged -> Arcade PR was merged using the stale CI success from before projectcache being merged.
Changes Made
ProjectCachePlugin.csproj
now targets net5.0.Microsoft.Build.Engine.UnitTests.csproj
assignsnet5.0
TargetFramework instead ofnetcoreapp2.1
.Relaxed nullable warnings that were turned into errors in pipeline builds. @cdmihai please double check
ProjectCacheItem.cs
for what I did with the nullable warnings.Testing
CI testing should be enough. If we want to play it safe, an
exp/
run should be good validation.Notes
Fun fact: Apparently,
Microsoft.Build.Engine.UnitTests.csproj
manually assigns a TargetFramework?