-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Consume dependencies updates automatically via darc #6676
Conversation
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreDesignPackageVersion)" PrivateAssets="All" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreSqlitePackageVersion)" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreSqlServerPackageVersion)" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="2.1.1" PrivateAssets="All" /> |
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.
@dougbu @sebastienros - It appears the "BenchmarkX" version variables haven't been updated since 2.1.1, and wasn't sure if you wanted to continue to do this manually or if you had other automation to override this. So,this PR isn't changing the version of this benchmark dependency, just the way it is set. I'm removing the variable until we have a strategy for updating this automatically via darc
.
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.
These versions were intentionally set to downlevel values. But, don't we have checks that disallow setting versions directly in the .csproj
files?
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.
Nope. That check is gone now.
FYI @chcosta |
<Uri>https://github.com/aspnet/EntityFrameworkCore</Uri> | ||
<Sha>000000</Sha> | ||
</Dependency> | ||
<Dependency Name="Microsoft.EntityFrameworkCore" Version="3.0.0-preview.18604.3"> |
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.
Random thought: You could just list this one from EF and use $(MicrosoftEntityFrameworkCoreVersion) everywhere.
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'd rather have a fully-defined dependency list. This will help us when doing static analysis for the impacts of changes like package deprecation, rename, one-off-builds, etc.
<MicrosoftExtensionsDependencyModelPackageVersion>3.0.0-preview-27219-3</MicrosoftExtensionsDependencyModelPackageVersion> | ||
<MicrosoftNETCoreAppPackageVersion>3.0.0-preview-27219-3</MicrosoftNETCoreAppPackageVersion> | ||
<MicrosoftDotNetPlatformAbstractionsPackageVersion>3.0.0-preview-27219-3</MicrosoftDotNetPlatformAbstractionsPackageVersion> | ||
|
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 don't think white space is preserved.
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.
Sigh.
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.
Probably a one-line fix. ...and a day to find the line, three days to discuss the change, and another day to get it approved, merge it, and get a new build. (lol, sorry--feeling pessimistic today)
@bricelam @rynowak running
I've checked the expected subscriptions exist. Maybe the build manifest isn't including this package, somehow?
cc @mmitche |
@natemcmaster investigating |
@@ -31,3 +31,4 @@ BenchmarkDotNet.Artifacts/ | |||
korebuild-lock.txt | |||
.gradle/ | |||
src/SignalR/clients/**/dist/ | |||
modules/ |
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.
/cc @sebastienros
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 kept accidentally committing submodules when switching between release/2.x and master branches.
I am fine with the changes on the Benchmarks apps. |
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.
Looks pretty right to me.
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.
Didn't read enough of this to approve. Please let me know if I should look deeper.
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreDesignPackageVersion)" PrivateAssets="All" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.Sqlite" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreSqlitePackageVersion)" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="$(BenchmarksOnlyMicrosoftEntityFrameworkCoreSqlServerPackageVersion)" /> | ||
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="2.1.1" PrivateAssets="All" /> |
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.
These versions were intentionally set to downlevel values. But, don't we have checks that disallow setting versions directly in the .csproj
files?
@@ -17,7 +17,8 @@ | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<PackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" Version="$(MicrosoftAspNetCoreHostingAbstractions20PackageVersion)" /> | |||
<!-- Hard-coded to 2.0 because this project currently targets netcoreapp2.0 --> | |||
<PackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" Version="2.0.0" /> |
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.
👍
Build and tests pass, so going to merge now. I will follow-up on why darc is missing some packages separately. It's at least partially due to missing config on AspNetCore-Tooling. |
This implements half of #4264 -- the half that consumes updates via darc.
The other half -- publishing to darc -- will come as a separate PR.