-
Notifications
You must be signed in to change notification settings - Fork 190
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
Do build time discovery of MVC ApplicationParts #598
Conversation
@nguerrera would you mind reviewing the task and the target? You have way more experience here than anybody on the Razor team. |
src/Razor/src/Microsoft.NET.Sdk.Razor/DiscoverMvcApplicationParts.cs
Outdated
Show resolved
Hide resolved
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.
eng/ and related changes look good but I didn't review the c# files
src/Razor/test/testapps/AppWithP2PReference/AppWithP2PReference.csproj
Outdated
Show resolved
Hide resolved
|
Followup in MVC: dotnet/aspnetcore#10271 |
src/Razor/src/Microsoft.NET.Sdk.Razor/ApplicationPartsProvider.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/DiscoverMvcApplicationParts.cs
Outdated
Show resolved
Hide resolved
....Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/ApplicationPartsProvider.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/ApplicationPartsProvider.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/ApplicationPartsProvider.cs
Outdated
Show resolved
Hide resolved
....Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets
Outdated
Show resolved
Hide resolved
<PropertyGroup Condition="'$(_ReferencesMicrosoftAspNetCoreApp)' == ''"> | ||
<_ReferencesMicrosoftAspNetCoreApp>@(FrameworkReference->AnyHasMetadataValue('Identity', 'Microsoft.AspNetCore.App'))</_ReferencesMicrosoftAspNetCoreApp> | ||
</PropertyGroup> | ||
</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.
I'd like to understand more about this? What kinds of test can't we write without it?
I'm also not sure that this works.
IIRC The dependencies of _DiscoverMvcApplicationParts
won't be executed unless _DiscoverMvcApplicationParts
is going to be. So using the dependencies of a target to configure whether the target runs doesn't super work.
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.
It's possible to run a target that determines whether a later target will run, but it is complex and subtle. If you can avoid that situation, please do so. If you can't, loop me back in and I'll look at it with a magnifying glass.
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 was obviously incorrect, but I think the scenario it's attempting to rule out is a vanishingly small number or projects (targets Razor or Web SDK, targets netcoreapp3.0
or later, but does not reference Microsoft.AspNetCore.App. Plus in the most ordinary case, it'll end up ruling that there's nothing to do here. Removed this
<_MvcAssemblyName Include="Microsoft.AspNetCore.Mvc.Razor" /> | ||
<_MvcAssemblyName Include="Microsoft.AspNetCore.Mvc.RazorPages" /> | ||
<_MvcAssemblyName Include="Microsoft.AspNetCore.Mvc.TagHelpers" /> | ||
<_MvcAssemblyName Include="Microsoft.AspNetCore.Mvc.ViewFeatures" /> |
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.
Do we need to list everything here and maintain that? What would happen if just listed Microsoft.AspNet.Mvc.Abstractions
?
What will we do if we do a big squish in the future? Make this depend on the TFM?
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.
What would happen if just listed Microsoft.AspNet.Mvc.Abstractions?
We want a way to avoid Mvc assemblies being treated as "ReferencesMvc" i.e. we don't want to treat Microsoft.AspNetCore.Mvc.Core
as an application part.
What will we do if we do a big squish in the future? Make this depend on the TFM?
I guess so. More importantly, I think it's generally ok assemblies listed here. If in 4.0, there's only Microsoft.AspNetCore.Mvc and none of the rest, the dependency lookup is still correct since Microsoft.AspNetCore.Mvc.Core
would generally never appear in your dependency graph
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.
I assume then that this works correctly when a non-existent assembly is specified (it kinda has to).
....Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets
Outdated
Show resolved
Hide resolved
....Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets
Outdated
Show resolved
Hide resolved
....Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets
Show resolved
Hide resolved
....Sdk.Razor/build/netstandard2.0/Microsoft.NET.Sdk.Razor.MvcApplicationPartsDiscovery.targets
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/build/netstandard2.0/Sdk.Razor.CurrentVersion.targets
Show resolved
Hide resolved
src/Razor/test/Microsoft.NET.Sdk.Razor.Test/Microsoft.NET.Sdk.Razor.Test.csproj
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/DiscoverMvcApplicationParts.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.NET.Sdk.Razor/DiscoverMvcApplicationParts.cs
Outdated
Show resolved
Hide resolved
ca171a1
to
16e0632
Compare
🆙 📅 |
e537363
to
58c7ff5
Compare
|
||
ApplicationPartAssemblyNames = assemblyNames.Count > 0 ? | ||
assemblyNames.ToArray() : | ||
Array.Empty<string>(); |
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.
I would hope that List<>.ToArray()
is smart enough to do this.
assemblyNames.ToArray() : | ||
Array.Empty<string>(); | ||
|
||
return !Log.HasLoggedErrors; |
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.
Will this actually happen? it's probably better if you just return true here right? Or do you have a reason to expect different.
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.
I really like and recommend this pattern. If you're careful it's slightly inefficient but harmless. When someone makes a small mistake (logs an error but a bug keeps the error from propagating all the way back out to the Execute()
return value), it prevents the super confusing "build succeeded with errors" case.
(In hindsight, it should have been void Execute()
with logging an error the way to fail it. Alas.)
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.
Zoinks! I've been overruled then 🤣
|
||
namespace Microsoft.AspNetCore.Razor.Tasks | ||
{ | ||
public class DiscoverMvcApplicationParts : Task |
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.
How would you feel about removing the MVC terminology from this? There's nothing MVC in it.
|
||
namespace Microsoft.AspNetCore.Razor.Tasks | ||
{ | ||
public class ResolveReferenceItem |
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.
internal?
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.
Use the renamed type in tests.
* Do build time discovery of MVC ApplicationParts \n\nCommit migrated from dotnet/razor@e79d560
* Do build time discovery of MVC ApplicationParts \n\nCommit migrated from dotnet/razor@e79d560 Commit migrated from dotnet/aspnetcore@b5a7daba2b8f
Use the result of RAR to identify dependencies that reference MVC