-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add DisableArcadeExcludeFromBuildSupport property #14414
Conversation
<Import Project="ExcludeFromBuild.BeforeCommonTargets.targets"/> | ||
that are loaded before the import of Arcade's Sdk.targets. See info in ExcludeFromBuild. | ||
Repositories that don't require this infrastructure can set DisableArcadeExcludeFromBuildSupport to true to disable it. --> | ||
<Import Project="ExcludeFromBuild.BeforeCommonTargets.targets" Condition="'$(DisableArcadeExcludeFromBuildSupport)' != 'true'" /> |
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.
If this is removed, will the target framework filtering still work as expected?
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.
If a project is completely filtered out then no, but I think that's fine. As mentioned, repos which control their dependency graph, don't need either. We still want the TargetFrameworkFilters.BeforeCommonTargets.targets
import as that also enables the netstandard1.x flag target.
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 general the whole exclusion infrastructure is busted IMHO. Because of that, workarounds like the EmptySdk in the VMR exist. If filtered out projects would be statically defined at traversal then they wouldn't even be walked by msbuild which means not evaluated and therefore the fake Microsoft.WindowsDesktop.Sdk wouldn't be needed.
I think it's fine to continue using this Exclude mechanism for now but I will file an issue to discuss a better mechanism in the long-term. I.e. there are now ways to filter projects out from an sln with msbuild conditions, without a separate slnf file. I think most projects could use that mechanism.
But for this PR, it's absolutely fine if the TFM filtering mechanism doesn't work fully when DisableArcadeExcludeFromBuildSupport
is set.
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.
Yeah can you file an issue for the long term?
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.
Filed dotnet/source-build#4047
Related: #14413
In runtime we control our projects and which ones to exclude for source-build / unified-build via ProjectReference Conditions and the Microsoft.Build.Traversal SDK. That's a better mechanism to filter out projects as they aren't even visited by MSBuild (meaning no msbuild evaluation and no target invocation -> much better perf).
While c0c425d fixed the issue that the
IsTestProject
andIsTestUtilityProject
properties were read too early, it made that infrastructure now run in dotnet/runtime.Add an opt-out switch to disable the ExcludeFromBuild infrastructure entirely.