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 net9.0 support for EFCore components #5932

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Sep 25, 2024

Fixes #5207

Microsoft Reviewers: Open in CodeFlow

global.json Outdated Show resolved Hide resolved
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

Not blocking, but for futureproofing also change EnrichNpgsqlDbContext, EnrichMySqlDbContext, EnrichOracleDatabaseDbContext and EnrichCosmosDbContext to use ConfigureDbContext

Consider renaming AspireAzureEFCoreCosmosExtensions

</PropertyGroup>

<PropertyGroup Condition="'$(TargetFramework)' == 'net9.0'">
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need to do this split. Microsoft.EntityFrameworkCore 9.0-rc1 targets net8.0. Why do we need to keep referencing the version 8 packages when targeting net8.0? Why can't we just continue to only target net8.0 and depend on EF v9.0?

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 talked about it with @joperezr before starting. efcore 9.0 brings extensions dependencies on 9.0 which we would have to bump (logging, ...), and customers would then need these too, even when targeting 8.0. If we didn't do that users who would like to stay on 9.0 for support reasons would not be able to.

Copy link
Member

Choose a reason for hiding this comment

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

I don't like this situation we are putting people (especially library authors) into.

You can reference the 9.0 extension libraries on net8.0. If we cared about this, we should make EF multi-target and not pull in the 9.0 extensions when on net8.0.

We shouldn't have some teams caring about this, and some teams not.

cc @ericstj @SamMonoRT

Copy link
Member

Choose a reason for hiding this comment

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

Lots of people care about this because it'll mean going from using extensions from the shared framework, to bin deploying all of extensions for .NET 8 apps.

Copy link
Member

@eerhardt eerhardt Sep 26, 2024

Choose a reason for hiding this comment

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

Should we fix EF then? Why does their version 9.0 packages support net8.0 but lift up MS.Ext references to version 9.0?

Copy link
Member

Choose a reason for hiding this comment

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

Im surprised it doesn't

Copy link
Member

Choose a reason for hiding this comment

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

Would this work? dotnet/efcore#34769

Wouldn't it cause downgrade warnings when used in conjunction with other 9.x packages?

…rencing project.

We are calling GetTargetPath, and when the referencing project references a different TargetFramework than ConfigurationSchemaGenerator, it is using the referencing projects TargetFramework, which isn't correct.
@davidfowl
Copy link
Member

Not blocking, but for futureproofing also change EnrichNpgsqlDbContext, EnrichMySqlDbContext, EnrichOracleDatabaseDbContext and EnrichCosmosDbContext to use ConfigureDbContext

We want to use these new APIs as part of these EF upgrade so that we can actually use the new methods and make sure they work in the scenarios they were designed for.

@sebastienros
Copy link
Member Author

@radical Please check the build changes. I updated the ZipTestArchive target to handle the multi-target build step which doesn't have OutDir. And also solved a concurrency issue when the two tfms are built in parallel and would write the same file. The non-default TFM is added as a prefix on the zip filename.

<ZipDirectory SourceDirectory="$(TestsArchiveSourceDir)"
DestinationFile="$([MSBuild]::NormalizePath('$(TestArchiveTestsDir)', '$(MSBuildProjectName).zip'))"
<!-- TestsArchiveSourceDir is empty (OutDir) for multi-target build step -->
<MakeDir Condition="$(TestsArchiveSourceDir) != ''" Directories="$(TestArchiveTestsDir)" />
Copy link
Member

Choose a reason for hiding this comment

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

If it is empty then how why doesn't ZipDirectory fail with empty SourceDirectory?

Copy link
Member

Choose a reason for hiding this comment

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

oh, you are skipping it in that case. I'll do a local build, and look at the binlog.

Copy link
Member Author

Choose a reason for hiding this comment

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

It failed before these changes. Now this is skipped when it is the case, when it's invoking the cross-target build.

Copy link
Member

Choose a reason for hiding this comment

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

You can use and '$(IsInnerBuild)' == 'true' in the condition for the target. This will be true when it is doing the inner build - which is one per target framework. And the target will get skipped for the outer build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, will try that instead

DestinationFile="$([MSBuild]::NormalizePath('$(TestArchiveTestsDir)', '$(MSBuildProjectName).zip'))"
<!-- TestsArchiveSourceDir is empty (OutDir) for multi-target build step -->
<MakeDir Condition="$(TestsArchiveSourceDir) != ''" Directories="$(TestArchiveTestsDir)" />
<ZipDirectory Condition="$(TestsArchiveSourceDir) != '' and '$(TargetFramework)' == '$(DefaultTargetFramework)'"
Copy link
Member

Choose a reason for hiding this comment

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

Also, the '$(TargetFramework)' == '$(DefaultTargetFramework)' means that we would get only the default zip file for net8.0, and not for net9.0.

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 could use some help for this one, when I ran helix without this it failed because it would not find a trx file matching the net90.zip file (I rephrased the error message).

Copy link
Member

Choose a reason for hiding this comment

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

Um I can work on that bit.

@radical
Copy link
Member

radical commented Oct 1, 2024

This needs the workload testing bits to be re-enabled, which would give us a 9.0 sdk with 8.0 runtime to allow running the net9.0 tests here on helix.

@radical
Copy link
Member

radical commented Oct 1, 2024

The net9.0 tests here will need a 9 sdk to run on helix. We could normally get that from the workload-tests setup, but since that is disabled in the repo right now, and waiting for on #5932 - we can't test it right now.

I hope to have the templates PR merged this week. If this needs to merged sooner, then an alternative would be to get the test infra (just msbuild should be enough) changes here.

@radical
Copy link
Member

radical commented Oct 3, 2024

net9.0 tests are running succesfully:

  • Aspire.Microsoft.EntityFrameworkCore.Cosmos.Tests-net9.0
  • Aspire.Microsoft.EntityFrameworkCore.SqlServer.Tests-net9.0
  • Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests-net9.0
  • Aspire.Oracle.EntityFrameworkCore.Tests-net9.0

@sebastienros
Copy link
Member Author

I have tested it works locally.

  • service with net9.0
  • both AddNpgsqlDbContext and EnrichNpgsqlDbContext
  • enabling/disabling metrics from code

@sebastienros
Copy link
Member Author

sebastienros commented Oct 5, 2024

Two tests failed. One seems to be a random issue with te TestHost.App playground and the fact that RabbitMQ management was assigned the same port as CatalogDb web app.

The second is related to this change, but the tests timed out. Increasing it for now since none show up as a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade EF components to 9.0
5 participants