- Notifications
You must be signed in to change notification settings - Fork311
Merge | LocalDBAPI#3163
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merge | LocalDBAPI#3163
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…ly loaded on Windows
…ot* want to try to decompose stuff in TdsParser to remove LocalDBAPI references on non-windows systems.
| @@ -8,6 +8,9 @@ namespace Microsoft.Data | |||
| { | |||
| internal static partial class LocalDBAPI | |||
| { | |||
| internal static string GetLocalDbInstanceNameFromServerName(string serverName) => | |||
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 think this is a change in behavior, but I agree with it. Right now, we try to get the local db instance name for unix, and may return a non-null value, but later steps can fail with a PlatformNotSupportedException.
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.
On second look, yes I think you're right that it's a behavior change. I think the one thing I'm not sure about is if this will raise a platformnotsupported exception or if it will just fail to connect. I'm going to fire this up on linux to verify.
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 confirm, on Linux, the behavior is the same - we get the "LocalDB is not supported on this platform" exception when trying to connect.
codecovbot commentedFeb 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3163 +/- ##==========================================- Coverage 72.96% 72.84% -0.12%========================================== Files 283 282 -1 Lines 58997 59175 +178 ==========================================+ Hits 43048 43107 +59- Misses 15949 16068 +119
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. |
e25c24a intomainUh oh!
There was an error while loading.Please reload this page.
This is a replacement of#3047 that will give us the complete CI
Description: This is one more PR that targets merging another file between the netfx project and the netcore project. This time, it's the LocalDBAPI. This one was a bit complicated because the files were split differently between netfx and netcore. The way I approached it was to merge the operative code into the LocalDBAPI.Windows file and merge the inoperative code to the LocalDBAPI.Unix file. The Windows file will be pulled in on both netfx and netcore projects, with it being conditionally included on netcore if the build is for Windows. The Unix file will be pulled in on netcore only, being conditionally included if the build is for Unix. Otherwise, this is a fairly standard merge with netfx having extra stuff that is
#if'd in.please note: cleanup to remove monitor sections, etc, is coming in a later PR.
Testing: There miiiiight be some issues with unix build, I might not have fully vetted it on unix.