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.

Sign up for GitHub

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

Jump to bottom

Convert Razor to use Reference #4954

Merged
merged 1 commit into from
Dec 17, 2018
Merged

Convert Razor to use Reference #4954

merged 1 commit into from
Dec 17, 2018

Conversation

ajaybhargavb
Copy link
Contributor

No description provided.

@ajaybhargavb
Copy link
Contributor Author

The build is failing with,

2018-12-14T01:46:35.7094575Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\AppWithP2PReference\AppWithP2PReference.csproj : error NU1100: Unable to resolve 'Microsoft.NETCore.App (>= 2.0.0)' for '.NETCoreApp,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]
2018-12-14T01:46:35.7094862Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\ClassLibrary2\ClassLibrary2.csproj : error NU1100: Unable to resolve 'NETStandard.Library (>= 2.0.3)' for '.NETStandard,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]
2018-12-14T01:46:35.7095309Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\ClassLibrary\ClassLibrary.csproj : error NU1100: Unable to resolve 'NETStandard.Library (>= 2.0.3)' for '.NETStandard,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]
2018-12-14T01:46:35.7095746Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\SimpleMvcFSharp\SimpleMvcFSharp.fsproj : error NU1100: Unable to resolve 'Microsoft.NETCore.App (>= 2.0.0)' for '.NETCoreApp,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]
2018-12-14T01:46:35.7096170Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\SimpleMvcFSharp\SimpleMvcFSharp.fsproj : error NU1100: Unable to resolve 'FSharp.Core (>= 4.5.2)' for '.NETCoreApp,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]
2018-12-14T01:46:35.7096769Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\SimpleMvc\SimpleMvc.csproj : error NU1100: Unable to resolve 'Microsoft.NETCore.App (>= 2.0.0)' for '.NETCoreApp,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]
2018-12-14T01:46:35.7099009Z D:\a\1\s\src\Razor\Razor.Design\test\testassets\SimplePages\SimplePages.csproj : error NU1100: Unable to resolve 'Microsoft.NETCore.App (>= 2.0.0)' for '.NETCoreApp,Version=v2.0'. [C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.targets]

I'm not sure why. It works fine when I do dotnet build Razor.sln

natemcmaster
natemcmaster reviewed Dec 14, 2018
View reviewed changes
Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only noticed one minor thing - see below

The NU1100 error is caused by this file: https://github.com/aspnet/AspNetCore/blob/ajbaaska/convert-reference/src/Razor/Razor.Design/test/testassets/Directory.Build.props. It isn't importing restore sources.

eng/Dependencies.props Outdated Show resolved Hide resolved
eng/ProjectReferences.props
<ProjectReferenceProvider Include="rzc" ProjectPath="$(RepositoryRoot)src\Razor\Razor.Tools\src\rzc.csproj" />
<ProjectReferenceProvider Include="dotnet-razorpagegenerator" ProjectPath="$(RepositoryRoot)src\Razor\RazorPageGenerator\src\dotnet-razorpagegenerator.csproj" />
<ProjectReferenceProvider Include="Microsoft.AspNetCore.Razor" ProjectPath="$(RepositoryRoot)src\Razor\Razor\src\Microsoft.AspNetCore.Razor.csproj" />
<ProjectReferenceProvider Include="Microsoft.NET.Sdk.Razor" ProjectPath="$(RepositoryRoot)src\Razor\Sdk.Razor\src\Microsoft.NET.Sdk.Razor.csproj" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that <Reference Include="Microsoft.NET.Sdk.Razor" /> won't work in this reference system. ProjectReferences don't flow build assets, like targets and tasks. My hunch is that any projects still this in aspnet/AspNetCore will need to replace the package ref with <Sdk Name="Microsoft.NET.Sdk.Razor" />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there is no <Reference Include="Microsoft.NET.Sdk.Razor" /> anywhere in Razor

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do see a bunch of <Project Sdk="Microsoft.NET.Sdk.Razor">...</Project>. Does that still work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, <Project Sdk="Microsoft.NET.Sdk.Razor"> is functionally the same as <Project><Sdk Name="Microsoft.NET.Sdk.Razor">.

These are the references I had in mind:

There are probably more.

You don't have to address these in this PR, btw. Just made the note as it's something we'll need to address when we convert Identity and others. cc @JunTaoLuo

@ajaybhargavb ajaybhargavb requested a review from rynowak December 14, 2018 19:43
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/convert-reference branch 5 times, most recently from 8c97ab2 to 50c8211 Compare December 15, 2018 01:12
@ajaybhargavb
Copy link
Contributor Author

It passed 🙌 🎉

natemcmaster
natemcmaster approved these changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 nice work! Two minor things to fix, then :shipit:

build/submodules.props Outdated Show resolved Hide resolved
build/submodules.props Outdated Show resolved Hide resolved
@natemcmaster natemcmaster mentioned this pull request Dec 17, 2018
Closed
24 tasks
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/convert-reference branch from 50c8211 to ccb6a1f Compare December 17, 2018 19:33
@ajaybhargavb ajaybhargavb merged commit b079041 into release/2.1 Dec 17, 2018
@ajaybhargavb ajaybhargavb deleted the ajbaaska/convert-reference branch December 17, 2018 20:10
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
@ajaybhargavb
Convert Razor to use Reference (dotnet/aspnetcore#4954)
7a891e5 
Commit migrated from dotnet/aspnetcore@b079041488f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natemcmaster natemcmaster natemcmaster approved these changes

@rynowak rynowak Awaiting requested review from rynowak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ajaybhargavb @natemcmaster