Support embedded interop types #13813

pinzart90 · 2023-03-10T17:34:38Z

Changes:
Interop types equivalence is checked when importing types from assemblies.

Embedded interop types will appear as distinct when you check type.Equals(other).
Dotnet provides a new API to check equivalence = IsEquivalentTo
https://learn.microsoft.com/en-us/dotnet/framework/interop/type-equivalence-and-embedded-interop-types

Potential issues:
Even though the main part of dynamo that deals with import of types is now updated, there is a risk that other parts of dynamo still have code that compare type.Equals(other). This sort of code will not work as expected for embedded interop types

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

pinzart90 · 2023-03-10T17:39:08Z

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

                            throw new InvalidOperationException(string.Format("Can't import {0}, {1} is already imported as {2}, namespace support needed.", type.FullName, type.Name, otherType.FullName));
+                        }

                        mTypes.Add(type, mtype);


if I add the other type in the type map (even though they are equivalent) Dynamo engine will complain and fail to compile the graph
If I skip the other type (and only keep the first equivalent type) then we get into stackoverflows (types are expected to get stored in ther type map)

so - does this mean that we cannot even the more general case where 2 packages depend on the same binary?
If I skip the other type (and only keep the first equivalent type) then we get into stackoverflows (types are expected to get stored in ther type map)

It seems it must be possible - consider the Revit API, many packages reference it, but none distribute it.

OR are you trying to solve a different problem with his PR?

The problem is visible only when you embed types in the managed assembly.
"Normal" references will work just fine (like Revit API references)

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

mjkkirschner · 2023-03-13T20:04:21Z

src/Engine/ProtoCore/FFI/CLRDLLModule.cs

            {
                lock (mTypes)
                {
-                    if (!mTypes.TryGetValue(type, out mtype))
+                    mtype = new CLRModuleType(type);
+                    //Now check that a type with same name is not imported.


is this comment entirely accurate anymore?

mjkkirschner · 2023-03-13T20:05:44Z

test/Engine/FFITarget/FFITarget.csproj

@@ -22,9 +22,16 @@
        <Reference Include="Microsoft.CSharp" />
        <Reference Include="System.Data" />
        <Reference Include="System.Xml" />
+        <Reference Include="$(PkgMicrosoft_Office_Interop_Excel)\lib\net20\Microsoft.Office.Interop.Excel.dll">
+            <EmbedInteropTypes>True</EmbedInteropTypes>
+            <WrapperTool>tlbimp</WrapperTool>


what is this?

mjkkirschner · 2023-03-13T20:06:04Z

test/Engine/FFITarget/FFITarget.csproj

@@ -22,9 +22,16 @@
        <Reference Include="Microsoft.CSharp" />
        <Reference Include="System.Data" />
        <Reference Include="System.Xml" />
+        <Reference Include="$(PkgMicrosoft_Office_Interop_Excel)\lib\net20\Microsoft.Office.Interop.Excel.dll">


should this reference be wrapped in a condition for now? (I mean the one below)

will this make it to the bin folder? I think we purposefully removed it at some point.

totally forgot about test dlls. Will work on removing them

mjkkirschner · 2023-03-13T20:09:06Z

test/test_support_projects/EmbeddedInterop.csproj

+        <PackageReference Include="Microsoft.Office.Interop.Excel" Version="15.0.4795.1000" GeneratePathProperty="true">
+            <ExcludeAssets>all</ExcludeAssets>
+        </PackageReference>
+        <Reference Include="$(PkgMicrosoft_Office_Interop_Excel)\lib\net20\Microsoft.Office.Interop.Excel.dll">


so is there anything in this binary except for the office types?

no, just office types and a wrapping class

mjkkirschner

provided the tests pass, I think it seems great except my question about the test binaries making their way to the bin folder.

I also am still not sure if the normal reference case works fine - but let's table that until we get a report from customers - maybe it was fixed a while ago and I missed it.

pinzart90 · 2023-03-13T23:58:42Z

provided the tests pass, I think it seems great except my question about the test binaries making their way to the bin folder.

I also am still not sure if the normal reference case works fine - but let's table that until we get a report from customers - maybe it was fixed a while ago and I missed it.

maybe we can discuss this tomorrow, maybe I am missing something

This reverts commit cab284e.

* update * Update CLRDLLModule.cs * update * Create EmbeddedInterop.dll * update * Update ProtoTest.csproj * update * update * Update FFITarget.csproj * update * Update ClassFunctionality.cs * update * Update DynamoCore.csproj * Revert "Update DynamoCore.csproj" This reverts commit cab284e. --------- Co-authored-by: pinzart <[email protected]>

update

e111360

pinzart90 commented Mar 10, 2023

View reviewed changes

src/Engine/ProtoCore/FFI/CLRDLLModule.cs Outdated Show resolved Hide resolved

pinzart90 commented Mar 10, 2023

View reviewed changes

src/Engine/ProtoCore/FFI/CLRDLLModule.cs Outdated Show resolved Hide resolved

pinzart90 commented Mar 10, 2023

View reviewed changes

src/Engine/ProtoCore/FFI/CLRDLLModule.cs Outdated Show resolved Hide resolved

pinzart added 3 commits March 13, 2023 10:18

Update CLRDLLModule.cs

1ce7ad6

update

18dffd7

Create EmbeddedInterop.dll

db0dcf8

pinzart90 changed the title ~~[TEST-Do not merge] update~~ Support embedded interop types Mar 13, 2023

pinzart added 7 commits March 13, 2023 12:24

update

1ab6a48

Update ProtoTest.csproj

e5a1107

update

18dd45c

update

27fc856

Update FFITarget.csproj

534190c

update

2c778e6

Update ClassFunctionality.cs

4b46443

pinzart90 requested a review from mjkkirschner March 13, 2023 19:34

pinzart90 marked this pull request as ready for review March 13, 2023 19:34

pinzart90 requested a review from sm6srw March 13, 2023 19:39

mjkkirschner reviewed Mar 13, 2023

View reviewed changes

mjkkirschner approved these changes Mar 13, 2023

View reviewed changes

update

585aeef

pinzart90 requested a review from mjkkirschner March 14, 2023 19:12

pinzart added 2 commits March 14, 2023 16:45

Update DynamoCore.csproj

cab284e

Revert "Update DynamoCore.csproj"

0f5c01c

This reverts commit cab284e.

pinzart90 merged commit fdaf68f into master Mar 15, 2023

pinzart90 deleted the test_embeded_iterop branch March 15, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support embedded interop types #13813

Support embedded interop types #13813

pinzart90 commented Mar 10, 2023 •

edited

Loading

pinzart90 Mar 10, 2023

mjkkirschner Mar 11, 2023

pinzart90 Mar 13, 2023

mjkkirschner Mar 13, 2023

mjkkirschner Mar 13, 2023

mjkkirschner Mar 13, 2023 •

edited

Loading

mjkkirschner Mar 13, 2023

pinzart90 Mar 13, 2023

pinzart90 Mar 14, 2023

mjkkirschner Mar 13, 2023

pinzart90 Mar 13, 2023

mjkkirschner left a comment

pinzart90 commented Mar 13, 2023

Support embedded interop types #13813

Support embedded interop types #13813

Conversation

pinzart90 commented Mar 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner left a comment

Choose a reason for hiding this comment

pinzart90 commented Mar 13, 2023

pinzart90 commented Mar 10, 2023 •

edited

Loading

mjkkirschner Mar 13, 2023 •

edited

Loading