Skip to content

Convert Mvc to use Reference #6047

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
Merged
merged 2 commits into from
Dec 21, 2018
Merged

Convert Mvc to use Reference #6047

merged 2 commits into from
Dec 21, 2018

Conversation

@ajaybhargavb
Copy link
Contributor

@ajaybhargavb ajaybhargavb commented Dec 20, 2018

No description provided.

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/convert-reference branch from 51a9ab6 to 7ae67a7 Compare December 20, 2018 02:11
ajaybhargavb
ajaybhargavb commented Dec 20, 2018
View reviewed changes
build/dependencies.props Outdated
Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

I had to add these because tests were complaining about mixing Razor 2.1.2 and 2.1.1 versions

Copy link
Member

@rynowak rynowak Dec 20, 2018

Choose a reason for hiding this comment

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

Should these be named differently so they don't get auto-updated? These are fixed versions and shouldn't change right?

Copy link
Member

@rynowak rynowak Dec 20, 2018

Choose a reason for hiding this comment

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

oh nvm this is 2.1 😆

src/Mvc/src/Microsoft.AspNetCore.Mvc.Razor/Microsoft.AspNetCore.Mvc.Razor.csproj Outdated
Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

Had to leave this as explicit package reference

Copy link
Member

@rynowak rynowak Dec 20, 2018

Choose a reason for hiding this comment

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

why though?

Copy link
Contributor

@natemcmaster natemcmaster Dec 20, 2018

Choose a reason for hiding this comment

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

+1. Can you share what doesn't work if you change to Reference?

Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

Because I get errors like these in some of the functional tests

k3dz3hwj.fsu(32,59): error CS1705: Assembly 'Microsoft.AspNetCore.Razor.Runtime' with identity 'Microsoft.AspNetCore.Razor.Runtime, Version=2.1.2.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' uses 'Microsoft.AspNetCore.Razor, Version=2.1.2.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' which has a higher version than referenced assembly 'Microsoft.AspNetCore.Razor' with identity 'Microsoft.AspNetCore.Razor, Version=2.1.1.0, Culture=neutral, PublicKeyToken=adb9793829ddae60'

Copy link
Contributor

@natemcmaster natemcmaster Dec 20, 2018

Choose a reason for hiding this comment

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

Can you collect a binlog of the test project? It appears the test project is getting the wrong versions of dependencies, but I'm not sure why. Possibly due to Directory.Build.targets/props imports.

Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

Discussed offline. Filed #6070 to clean this up later

src/Mvc/src/Microsoft.AspNetCore.Mvc.TagHelpers/Microsoft.AspNetCore.Mvc.TagHelpers.csproj Outdated
Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

Same here

src/Mvc/test/Microsoft.AspNetCore.Mvc.TestCommon/Microsoft.AspNetCore.Mvc.TestCommon.csproj Outdated
Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

Same here

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/convert-reference branch from 7ae67a7 to 6a9fa7d Compare December 20, 2018 02:16
jkotalik
jkotalik reviewed Dec 20, 2018
View reviewed changes
jkotalik
jkotalik reviewed Dec 20, 2018
View reviewed changes
jkotalik
jkotalik reviewed Dec 20, 2018
View reviewed changes
.../benchmarks/Microsoft.AspNetCore.Mvc.Performance/Microsoft.AspNetCore.Mvc.Performance.csproj

<ItemGroup>
<ProjectReference Include="..\..\src\Microsoft.AspNetCore.Mvc\Microsoft.AspNetCore.Mvc.csproj" />
<Reference Include="Microsoft.AspNetCore.Mvc" />
Copy link
Contributor

@jkotalik jkotalik Dec 20, 2018

Choose a reason for hiding this comment

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

nit condense item group.

...est/Microsoft.AspNetCore.Mvc.FunctionalTests/Microsoft.AspNetCore.Mvc.FunctionalTests.csproj Outdated
<ProjectReference Include="..\WebSites\WebApiCompatShimWebSite\WebApiCompatShimWebSite.csproj" />
<ProjectReference Include="..\WebSites\XmlFormattersWebSite\XmlFormattersWebSite.csproj" />
<Reference Include="Microsoft.AspNetCore.Mvc.Testing" />
<Reference Include="ApiExplorerWebSite" />
Copy link
Contributor

@jkotalik jkotalik Dec 20, 2018

Choose a reason for hiding this comment

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

Oh nvrm I guess they are used. Why do we need these project references?

Copy link
Contributor Author

@ajaybhargavb ajaybhargavb Dec 20, 2018

Choose a reason for hiding this comment

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

This is how we set up our functional test infrastructure. https://github.com/aspnet/AspNetCore/blob/release/2.1/src/Mvc/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ApiExplorerTest.cs#L19

@ajaybhargavb
Copy link
Contributor Author

ajaybhargavb commented Dec 20, 2018

The CI is failing with

D:\a\1\s\.w\pkg\Microsoft.AspNetCore.Analyzers\Microsoft.AspNetCore.Analyzers.csproj : error NU1603: Microsoft.AspNetCore.Analyzers depends on Microsoft.AspNetCore.Mvc.Analyzers.Experimental (>= 0.1.7-servicing-20181219.45) but Microsoft.AspNetCore.Mvc.Analyzers.Experimental 0.1.7-servicing-20181219.45 was not found. An approximate best match of Microsoft.AspNetCore.Mvc.Analyzers.Experimental 0.2.0-preview1-34785 was resolved.

@natemcmaster, any ideas?

@natemcmaster
Copy link
Contributor

natemcmaster commented Dec 20, 2018

Microsoft.AspNetCore.Analyzers never shipped and we have no plans to ship it. Since it's causing issues, let's get rid of this dead code. See #6053

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/convert-reference branch from e83118a to 34f88cd Compare December 20, 2018 18:57
@ajaybhargavb ajaybhargavb mentioned this pull request Dec 20, 2018
Closed
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/convert-reference branch from 34f88cd to 9afdfdc Compare December 20, 2018 23:45
@ajaybhargavb
Copy link
Contributor Author

ajaybhargavb commented Dec 21, 2018

🆙 📅

@ajaybhargavb ajaybhargavb merged commit 7f17d09 into release/2.1 Dec 21, 2018
@ajaybhargavb ajaybhargavb deleted the ajbaaska/convert-reference branch December 21, 2018 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rynowak rynowak rynowak left review comments

+3 more reviewers

@dougbu dougbu dougbu left review comments

@jkotalik jkotalik jkotalik left review comments

@natemcmaster natemcmaster natemcmaster approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ajaybhargavb @natemcmaster @rynowak @dougbu @jkotalik