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

Add NetFramework Configurations to OOB packages #39099

Merged
merged 9 commits into from
Jul 15, 2020

Conversation

joperezr
Copy link
Member

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

@joperezr joperezr requested review from ericstj and Anipik July 10, 2020 20:31
@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

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.

@Dotnet-GitSync-Bot
Copy link
Collaborator

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.

@ghost
Copy link

ghost commented Jul 10, 2020

Tagging subscribers to this area: @ViktorHofer
Notify danmosemsft if you want to be subscribed.

@joperezr joperezr requested a review from safern July 10, 2020 20:32
<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>
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

@ericstj ericstj left a 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

@safern safern mentioned this pull request Jul 15, 2020
7 tasks
@joperezr
Copy link
Member Author

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.

@joperezr
Copy link
Member Author

Green CI on netfx and allConfigurations leg 😄
image

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.

@joperezr joperezr merged commit b7c4cb7 into dotnet:master Jul 15, 2020
@joperezr joperezr deleted the AddNetfxConfigs branch July 15, 2020 05:00
@ViktorHofer
Copy link
Member

@joperezr please wait with addressing the follow-up feedback until #35606 is in as it overlaps.

@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port netfx configuration changes to master
6 participants