First round of converting System.Data.OleDb to COMWrappers by kant2002 · Pull Request #54822 · dotnet/runtime

kant2002 · 2021-06-28T12:33:22Z

This change just remove warning from ILLink and do not make attempt
to convert other COM object creation to use ComWrappers

Also I have to add target net5.0-windows where ComWrappers would be
used. Same concerns regarding testing as in
#54636

Two calls inside OleDbDataAdapter call GetErrorInfo but do not do anything with results. That's probably mistake from previous refactoring. These parts does not touched and left as is. Need advice on that specific parts how to proceed.

This change just remove warning from ILLink and do not make attempt to convert other COM object creation to use ComWrappers Also I have to add target net5.0-windows where ComWrappers would be used. Same concerns regarding testing as in dotnet#54636 Two calls inside OleDbDataAdapter call GetErrorInfo but do not do anything with results. That's probably mistake from previous refactoring. These parts does not touched and left as is. Need advice on that specific parts how to proceed.

ghost · 2021-06-28T12:33:28Z

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

Issue Details

This change just remove warning from ILLink and do not make attempt
to convert other COM object creation to use ComWrappers

Also I have to add target net5.0-windows where ComWrappers would be
used. Same concerns regarding testing as in
#54636

Two calls inside OleDbDataAdapter call GetErrorInfo but do not do anything with results. That's probably mistake from previous refactoring. These parts does not touched and left as is. Need advice on that specific parts how to proceed.

Author:	kant2002
Assignees:	-
Labels:	`area-System.Data`
Milestone:	-

jkotas · 2021-06-28T12:59:56Z

cc @AaronRobinsonMSFT @eerhardt

eerhardt · 2021-06-28T15:55:46Z

+ [DllImport(Interop.Libraries.OleAut32, CharSet = CharSet.Unicode, PreserveSig = true)]
+ internal static extern System.Data.OleDb.OleDbHResult GetErrorInfo(
+ [In] int dwReserved,
+ [Out] out System.IntPtr ppIErrorInfo);


Don't use out in P/Invokes, but instead use pointers. This matches the native signatures as close as possible, and allows for the native marshalling to do less work. In this case, the native marshalling needs to pin the managed reference out IntPtr ppIErrorInfo. But it doesn't if the signature is a pointer.
Suggested change
[Out]outSystem.IntPtr ppIErrorInfo);
[Out]System.IntPtr* ppIErrorInfo);

Please also drop the [Out] attribute.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

…oledb

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

eerhardt · 2021-06-30T19:58:38Z

+ <Compile Include="OleDbConnection.COMWrappers.cs" />
+ <Compile Include="OleDbException.COMWrappers.cs" />
+ </ItemGroup>
+ <ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETStandard'">


Is this right? Don't you want the ! of the above condition on line 102?
Or maybe even just not on TargetFramework net5.0 compatible, but still on Windows:
<ItemGroupCondition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net5.0')) and '$(TargetsWindows)' == 'true'">

This would include net461-windows which is wrong as that's facade assembly. The condition is probably missing a '$(TargetsWindows)' == 'true'.

I guess I would use
<ItemGroupCondition="!$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net5.0')) and'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' == 'true'">
to be symmetrical with the first ItemGroup condition above.
runtime/src/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj
Line 22 in f6cc321
<ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' == 'true'">

@ViktorHofer and @eerhardt I made changes, but I'm not sure if this is proper way. All I can say, not it does not fails during build.

Looks pretty good. My only comments would be to make the ItemGroup conditions in the same order, to help with readability:
<ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' == 'true'"> ... <ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', $(NetCoreAppCurrent))) and '$(TargetsWindows)' == 'true'"> ... <ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and !$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', $(NetCoreAppCurrent))) and '$(TargetsWindows)' == 'true'">
would become
<ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' == 'true'"> ... <ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' == 'true' and $([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', $(NetCoreAppCurrent)))"> ... <ItemGroupCondition="'$(IsPartialFacadeAssembly)' != 'true' and '$(TargetsWindows)' == 'true' and !$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', $(NetCoreAppCurrent)))">
And then change the IsTargetFrameworkCompatible to use net5.0, since COMWrappers were introduced in net5.0. This helps with maintainability in the long term, when we update NetCoreAppCurrent to be net7.0 in the future.

eerhardt · 2021-06-30T20:01:33Z

+{
+ internal static partial class ODB
+ {
+ internal static OleDbHResult GetErrorDescription(UnsafeNativeMethods.IErrorInfo errorInfo, OleDbHResult hresult, out string message)


The code here between COMWrappers and NoCOMWrappers looks duplicated. Can it be folded back together?

eerhardt · 2021-06-30T20:04:49Z

+
+ public sealed partial class OleDbConnection
+ {
+ internal static Exception? ProcessResults(OleDbHResult hresult, OleDbConnection? connection, object? src)


Can we share the bulk of this code, and only pull out the differences into COMWrappers vs. NoCOMWrappers files? i.e. create new private methods on the partial classes.

eerhardt · 2021-07-12T14:52:40Z

 namespace System.Data.OleDb
 {
- public sealed class OleDbException : System.Data.Common.DbException
+ public sealed partial class OleDbException : System.Data.Common.DbException


Does this need to be partial? I don't see the other side of the partial class.

eerhardt · 2021-07-12T14:54:12Z

 string? source = null;
 OleDbHResult hr = 0;
-
- if (null != errorInfo)


Why don't we need a null check here?

There discussion about that #54822 (comment)

eerhardt · 2021-07-12T14:55:06Z

+
+namespace System.Data.OleDb
+{
+ using SysTx = Transactions;


Is this using used?

kant2002 · 2021-09-17T13:17:42Z

@eerhardt can you take a look? Also do I need to pursue creation of fake OleDbProvider to test for error condition which you notice?

eerhardt

This is looking really good. I just had a few final comments. After these are addressed, I won't have anymore concerns. Would also be good to get @AaronRobinsonMSFT to sign off.

eerhardt · 2021-09-20T16:12:58Z

+ return e;
+ }
+
+ internal void OnInfoMessage(UnsafeNativeMethods.IErrorInfo errorInfo, OleDbHResult errorCode)


I don't see a difference in the code of OnInfoMessage between the COMWrappers file and the NoCOMWrappers file. Why do we need them split?

eerhardt · 2021-09-20T16:14:11Z

+ OleDbHResult hr = UnsafeNativeMethods.GetErrorInfo(0, &pErrorInfo); // 0 - IErrorInfo exists, 1 - no IErrorInfo
+ if ((OleDbHResult.S_OK == hr) && (IntPtr.Zero != pErrorInfo))
+ {
+ UnsafeNativeMethods.IErrorInfo errorInfo = (UnsafeNativeMethods.IErrorInfo)OleDbComWrappers.Instance


Can we just split this line out into separate partial methods between the two? That way we don't need to duplicate this whole method?

…s.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

AaronRobinsonMSFT · 2021-09-20T18:16:56Z

+ [In] int dwReserved,
+ [Out, MarshalAs(UnmanagedType.Interface)] out UnsafeNativeMethods.IErrorInfo? ppIErrorInfo);


Suggested change
[In]int dwReserved,
[Out,MarshalAs(UnmanagedType.Interface)]out UnsafeNativeMethods.IErrorInfo? ppIErrorInfo);
intdwReserved,
[MarshalAs(UnmanagedType.Interface)]out UnsafeNativeMethods.IErrorInfo? ppIErrorInfo);

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

eerhardt

This LGTM. Thanks for the contribution here, @kant2002! I like how this change cleaned up, and really didn't need that much changes in the end.

kant2002 · 2021-09-21T15:23:28Z

Yeah, @eerhardt your proposal to consolidate files was excellent. I remove 4 not needed files and that was great.

eerhardt · 2021-09-21T16:13:32Z

@lewing @radical - is this a known issue? I can't see how this change could have caused this failure in the browser wasm staging leg.

src\mono\mono.proj(492,5): error MSB3073: (NETCORE_ENGINEERING_TELEMETRY=Build) The command "call "D:\workspace\_work\1\s\eng\native\init-vs-env.cmd" && call "D:\workspace\_work\1\s\src\mono\wasm\emsdk\emsdk_env.bat" && emcmake cmake -DENABLE_WERROR=1 -G Ninja -DCMAKE_INSTALL_PREFIX="D:\workspace\_work\1\s\artifacts\obj\mono\Browser.wasm.Release\out" -DCMAKE_INSTALL_LIBDIR=lib -DCMAKE_BUILD_TYPE=Release -DGC_SUSPEND=preemptive -DMONO_LIB_NAME=monosgen-2.0 -DMONO_SHARED_LIB_NAME=monosgen-2.0 -DENABLE_MINIMAL=jit,sgen_major_marksweep_conc,sgen_split_nursery,sgen_gc_bridge,sgen_toggleref,sgen_debug_helpers,sgen_binary_protocol,logging,shared_perfcounters,interpreter,threads,qcalls,debugger_agent,log_dest,assert_messages -DENABLE_INTERP_LIB=1 -DDISABLE_ICALL_TABLES=1 -DENABLE_ICALL_EXPORT=1 -DENABLE_LAZY_GC_THREAD_CREATION=1 -DENABLE_LLVM_RUNTIME=1 -DSTATIC_COMPONENTS=1 -DMONO_COMPONENTS_RID=Browser-wasm -DCMAKE_C_FLAGS=" -fexceptions \"-ID:\workspace\_work\1\s\.packages\microsoft.netcore.runtime.icu.transport\7.0.0-alpha.1.21463.1\runtimes\browser-wasm\native\include\"" -DCMAKE_CXX_FLAGS=" -fexceptions" "D:\workspace\_work\1\s\src\mono"" exited with code 9009

radical · 2021-09-21T16:20:48Z

This failed because emsdk could not be activated:

 HEAD is now at f44b841 2.0.23 (#828)
Error: Downloading URL 'https://storage.googleapis.com/webassembly/emscripten-releases-builds/deps/node-v14.15.5-win-x64.zip': <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1123)>
Warning: Possibly SSL/TLS issue. Update or install Python SSL root certificates (2048-bit or greater) supplied in Python folder or https://pypi.org/project/certifi/ and try again.
Installation failed!

@radekdoulik the update machine certs step passed. Is this a network issue?

eerhardt · 2021-09-21T19:01:07Z

It must have been a network issue. I ran it for the 3rd time, and it looks like it is further than it was before.

I'll merge this once that leg goes green.

ghost added the area-System.Data label Jun 28, 2021

eerhardt reviewed Jun 28, 2021
View reviewed changes

Comment threadsrc/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj Outdated

eerhardt reviewed Jun 28, 2021
View reviewed changes

AaronRobinsonMSFT reviewed Jun 28, 2021
View reviewed changes

kant2002and others added 7 commits June 29, 2021 11:22

Update src/libraries/System.Data.OleDb/src/OleDbComWrappers.cs
68ba34e
Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Update src/libraries/System.Data.OleDb/src/OleDbComWrappers.cs
2e0441c
Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Apply PR recommendations
2a753ac

Make closer to original
5b7c9dc

One more change
749fee8

Apply PR feedback
505e6f2

Merge remote-tracking branch 'upstream/main' into kant/remove-il2050-…
51b3456
…oledb

ViktorHofer reviewed Jun 30, 2021
View reviewed changes

Comment threadsrc/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj

ViktorHofer reviewed Jun 30, 2021
View reviewed changes

Comment threadsrc/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj

kant2002and others added 4 commits June 30, 2021 19:45

Update src/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj
db65b0b
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

Update src/libraries/System.Data.OleDb/src/System.Data.OleDb.csproj
7e9e956
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

Fix compilation errors
2cf1788

Fix errors
b928f03

eerhardt reviewed Jun 30, 2021
View reviewed changes

kant2002 added 4 commits July 1, 2021 09:09

Consolidate code
7401706

Fix compilation warnings
5750c8e

Fix compilation error
c301fb6

Fix compilation error
5d11dea

eerhardt reviewed Jul 12, 2021
View reviewed changes

eerhardt reviewed Sep 20, 2021
View reviewed changes

kant2002and others added 9 commits September 20, 2021 22:19

Update src/libraries/System.Data.OleDb/src/OleDbConnection.COMWrapper…
0e1177f
…s.cs Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Update src/libraries/System.Data.OleDb/src/OleDbComWrappers.cs
9f3a972
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Update src/libraries/System.Data.OleDb/src/OleDbComWrappers.cs
f41457f
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Update src/libraries/System.Data.OleDb/src/DbPropSet.COMWrappers.cs
dd9e48a
Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Restore changes in OnInfoMessage
adf11e5

Consolidate different part of code
27a857b

Remove junk (probably from merge)
eca9732

Found that last commit was a mess
76b3032

Remove no longer needed annotations
75567c2

AaronRobinsonMSFT reviewed Sep 20, 2021
View reviewed changes

kant2002and others added 7 commits September 21, 2021 11:17

Apply suggestions from code review
3c66ce7
Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Update src/libraries/System.Data.OleDb/src/OleDbComWrappers.cs
af6d01e
Co-authored-by: Aaron Robinson <arobins@microsoft.com>

Free BSTR
ed5e796

Remove reference to no longer used files in this PR
c897950

Add data
a331a8e

Fix compilation errors
e6efd5c

Last blow to compiler
93b8fd3

eerhardt approved these changes Sep 21, 2021
View reviewed changes

AaronRobinsonMSFT approved these changes Sep 21, 2021
View reviewed changes

eerhardt merged commit 3de6468 into dotnet:mainSep 21, 2021

kant2002 deleted the kant/remove-il2050-oledb branch September 22, 2021 10:32

runfoappBot mentioned this pull request Sep 24, 2021
emsdk failure: emcmake not found #59556
Closed

jkotas mentioned this pull request Oct 8, 2021
Add proposed plan for the COM source generator #60143
Merged

ghost locked as resolved and limited conversation to collaborators Nov 3, 2021

	[Out]outSystem.IntPtr ppIErrorInfo);
	[Out]System.IntPtr* ppIErrorInfo);

		[In] int dwReserved,
		[Out, MarshalAs(UnmanagedType.Interface)] out UnsafeNativeMethods.IErrorInfo? ppIErrorInfo);

-[In]int dwReserved,
-[Out,MarshalAs(UnmanagedType.Interface)]out UnsafeNativeMethods.IErrorInfo? ppIErrorInfo);
+intdwReserved,
+[MarshalAs(UnmanagedType.Interface)]out UnsafeNativeMethods.IErrorInfo? ppIErrorInfo);

Uh oh!

Conversation

kant2002 commented Jun 28, 2021

Uh oh!

ghost commented Jun 28, 2021

Uh oh!

jkotas commented Jun 28, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardtJun 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHoferJun 30, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kant2002 commented Sep 17, 2021

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

kant2002 commented Sep 21, 2021

Uh oh!

eerhardt commented Sep 21, 2021

Uh oh!

radical commented Sep 21, 2021

Uh oh!

eerhardt commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

eerhardtJun 30, 2021•
edited
Loading

ViktorHoferJun 30, 2021•
edited
Loading