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.

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

Move IServerVariablesFeature to Http.Features #11007

Merged
merged 9 commits into from
Jun 11, 2019
Merged

Conversation

jkotalik
Copy link
Contributor

@jkotalik jkotalik commented Jun 7, 2019

  • Move IServerVariablesFeature to Http.Features and forward in Servers.IIS
  • Create GetServerVariable in Http.Extensions
  • Mark GetIISServerVariable obsolete in Servers.IIS

Addresses #10167

(ported from #10374)

@aspnet/build big question for you. We have a scenario with IIS tests that require us to test a 2.2 app with a new ASP.NET Core Module. Before, this app was taking a single dependency from 2.2 (Microsoft.AspNetCore.Server.IIS). With this change, there was a type conflict error CS0433: The type 'IServerVariablesFeature' exists in both 'Microsoft.AspNetCore.Http.Features, Version=3.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' and 'Microsoft.AspNetCore.Server.IIS, Version=2.2.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60' where two types with the same name existed in the same namespace. Instead of trying to resolve that, I did the "right" thing and made app actually target netcoreapp2.2. However, I'd imagine this could be a build nightmare (though I don't care about updating the versions). What are your thoughts?

@jkotalik jkotalik requested review from Tratcher and a team June 7, 2019 23:08
@jkotalik jkotalik requested a review from analogrelay as a code owner June 7, 2019 23:08
@natemcmaster
Copy link
Contributor

Re: type conflict. Given that Microsoft.AspNetCore.Http.Features is both shared framework and in a nuget package, it's conceivable a customer could run into issues. I believe the only reason Http.Features is a package is because the SignalR 3.0 client uses it.

One idea: cross-compile Microsoft.AspNetCore.Http.Features for netcoreapp3.0, and exclude the IServerVariablesFeature when building for netstandard2.0.

cc @davidfowl

@Tratcher
Copy link
Member

Tratcher commented Jun 8, 2019

@natemcmaster the type conflict was caused by referencing 2.2 and 3.0 AspNetCore assemblies from the same application which as a rule we do not support. I asked him to change the backcompat test asset project to use only AspNetCore 2.2 dependencies and that fixed the issue.

@Tratcher
Copy link
Member

Tratcher commented Jun 8, 2019

Hmm, Helix didn't like this.

F:\workspace\_work\1\s\.dotnet\x64\sdk\3.0.100-preview5-011568\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(233,5): error NETSDK1005: Assets file 'F:\workspace\_work\1\s\artifacts\obj\InProcessNewShimWebSite\project.assets.json' doesn't have a target for '.NETCoreApp,Version=v3.0'. Ensure that restore has run and that you have included 'netcoreapp3.0' in the TargetFrameworks for your project. [F:\workspace\_work\1\s\src\Servers\IIS\IIS\test\testassets\InProcessNewShimWebSite\InProcessNewShimWebSite.csproj]

That's an understandable assumption on Helix's part. Any way to override it?

@davidfowl
Copy link
Member

Re: type conflict. Given that Microsoft.AspNetCore.Http.Features is both shared framework and in a nuget package, it's conceivable a customer could run into issues. I believe the only reason Http.Features is a package is because the SignalR 3.0 client uses it.

One idea: cross-compile Microsoft.AspNetCore.Http.Features for netcoreapp3.0, and exclude the IServerVariablesFeature when building for netstandard2.0.

Hmm this is an interesting problem, we may need to move IFeatureCollection to a lower assembly and type forward.

@jkotalik jkotalik added this to the 3.0.0-preview7 milestone Jun 10, 2019
@jkotalik
Copy link
Contributor Author

As @Tratcher mentioned, this conflict was only occurring due to this test site referencing 2.2 and 3.0 assets. Doing that hasn't been a problem up until now.
Regarding:

F:\workspace\_work\1\s\.dotnet\x64\sdk\3.0.100-preview5-011568\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(233,5): error NETSDK1005: Assets file 'F:\workspace\_work\1\s\artifacts\obj\InProcessNewShimWebSite\project.assets.json' doesn't have a target for '.NETCoreApp,Version=v3.0'. Ensure that restore has run and that you have included 'netcoreapp3.0' in the TargetFrameworks for your project. [F:\workspace\_work\1\s\src\Servers\IIS\IIS\test\testassets\InProcessNewShimWebSite\InProcessNewShimWebSite.csproj]

IIRC this was the main reason I was hesitant on making this change: we don't target the 2.2 tfm anywhere else. @aspnet/build thoughts?

@Tratcher
Copy link
Member

@jkotalik could you use netcoreapp3.0? It's primarily the 2.2 package dependencies that matter here, no?

@jkotalik jkotalik merged commit d7fc06a into master Jun 11, 2019
@ghost ghost deleted the jkotalik/serverVariables branch June 11, 2019 04:16
@dev-thinks
Copy link

Because of this conflict, am not able to get IServerVariablesFeature in .net standard 2.0/2.1 and available in core app 3.0+ ?

@davidfowl
Copy link
Member

ASP.NET Core 3.x is .NET Core only. See https://docs.microsoft.com/en-us/aspnet/core/migration/22-to-30?view=aspnetcore-3.1&tabs=visual-studio#migrate-libraries-via-multi-targeting

@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants