-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Support embedding of the type library into the assembly. #278
Conversation
…ow for unloading of the types on demand. The class will automatically unload types on dispose. However, the unloading is still cooperative, so it is necessary to handle the garbage collection from outside. Reference: https://learn.microsoft.com/en-us/dotnet/standard/assembly/unloadability
…ate a file's resources and therefore embed a generated type library into the source assembly, which allows distributing the assembly without having to distribute a type library alongside it. Note that embedding a type library will not make sense for an AnyCPU assembly, particularly for net48 assemblies where the same assembly can be loaded by either 32-bit or 64-bit processes and shipping two separate type libraries are preferable. However for net48 x86/x64 or .net5 assemblies where comhost.dll are already tied to specific architecture, it can be helpful in simplifying the distribution. This can be invoked by calling `--embedtlb` argument included to the tool.
…o support embedding of the type library as part of the build process. The new property `DsComTypeLibraryEmbedAfterBuild` can be used in a .csproj to indicate that embedding of the type library is desired. In the native build, the embedding is a separate step that follows the exporting. In the tool-based build, it should be passed as an argument into the CLI tool.
… library works and is effective. Use the OleAut32.LoadTypeLib to test and assert that the type library was correctly embedded in the assembly.
@marklechtermann or @carstencodes FYI - I put this in draft because there is one step I am having difficulty with validating and that is passing the property
I temporarily omitted the |
There is still an error in the build task acceptance-test-msbuild ( You can check this locally by executing the script |
src/dscom.build/dSPACE.Runtime.InteropServices.BuildTasks.Tools.targets
Outdated
Show resolved
Hide resolved
One general issue I observed was #104 which leads to the issue that TLBs were missing or empty, if dscom was used inside MsBuild environments. @marklechtermann decided, that using the executable was a good workaround. I was also about to delete that part once I got time. My current suggestion is not to use dscom related functions hosted in MsBuild but to use the executables. Next thing I would suggest is going more straight forward: Why add the ability to add an arbitrary TLB to the Assembly with the TlbExport Task. If there is the necessity to add a special id, I can provide an msbuild target to find the next free id. Is this really necessary, as this already works for existing TLBs in MsBuild? You wrote, that it does not work that tlbembed parameter is not set. Can you provide me a .binlog file (using dotnet msbuild -bl) or a verbose / detailed / diagnostic log file that would help us investigating us more? |
FYI the documentation seems to imply that both are in use, that the client tool would be only used if 32-bit target is selected. On the review, and if I understand you correctly, the client tool is currently always used and the build task is never used unless I pass I think the documentation should be updated to indicate that client tool is always used. In particular this passage: Lines 241 to 245 in 869f481
I'm not sure I understand your concern so I'll try to answer: If you are referring to the
If you are referring to the setting the ID to an arbitrary number when adding the resource, the PR will not allow this; it assumes that the index is always
Did I address your concerns?
Thanks for the suggestion. I will post once I've done more local testing and if I'm stuck. |
I added a new issue to remove the BuildTask from the package and the documentation. As the library writer still may have issues, it's better to use the CLI and hence use a verb, if you want to add any TLBs to the assembly.
Ok, I see. I think it's up to @SOsterbrink, @matthiasnissen and @marklechtermann to check, if .NET 5 - as it is EOL - should still be targeted. I have no opinion on that.
Yeah, ok, I think I could not make my point clear. If we use And please don't get me wrong: I appreciate your work. But I'm taking a closer look at the workflow right now.
Ok, I see. If the issue comes around, we can still add a parameter for that. /cc @marklechtermann So, in conclusion:
Or did I get anything wrong here? |
First of all, thank you once again for your commitment! @bclothier @carstencodes is more familiar with the BuildTask. I trust your assessment ;-) Regarding .NET 5: Regarding 32Bit: From my point of view, it would be okay if the "tlbembed" feature were only available in the case of 64Bit. Regarding build task CLI vs library:
I agree
I agree I would suggest that the dscom gets a new CLI command.
Since the DLL can be changed, I don't know if assembly signing is a problem. The build Task should use the new dscom cli command to embed the TLB into the assembly. |
Replying to both @carstencodes and @marklechtermann :
Agreed. It's mainly NET48 that might be more interesting.
I see what you mean now. I assumed that the feature would not be available if a project wasn't targeting .NET6+. Even if my assumption was incorrect, we still have the fact of building twice after a Clean or from a new setup. That is not conductive for automated build.
Good question! It is possible to expose this as a new command verb which is why I made When using the dscom as a CLI, I can see how it would be useful if you wanted to do arbitrary embedding of various TLBs, but I do not see how that would be done as a part of the build task operating on the assembly being built? Even if we convert to a TaskItems, it would imply that the type library is coming from somewhere else outside the assembly being built in which case it becomes equivalent to the existing I can build out a new command to expose
In my scenario, I intend to cross compile and generate a both 32-bit and 64-bit DLL to be then shipped and therefore expect the ability to embed a 32-bit TLB into a 32-bit DLL and 64-bit TLB into a 64-bit DLL. |
Sound good to me! |
Sorry, I've been on sick leave for five days. I'm trying to catch up.
How is that possible? The TLB is generated during build and then embedded into the DLL via --tlbembed. What do I miss? So it would be one build to get the desired result?
Correct.
Exactly. If we put these pieces together, in addition to #280 , I think, we can skip the TlbEmbed task. Would you mind, if I remove the TlbEmbed task with the solution of #280?
Sounds good to me.
I think this point @marklechtermann showed up, should be definitly mentioned. This also applies to Strong Names that still have a purpose in .NET 4.8. So tlbembed must be denied, if the assembly has a strong name or we must be sure that dscom is called before the Strong name is applied. Or am I wrong, @marklechtermann ? So in conclusion: As soon as the failing acceptance test is fixed, I think we can start merging it. |
I hope you're feeling better!
Sorry, that comment was in reference to what would happen if one used
Yes, agreed. I now understand what was the problem and it was indeed with the build task. The tool task are working fine. I will not enhance the build task any further. However, I still have some work to do on the tool task & unit tests & acceptance tests to address the new changes.
Yes, I agree. I'm not too sure when this will happen but if for some reason it happens before the tool task runs, that would be a problem. For now, I'll update the documentation to make it clear that this is not a tested/supported scenario and they might need to consider using a build script outside the msbuild.
It is now passing locally but as mentioned, I still have more work, and hope to have an update later this week. |
…edTlb # Conflicts: # src/dscom.test/dscom.test.csproj
…bedTlb # Conflicts: # src/dscom.test/dscom.test.csproj
…support for passing an arbitrary index for the embedding operation. Modify the TypeLibEmbedder class to handle the index.
…ved but we need to avoid failing tests due to XML errors.
…s comhost DLL vs. .NET framework's assembly.
…tch from `--tlbembed` to `--embed` to avoid ambiguity between the verb and the option on a different verb. Also modify the `--embed` from a switch to one that can optionally accept argument. This supports the scenario where we need to provide a different assembly (e.g. the comhost DLL for .NET 5+) other than the one being embedded.
…itted to handle the embeds tests which runs into parallelization problems when running all tests. This was handled by separating the embed tests into their own CLI tests while making all a collection to ensure that xUnit do not try to parallelize the new classes for CLI tests. An abstract class is provided for common functionality between the CLI test classes. Additionally, avoid using incorrect build of assembly when running tests; the original tests were all using .NET 6.0 rather than corresponding to the .NET version of the tests running. Finally adjust the properties in the demo & test assemblies.
…y the new verb and the change in the option name from `--embedtlb` to simply `embed` for clarity and also add a warning about unsupported scenario of signing the assembly.
So according to the log, this is where it's failing, on line 47 when executing the constructor for the dscom/src/dscom.test/tests/CLITest/CLITest.cs Lines 45 to 48 in 9f46ae6
I just ran |
This is really strange! Can you implement some kind of retry? Something like this: int retryAttempts = 5;
while (retryAttempts > 0)
{
try
{
foreach (var file in Directory.EnumerateFiles(Environment.CurrentDirectory, "*.tlb"))
{
File.Delete(file);
}
}
catch (System.UnauthorizedAccessException ex)
{
Thread.Sleep(500);
}
finally
{
retryAttempts--;
}
} |
At least on NUnit there is a way to set retries by the framework using a special attribute. But I don't know if the feature is available in XUnit or if this is considered as bad practice. |
Technically the tests in the We'll see if the retry logic helps. |
...failed. Any other idea? |
So it's not a timing issue. I wonder if running those tests on a STA thread would help avoid issues. Going to try this theory out. |
… that using a single thread might avoid issues when running an external process.
.. finally. This was the solution. Thank you for your work! |
New version is v1.11.0: |
In .NET 8, they introduced the ability to embed the type library into the
*.comhost.dll
file with theComHostTypeLibrary
property. However, this property is not compatible with projects using dscom to help with exporting the type library. The reason for this is because the property expects a type library to be already present at the start of the build whereas dscom's build generates a type library as part of the build process.With this PR, we gain the ability to embed the type library that was generated as part of the build. This is exposed both as a new switch on the client tool, using
--tlbembed
option and as a new build taskTlbEmbed
which is run after theTlbExport
build task. This is an opt-in step by using the new propertyDsComTypeLibraryEmbedAfterBuild
which is defaulted tofalse
.