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.

Sign up for GitHub

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

Jump to bottom

[preview7] Unpin Microsoft.Extensions.Caching.Memory and Microsoft.Extensions.Logging #25324

Merged
merged 8 commits into from
Jul 26, 2021
Merged

[preview7] Unpin Microsoft.Extensions.Caching.Memory and Microsoft.Extensions.Logging #25324

merged 8 commits into from
Jul 26, 2021

Conversation

mmitche
Copy link
Member

@mmitche mmitche commented Jul 23, 2021

Fixes #25148

@mmitche mmitche requested a review from dougbu as a code owner July 23, 2021 15:52
@mmitche mmitche requested a review from bricelam July 23, 2021 15:53
@mmitche mmitche changed the title Unpin Microsoft.Extensions.Caching.Memory and Microsoft.Extensions.Logging [preview7] Unpin Microsoft.Extensions.Caching.Memory and Microsoft.Extensions.Logging Jul 23, 2021
@mmitche
Copy link
Member Author

mmitche commented Jul 23, 2021

@bricelam @ajcvickers FYI...this is current blocking p7.

@mmitche
Copy link
Member Author

mmitche commented Jul 23, 2021

@AndriySvyryd, since @bricelam is on vacation, can you take a look?

@smitpatel
Copy link
Member

It is failing OData related breaking changes.

@dougbu
Copy link
Member

dougbu commented Jul 23, 2021

It is failing OData related breaking changes.

Looks very much like the failures in #25323. I'm hoping an ASP.NET update will react more completely to the logging API changes. But, it's possible we need OData to react first.

@AndriySvyryd
Copy link
Member

@mmitche You can disable EFCore.OData.FunctionalTests in p7 and we'll deal with this later in main.

@mmitche
Copy link
Member Author

mmitche commented Jul 23, 2021

@mmitche You can disable EFCore.OData.FunctionalTests in p7 and we'll deal with this later in main.

Can you do that? (you should have perms to push to this PR). I'm not familiar with the infra.

@dougbu
Copy link
Member

dougbu commented Jul 23, 2021

@mmitche You can disable EFCore.OData.FunctionalTests in p7 and we'll deal with this later in main.

Not a great idea because dotnet/runtime#54581 was checked in long before that repo branched for preview7. We're going to end up with incoherent runtime dependencies in the efcore packages. See also #25244 which could 'a, should 'a reacted to the runtime changes.

@dougbu
Copy link
Member

dougbu commented Jul 23, 2021

Ahhhhh…

This repo doesn't use the removed skipEnabledCheck: parameter in LoggerMessage.Define(...) calls as far as I can tell. The issue is building against ASP.NET Core bits (either from the preview6 SDK or transitively through OData because it builds against net5.0 or netcoreapp3.1) that do use that parameter.

If compatibility w/ OData isn't required, suggest building with a preview 7 SDK that brings in post-break ASP.NET Core bits.

@ericstj
Copy link
Member

ericstj commented Jul 23, 2021

Exception looks to me like it's coming from https:/dotnet/efcore/blob/main/test/EFCore.OData.FunctionalTests/EFCore.OData.FunctionalTests.csproj

According to the stack trace the offending caller is Microsoft.AspNetCore.Mvc.MvcCoreLoggerExtensions which comes from Microsoft.AspNetCore.Mvc.Core.dll.

Suspect that project is picking up the new Logging assembly, but running on older ASP.NET core. Is it possible that this test is on latest runtime shared framework but stale ASP.NETCore shared framework?

@dougbu
Copy link
Member

dougbu commented Jul 23, 2021

Suspect that project is picking up the new Logging assembly

Agreed. This PR brings in the latest and greatest preview 7 Logging package but a preview 6 SDK. That SDK pulls in older ASP.NET Core bits.

@mmitche
Copy link
Member Author

mmitche commented Jul 23, 2021

Suspect that project is picking up the new Logging assembly

Agreed. This PR brings in the latest and greatest preview 7 Logging package but a preview 6 SDK. That SDK pulls in older ASP.NET Core bits.

Update to a newer SDK in this PR?

@dougbu
Copy link
Member

dougbu commented Jul 23, 2021

Update to a newer SDK in this PR?

Plz

@mmitche
Copy link
Member Author

mmitche commented Jul 23, 2021

LE SIGH artifacts\obj\EFCore.Abstractions\Release\net6.0\EFCore.Abstractions.ImplicitNamespaceImports.cs(2,1): error CS8773: (NETCORE_ENGINEERING_TELEMETRY=Build) Feature 'global using directive' is not available in C# 9.0. Please use language version 10.0 or greater.

@AndriySvyryd
Copy link
Member

LE SIGH artifacts\obj\EFCore.Abstractions\Release\net6.0\EFCore.Abstractions.ImplicitNamespaceImports.cs(2,1): error CS8773: (NETCORE_ENGINEERING_TELEMETRY=Build) Feature 'global using directive' is not available in C# 9.0. Please use language version 10.0 or greater.

Change <LangVersion>9.0</LangVersion> to 10.0 in Directory.Build.props ?

mmitche and others added 2 commits July 23, 2021 15:02
@mmitche
Update Directory.Build.props
d8e931d
@dotnet-maestro
Update dependencies from https:/dotnet/runtime build 2021…
131d5da 
…0723.17 (dotnet#25328)

[release/6.0-preview7] Update dependencies from dotnet/runtime
@mmitche
Copy link
Member Author

mmitche commented Jul 25, 2021

@ajcvickers Can you have someone look at this? It's currently blocking shipping of p7.

@Pilchie
Copy link
Member

Pilchie commented Jul 25, 2021

@bricelam - if you're around, can you take a look at this one?

@AndriySvyryd
Copy link
Member

Seems that we need to react to nullability changes. I can do it in about 2 hours, when I get home.

@AndriySvyryd
Copy link
Member

2 tests fail to compile now. Filed https:/dotnet/runtime/issues/56285

@lukas-lansky
Copy link
Contributor

lukas-lansky commented Jul 26, 2021 •
edited
Loading

@AndriySvyryd @ajcvickers Thanks for the approvals! Can we merge this now to start the flow as soon as possible? (I don't have rights.)

@ajcvickers ajcvickers merged commit 4950317 into dotnet:release/6.0-preview7 Jul 26, 2021
@ajcvickers
Copy link
Member

Thanks all! This is now merged.

@mmitche mmitche mentioned this pull request Jul 26, 2021
Merged
@mmitche
Copy link
Member Author

mmitche commented Jul 26, 2021

Thanks @ajcvickers!

@dougbu
Copy link
Member

dougbu commented Jul 26, 2021

Just curious: How did this get in while dotnet/runtime#56285 is open❔ Checks here were green.

@mmitche
Copy link
Member Author

mmitche commented Jul 26, 2021

Just curious: How did this get in while dotnet/runtime#56285 is open❔ Checks here were green.

Looks like those couple places got commented out.

@AndriySvyryd
Copy link
Member

Looks like those couple places got commented out.

Yes, the break didn't look like a ship-stopper

@AndriySvyryd AndriySvyryd mentioned this pull request Jul 26, 2021
Merged
@bricelam bricelam mentioned this pull request Jul 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajcvickers ajcvickers ajcvickers approved these changes

@AndriySvyryd AndriySvyryd AndriySvyryd approved these changes

@dougbu dougbu Awaiting requested review from dougbu

@bricelam bricelam Awaiting requested review from bricelam

@maryamariyan maryamariyan Awaiting requested review from maryamariyan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mmitche @smitpatel @dougbu @AndriySvyryd @ericstj @Pilchie @lukas-lansky @ajcvickers