-
Notifications
You must be signed in to change notification settings - Fork 519
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
Refactor PackageVersions and dependencies #7037
base: main
Are you sure you want to change the base?
Conversation
tests/Aspire.Oracle.EntityFrameworkCore.Tests/Aspire.Oracle.EntityFrameworkCore.Tests.csproj
Outdated
Show resolved
Hide resolved
@@ -1,6 +1,10 @@ | |||
<Project Sdk="Microsoft.NET.Sdk.Web"> | |||
|
|||
<PropertyGroup> | |||
<!-- |
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.
Can you elaborate a bit more on this change? Not sure I'm following why the comment was moved but all package references were removed.
|
||
<!-- The following 2 groups are for packages that need to switch based on the .NET TFM being used. --> | ||
|
||
<ItemGroup> |
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.
NIT: Should this be conditioned as well for net8 tfm?
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.
My thinking was that these are unconditional, and then the "STS" version "updates" the PackageVersion. Either way could work, I don't have strong opinion. Do you?
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.
Couple of questions but looks great overall. Thanks!
In the dotnet/aspire repo today, the way we switch out dependency versions between net8 and net9 doesn't work if you only target a single TFM - net9.0. This is because we have an MSBuild Property condition based on TargetFramework, which isn't set at property evaluation time. To fix this, refactor the condition to be an ItemGroup condition on TargetFramework, which gets evaluated after properties get set, so TargetFramework is set. This follows the same approach used in dotnet/extensions. - To also follow dotnet/extensions (and so the property names aren't so long), I changed the PackageVersion suffix to just Version.
4e3334d
to
ba1000f
Compare
In the dotnet/aspire repo today, the way we switch out dependency versions between net8 and net9 doesn't work if you only target a single TFM - net9.0. This is because we have an MSBuild Property condition based on TargetFramework, which isn't set at property evaluation time.
To fix this, refactor the condition to be an ItemGroup condition on TargetFramework, which gets evaluated after properties get set, so TargetFramework is set. This follows the same approach used in dotnet/extensions.