-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add NetFramework Configurations to OOB packages #39099
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to this area: @ViktorHofer |
...crosoft.Extensions.Caching.Abstractions/src/Microsoft.Extensions.Caching.Abstractions.csproj
Show resolved
Hide resolved
src/libraries/Microsoft.XmlSerializer.Generator/src/Microsoft.XmlSerializer.Generator.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Pipelines/ref/System.IO.Pipelines.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Http.Json/ref/System.Net.Http.Json.csproj
Outdated
Show resolved
Hide resolved
...s/System.Net.WebSockets.WebSocketProtocol/ref/System.Net.WebSockets.WebSocketProtocol.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Reflection.DispatchProxy/ref/System.Reflection.DispatchProxy.csproj
Outdated
Show resolved
Hide resolved
...aries/System.Reflection.MetadataLoadContext/ref/System.Reflection.MetadataLoadContext.csproj
Outdated
Show resolved
Hide resolved
28fd379
to
14d5e61
Compare
...ft.Extensions.FileProviders.Physical/pkg/Microsoft.Extensions.FileProviders.Physical.pkgproj
Outdated
Show resolved
Hide resolved
a4e14f8
to
e2fedd4
Compare
e2fedd4
to
4cd6dc2
Compare
61e1002
to
e532259
Compare
...oft.Extensions.FileProviders.Physical/src/Microsoft.Extensions.FileProviders.Physical.csproj
Outdated
Show resolved
Hide resolved
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
<EnableDefaultItems>true</EnableDefaultItems> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup> | ||
<!-- Ensure Assemblies are first resolved via targeting pack when targeting net461 --> | ||
<AssemblySearchPaths Condition="'$(TargetFramework)' == 'net461'">$(NuGetPackageRoot)\microsoft.targetingpack.netframework.v4.6.1\1.0.1\lib\net461\;$(AssemblySearchPaths)</AssemblySearchPaths> |
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.
Would it make sense to just stop binplacing NETStandardLibrary.NETFramework
into RefPath completely? That's only there to support loading/running .NETStandard assemblies on .NETCore, and with this change don't we aim to eliminate all the cases where we would need to do that?
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.
Or maybe this is just another opportunity for @ViktorHofer to clean up in his PR that eliminates the RefPath for netfx.
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'm pretty sure I tried doing exactly this on release 3.1 branch when I tried to fix this error initially and found many build breaks because of it, and I tried working through the errors but gave up since it felt like peeling an onion and I found the workaround for using AssemblySearchPaths instead. I'm happy to try this again if you want me to but I feel that the real time to fix this w ould be whenever we actually remove RefPath for netfx.
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 just don’t like this cruft in all our projects that area owners won’t understand. Let’s make sure it gets cleaned up.
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.
Functionally this looks ok. I’d like to clean up these projects before GA
…on't ship in their package
…ackage dependency
…Microsoft.Extensions.FileProviders.Physical.csproj Co-authored-by: Eric StJohn <[email protected]>
e1826b7
to
e590021
Compare
Do not review last change, I only made it so that we can run some CI check on allConfigurations and netfx legs given that there is a current backup on the Windows machine pools. Last change I made is temporarily changing allConfigurations and netfx to hosted pool as recommended by @safern, and once both checks pass I will revert that last yml change and merge this. |
Green CI on netfx and allConfigurations leg 😄 Here is the link to the build for future reference: https://dev.azure.com/dnceng/public/_build/results?buildId=730238&view=results I'll now undo last commit that made this temporary change and merge the PR. Thanks @ericstj and @safern for the review, I'll follow up and clean up the way we build implementations against targeting packs. |
e590021
to
1124b48
Compare
Fixes #36840
cc: @ericstj @Anipik
FYI: @maryamariyan since you are working on changes on Microsoft.Extensions that might conflict with this.
FYI: @GrabYourPitchforks since I'm changing some .cs files on Utf8String