Move DependencyModel to libraries#34296
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. |
|
error: This is unfortunate that we shipped this API already. Would this be OK to make a breaking change in
However, this would be a binary and potentially source breaking change to change |
|
We found a similar bug in other extensions libraries #33998. Since its just extension methods that folks aren't going to directly reference it's not a terrible type conflict. I'd just suppress the error for now and file a bug. If folks have a scenario for using the two types and notice the clash we can address it. runtime/src/libraries/pkg/test/packageTest.targets Lines 24 to 25 in 562232c
What do you suggest? Removing is breaking, we could obsolete it and/or implement on top of System.HashCode? |
src/libraries/Microsoft.Extensions.DependencyModel/Directory.Build.props
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.DependencyModel/Directory.Build.props
Outdated
Show resolved
Hide resolved
...braries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@safern do you think that the versions of these packages should live in Versions.props?
There was a problem hiding this comment.
Both Microsoft.DotNet.InternalAbstractions and Microsoft.DotNet.ProjectModel are obsolete packages (think project.json and .xproj time frame). The issue is our unit tests are written relying on these old packages, and we never took the time to rewrite the tests. But we don't want to lose the test coverage.
These versions will never be updated, so I didn't think we would want them in a global/central place.
FluentAssertions is different - this version could get updated in the future. However since no other test in src\libraries appears to use this package, my hope is we can remove this dependency in the future.
There was a problem hiding this comment.
I think it is fine to keep them here. Maybe FluentAssertions as you mention might be good to move it, but since you intend to remove it, I don't have an strong opinion.
There was a problem hiding this comment.
Do we intend to use FluentAssertions more in the repo? If we wanted to use it more, then I'd be OK with moving it. But I was thinking we didn't want to use it (since no one uses it today).
There was a problem hiding this comment.
My impression is that we don't want to - as it's inconsistent with what we've been doing (amongst some other reasons). if we want to, we should have a discussion first and achieve consensus.
|
Left a few comments but this Looks good in general. |
Moving DependencyModel to the same folder and infrastructure as the rest of our libraries, and out of the installer folder. I also dropped support for anything below netstandard2.0 at this time. Contributes to dotnet#3470 Fix dotnet#3425
…plicated type with System.Collections.
eerhardt
force-pushed
the
MoveDependencyModel
branch
from
63f665d to
2e20e8b
Compare
Also, clean up references to the old DependencyModel project.
Done - #34420
My suggestion is to make the breaking change in |
This also means we start building DependencyModel for net461 to ensure full framework support works correctly, and doesn't pick up the old net451 asset.
| @@ -0,0 +1,11 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
| <PropertyGroup> | |||
| <TargetFrameworks>netstandard2.0;net461</TargetFrameworks> | |||
There was a problem hiding this comment.
NIT: technically since you are not shipping the ref in the package and the API surface is not changing between netstandard2.0 and net461, it should be fine to only keep the netstandard2.0 configuration in this project if you wanted.
...braries/Microsoft.Extensions.DependencyModel/src/Microsoft.Extensions.DependencyModel.csproj
Outdated
Show resolved
Hide resolved
…tonsoft.Json for the harvested package assets.
| <ReferencePath | ||
| Include="$(RefPath)*.dll" | ||
| Exclude="$(RefPath)$(MSBuildProjectName).dll;$(RefPath)netstandard.dll" /> | ||
| Exclude="$(RefPath)$(MSBuildProjectName).dll;$(RefPath)netstandard.dll;$(RefPath)Microsoft.Extensions.DependencyModel.dll" /> |
There was a problem hiding this comment.
Shouldn't we be able to just prefer one of the seed types so that we can still generate the shims using this dependency? I'm actually not too familiar with this so if @ericstj suggested this that's fine.
There was a problem hiding this comment.
I'm not aware of a better fix here. If there is one, I'd be happy to change this.
Honestly, I'm not sure why we are passing Microsoft.Extensions.* libraries in here at all.
There was a problem hiding this comment.
We pass in the whole RefPath (line 17 above) if you think it would make sense to remove all of these that would be fine. Basically the idea of this shims are so that if some library tries to load types that have been moved into these Microsoft.Extensions* packages for core, that we redirect the types correctly.
There was a problem hiding this comment.
You could, however I'm not sure how much value that has. We don't intent for any Micrsoft.Extensions assemblies to satisfy .NETFramework nor .NETStandard surface area, so I am fine omitting them.
ghost
locked as resolved and limited conversation to collaborators
7 participants
Moving DependencyModel to the same folder and infrastructure as the rest of our libraries, and out of the installer folder.
I also dropped support for building live anything below netstandard2.0 so we don't need to reference Newtonsoft. The lower TFMs are harvested from the previously shipped package.
Contributes to #3470
Fix #3425