-
Notifications
You must be signed in to change notification settings - Fork 556
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
dotnet-svcutil: update netnamedpipe pkg for metadata downloads and improve exception handling for WCFCSTools #5543
Conversation
a862454
to
a7aa89d
Compare
...dotnet-svcutil/lib/src/internalAssets/net6.0/Microsoft.Svcutil.NamedPipeMetadataImporter.dll
Outdated
Show resolved
Hide resolved
…handling for WCFCSTools
…v6.2 netnamedpipe pkg for net6.0 and 8.0 project support.
de0d6a4
to
fc7d368
Compare
…sion matching the target framework.
fc7d368
to
6c107ba
Compare
<PackageReference Include="System.ServiceModel.NetNamedPipe" Version="6.0.*" Condition="'$(TargetFramework)' == 'net6.0'"> | ||
<PrivateAssets>contentfiles;analyzers;build</PrivateAssets> | ||
<PackageReference Include="System.ServiceModel.NetNamedPipe" Version="6.2.*" Condition="'$(TargetFramework)' == 'net6.0'"> | ||
<IncludeAssets>all</IncludeAssets> | ||
</PackageReference> |
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.
since this 8.0 version of dotnet-svcutil.exe, only need to reference 8.0 version of WCF Client library.
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.
If only reference the 8.0 NetNamedPipe package, it can cause issues for user project targeting net6.0, particularly when adding net.pipe service reference with type reuse option enabled. In such case, the associated bootstrapper project will also target net6.0, causing problem situation of loading the 8.0 NetNamedPipe package assembly in a net6.0 runtime environment.
We have two potential solutions::
- Use the 6.2 named pipe package for net6.0 target and 8.0 named pipe package for the net8.0 target.
- Use the 6.2 named pipe package for both net6.0 and net8.0 targets.
The current implementation adopts the 1st option.
<ItemGroup> | ||
<PublishedDlls Include="$(OutputPath)\*.dll" /> | ||
</ItemGroup> | ||
<Copy SourceFiles="@(PublishedDlls)" DestinationFolder="$(CustomOutputFolder)\internalAssets\net6.0" Condition="$(OutputPath.Contains('net6.0'))" /> |
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.
same reason as above.
@@ -107,7 +107,7 @@ public async Task ImportMetadataAsync(Action<WsdlImporter> onWsdlImporterCreated | |||
} | |||
else | |||
{ | |||
tfn = "net6.0"; | |||
tfn = Environment.Version.Major >= 8 ? "net8.0" : "net6.0"; |
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.
Is this only relying on .NET 8.0? If so, throw some kind of exception? Is this protected somewhere else? If so, at least add a comment.
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.
Determined by NetNamedPipe package versions referenced by Microsoft.Svcutil.NamedPipeMetadataImporter.csproj's framework targets, there are two versions of the NetNamedPipe package assembly to choose from for downloading metadata: one based on .net8.0 and the other on net6.0 runtime. Here we pick the right version based on the execution CLR version.
try | ||
{ | ||
object typeInstance = Activator.CreateInstance(type, null); | ||
MethodInfo methodInfo = type.GetMethod("GetMetadatadataAsync", BindingFlags.Public | BindingFlags.Instance); |
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.
Is this necessary? If necessary, at least add comment.
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.
It is necessary. As per our original plan, we are utilizing a reflection approach to download metadata for the named pipe service. The method entry point for this is GetMetadataAsync, defined within Microsoft.Svcutil.NamedPipeMetadataImporter.csproj.
No description provided.