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

[release/2.1] Update to SQLitePCLRaw 1.1.15 #29433

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

bricelam
Copy link
Contributor

@bricelam bricelam commented Oct 26, 2022

Part of #29429

@bricelam bricelam requested a review from a team October 26, 2022 18:40
@bricelam
Copy link
Contributor Author

bricelam commented Oct 26, 2022

Need to mirror SQLitePCLRaw.bundle_green 2.1.2

@wtgodbe
Copy link
Member

wtgodbe commented Oct 26, 2022

Need to mirror SQLitePCLRaw.bundle_green 2.1.2

@dotnet/dnceng can somebody help with this?

@wtgodbe
Copy link
Member

wtgodbe commented Oct 26, 2022

This will also require a submodule update in aspnetcore's 2.1 branch, and (I think) a change to patchconfig.props - @dougbu will that be sufficient to get these EF packages building?

@dougbu
Copy link
Member

dougbu commented Oct 26, 2022

Need to mirror SQLitePCLRaw.bundle_green 2.1.2

May need to ask in First Responders channel.

@dougbu
Copy link
Member

dougbu commented Oct 26, 2022

will that be sufficient to get these EF packages building?

No, will need to change build/submodules.props in dotnet/aspnetcore.

Also need to update versions.props and probably build/dependencies.props in this PR to align w/ next version. Or we should do a branding PR then merge this, both before updating the submodule in dotnet/aspnetcore and changing its build to build EF.

@dougbu
Copy link
Member

dougbu commented Oct 26, 2022

See #15369 for example

@AlitzelMendez
Copy link
Member

Need to mirror SQLitePCLRaw.bundle_green 2.1.2

@dotnet/dnceng can somebody help with this?

done :)

@wtgodbe
Copy link
Member

wtgodbe commented Oct 26, 2022

/azp run

@wtgodbe
Copy link
Member

wtgodbe commented Oct 26, 2022

#29437

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bricelam

This comment was marked as outdated.

@bricelam
Copy link
Contributor Author

bricelam commented Nov 2, 2022

Looks like it's failing because it's using version 2.1.526 of the SDK which doesn't support NuGet's buildTransitive directory.

@bricelam
Copy link
Contributor Author

bricelam commented Nov 2, 2022

In other words, this will break compatibility with VS2017

@dougbu
Copy link
Member

dougbu commented Nov 4, 2022

@bricelam is there a workaround for the problem you described❔ Would be good to get this in this month.

@ajcvickers
Copy link
Contributor

@bricelam Is there anything blocking this now that we have the go-ahead from Tactics on using the newer SDK?

@bricelam
Copy link
Contributor Author

Hmm... Trying to manually verify the packages, I get Library e_sqlite3 not found. If I specify a RuntimeIdentifier, it works.

@ericsink I don't think the build script is working as expected. Instead of a runtimes directory with all three Windows dlls under it, I see one (arbitrary?) e_sqlite3.dll copied to the output directory. In other words, I don't think the Link metadata is working as expected...

@ericsink
Copy link

Coming up to speed here on the context... you're trying to use recent builds of SQLitePCLRaw (2.1.2) with EFCore 2.1? Which is several years old? It never occurred to me that my current builds would need to work with configurations that old, although I can't defend any reason why not.

Anyway, that targets file for .NET Framework scenarios is a never ending source of problems. The last time somebody convinced me to make significant changes in that area was ericsink/SQLitePCL.raw#389 , the goal of which was to support RuntimeIdentifier, but since those changes, I've seen various problems happening when RuntimeIdentifier is NOT present.

It's also possible that I broke something with an associated change to the code in the dynamic provider.

I'll need to dig into this to figure things out. If by chance you want to suggest a fix, feel free.

@ericsink
Copy link

So I constructed a quick test, and if I tweak the Condition on that ItemGroup to remove '$(RuntimeIdentifier)' == '' AND, then I once again get a runtimes directory with all 3 Windows rids under it.

So it would seem that RuntimeIdentifier is set to something even when it is not set, and depending on it being '' isn't going to work. That change was made in commit b4d03d52cbb485fe1b6fad9586687c21e1371913 in ericsink/SQLitePCL.raw. I can remove that Condition, but then I might re-break the problem reported in ericsink/SQLitePCL.raw#389

I also do still get that "one (arbitrary?) e_sqlite3.dll copied to the output directory". I'm not sure what's putting it there. Perhaps this is related to whatever RuntimeIdentifier default value is showing up, but it's interesting to note that whatever e_sqlite3.dll is being placed there, it doesn't seem to actually work at runtime.

@ericsink
Copy link

ericsink commented Nov 23, 2022

Running a build with -v diag includes the following:

Property reassignment: $(RuntimeIdentifier)="win7-x86" (previous value: "") at C:\Program Files\dotnet\sdk\7.0.100\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.RuntimeIdentifierInference.targets (61,5)

Which seems consistent with the observation above that it is (no longer | not) safe to assume that RuntimeIdentifier is an empty string if the user did not set it.

I find the available documentation to be lacking when it comes to arcane details of the behavior of msbuild properties like this. Guidance welcome.

@ericsink
Copy link

@bricelam Any suggestions for what I should do here?

My goal would be to find a way to fix that targets file such that:

  1. When someone explicitly sets RuntimeIdentifier, it works the way it does now, with the proper version of e_sqlite3.dll appearing in the output directory.
  2. And if RuntimeIdentifier is not explicitly set, we get the "runtimes directory with all 3 Windows rids under it", but NOT the "one (arbitrary?) e_sqlite3.dll copied to the output directory"

But I'm not sure (yet) how to do that.

It would seem that AFTER @AArnott suggested the current approach (the '$(RuntimeIdentifier)' == '' condition, back in ericsink/SQLitePCL.raw#389 -- Hi Andrew), the .NET SDK may have changed the way RuntimeIdentifier is used, giving it a default value.

@AArnott
Copy link

AArnott commented Nov 30, 2022

I doubt RuntimeIdentifier has a default value set. But it gets set in a variety of ways including dotnet publish -r win-x86, or maybe even based on the Platform property.
What .NET SDK is being used at this point, and what is the TargetFramework? 2.1 is long out of support.

@ajcvickers
Copy link
Contributor

@AArnott 2.1 is still supported for ASP.NET Core applications targeting .NET Framework. This is for an MSRC relating to that.

@ericsink
Copy link

I used the term "default value" rather imprecisely, intending to mean "any value that property ever gets that was not assigned by the developer editing the csproj file". :-)

@AArnott
Copy link

AArnott commented Nov 30, 2022

@ajcvickers Makes sense. And what version of the .NET SDK (or Visual Studio) are you using?

@bricelam
Copy link
Contributor Author

@AArnott I was using the 7.0 SDK to manually verify the packages. (With a project targeting .NET Framework)

@dougbu
Copy link
Member

dougbu commented Jan 11, 2023

@ajcvickers @bricelam, is this a tell-mode change and should we make sure we build dotnet/aspnetcore release/2.1 (which includes efcore as a submodule) for March❔

/cc @wtgodbe

@bricelam
Copy link
Contributor Author

@dougbu We ran into issues with this change and have taken a new approach. As soon as the new build passes, yes, let's get it into the March 2.1.x patch. And yes, ASP.NET Identity should update its dependencies on these EF packages.

@bricelam
Copy link
Contributor Author

We may need to mirror the 1.1.15 versions of these packages when they're available: (If we do that on 2.1)

  • SQLitePCLRaw.bundle_green
  • SQLitePCLRaw.lib.e_sqlite3.v110_xp

@dougbu
Copy link
Member

dougbu commented Jan 11, 2023

And yes, ASP.NET Identity should update its dependencies on these EF packages.

IIRC, that happens automatically when we update dotnet/aspnetcore to produce dotnet/efcore packages. It's all hinged on a one-line change in aspnetcore.

@dougbu
Copy link
Member

dougbu commented Jan 11, 2023

We may need to mirror the 1.1.15 versions of these packages when they're available: (If we do that on 2.1)

release/2.1 branches use dotnet-public too. Unless the feed already contains what you need, head over to First Responders and request them.

@@ -684,6 +685,12 @@ public override void Select_datetime_millisecond_component()
}
#endif

[ConditionalFact(Skip = "Issue#21541")]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, this actually passes on .NET Framework and isn't regressed there. Just skipping everywhere since that's what we did for a while in downstream branches.

@bricelam bricelam changed the title [release/2.1] Update to SQLitePCLRaw 2.1.2 [release/2.1] Update to SQLitePCLRaw 1.1.15 Jan 11, 2023
@bricelam
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bricelam bricelam added this to the 2.1.x milestone Jan 12, 2023
@bricelam bricelam merged commit 74055e1 into dotnet:release/2.1 Jan 12, 2023
4 checks passed
@bricelam bricelam deleted the printf21 branch January 12, 2023 18:09
@ajcvickers ajcvickers removed this from the 2.1.x milestone Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants