Skip to content

Remove dependency on netstandard.library.ref #20039

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

Merged
merged 1 commit into from
Mar 25, 2020
Merged

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Mar 20, 2020

This package isn't being produced out of dotnet/runtime anymore - we should just use the default version that is bundled with the SDK rather than listing a dependency on it.

CC @dagood @Pilchie @ericstj

@wtgodbe wtgodbe requested review from Pilchie, dagood and a team March 20, 2020 22:16
@wtgodbe wtgodbe requested a review from dougbu as a code owner March 20, 2020 22:16
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 20, 2020
@ericstj
Copy link
Member

ericstj commented Mar 20, 2020

Can't you just remove the reference and let the SDK provide the default one?

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 20, 2020

We probably could - looks like the only place we use the version is here:

<KnownFrameworkReference Update="NETStandard.Library">
<TargetingPackVersion Condition="'%(TargetFramework)' == 'netstandard2.1' and '$(IsServicingBuild)' != 'true'">$(NETStandardLibraryRefPackageVersion)</TargetingPackVersion>
</KnownFrameworkReference>

And core-sdk is already pinned to 2.1.0:

https://github.com/dotnet/core-sdk/blob/02ee648fc6ef21f615e98cf4fe0b0c7e57fb936a/eng/Version.Details.xml#L38-L40

So we should be able to get away with relying on them for the version & removing the workaround. @dougbu @JunTaoLuo do you remember if there was another reason that we needed that KnownFrameworkReference update?

@Pilchie
Copy link
Member

Pilchie commented Mar 20, 2020

Yeah, I'd prefer to remove the dependency over pinning if possible.

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 20, 2020

Updated

@wtgodbe wtgodbe changed the title Pin netstandard.library.ref dependency to 2.1.0 Remove dependency on netstandard.library.ref Mar 20, 2020
Copy link
Member

@Pilchie Pilchie left a comment

Choose a reason for hiding this comment

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

LGTM assuming the KnownFrameworkReference part is ok.

@dougbu
Copy link
Contributor

dougbu commented Mar 20, 2020

do you remember if there was another reason that we needed that KnownFrameworkReference update?

Believe that was added before the SDKs referenced the final .NET Standard library. Doubt we need the workaround in any branch anymore.

@ericstj dotnet/runtime is only 'master'. Do dotnet/core-setup and related repos still create / package NETStandard.Library in other branches? (@wtgodbe if not, please stand up PRs for when the other branches open using the https://github.com/dotnet/aspnetcore-internal/blob/master/docs/engineering/servicing-and-preview-fixes.md process.)

@dougbu
Copy link
Contributor

dougbu commented Mar 20, 2020

assuming the KnownFrameworkReference part is ok

Yup, what @wtgodbe did is perfect.

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 20, 2020

@dougbu I followed the instructions in https://github.com/dotnet/aspnetcore-internal/blob/master/docs/engineering/servicing-and-preview-fixes.md for this branch, is it expected there'd be merge conflicts?

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 20, 2020

3.1 PR (same branch): #20043

@dougbu
Copy link
Contributor

dougbu commented Mar 20, 2020

is it expected there'd be merge conflicts?

Yeah, I'd expect merge conflicts because you're touching eng/Versions.props -- one of the files that changes significantly between branches. Use the no-op merge approach I detailed near the bottom of https://github.com/dotnet/aspnetcore-internal/blob/master/docs/engineering/servicing-and-preview-fixes.md and restore your previous "fix for realz" changes in this branch -- separately.

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 24, 2020

@dougbu I'm confused - I'll need to create a new branch for master, correct? I can't modify this branch without messing up #20043.

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 24, 2020

Or is it saying I need to create a branch off of wtgodbe/standard that no-op merges #20043 to master, then separately create another PR that does the "right thing" in master off of a 3rd branch? And won't the "no-op" PR actually have some changes, since some but not all of that branch merges cleanly into master? This process seems very cumbersome...

@dougbu
Copy link
Contributor

dougbu commented Mar 25, 2020

@wtgodbe
Copy link
Member Author

wtgodbe commented Mar 25, 2020

Rebased this to be strictly against master, since the 3.1 change is unneeded

@wtgodbe wtgodbe merged commit 2e2a82d into master Mar 25, 2020
@wtgodbe wtgodbe deleted the wtgodbe/standard branch March 25, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants