Skip to content
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

System.Web.Services.Description package needed for improving WSDL support #4637

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

mconnew
Copy link
Member

@mconnew mconnew commented Jun 21, 2021

No description provided.

@mconnew mconnew requested a review from ericstj June 21, 2021 21:35
@mconnew mconnew marked this pull request as draft June 21, 2021 21:37
@mconnew
Copy link
Member Author

mconnew commented Jun 21, 2021

@ericstj, basically I'm trying to create a reference assembly using Roslyn so as not to need to maintain source files, and then generate the type forwarding facade for NetFx.

I tried doing this in the src project, but generation of the NetFx façade and the reference assembly are tied together in the ref project. So instead I'm referencing the same product code from the ref project but telling Roslyn to only generate the reference assembly. I'm running into problems where generation of the SR.xxxxx source files is being disabled in the ref project (I can't work out where so can't work out how to override). Then Roslyn instead of failing the build generates a reference assembly skipping any methods which reference resource strings, so you get a weird broken assembly (the generated assembly looks like it's missing some necessary boilerplate stuff and is really broken).

@ericstj
Copy link
Member

ericstj commented Jun 22, 2021

not to need to maintain source files

It's up to you of course, but in dotnet/runtime we see the maintenance of public API sources as a bit of a feature. It's a way for us to keep track of the public API separately and scrutinize API changes and ensure they go through review. We've considered eliminating the ref projects, but keeping the source (as an input to API Compat).

Why do you need a reference assembly here? In the cases of the other WCF assemblies they need to have a reference assembly since the types are in a different place at compile time (System.ServiceModel.*) than runtime (System.Private.ServiceModel) since that's not the case here, then I'd suggest you avoid it as it is unnecessary complexity and grows the package. You can still use the roslyn feature to generate a reference assembly in order to get the incremental build benefits if you like.

generation of the NetFx façade and the reference assembly are tied together in the ref project

All you need to generate a facade is a assembly to use as input to specify the types. The targets here assume that input is a reference assembly project.

<ProjectReference Include="..\ref\$(AssemblyName).Ref.csproj">

Those can use a different input, as I did here: ericstj@adfab46#diff-207f6e98d9caeaca81338ee172b4875cce4ac6a5f1ed4d5ce0d1eaca338dbf8fR16

Perhaps we can work on changing those targets to accomodate your scenario as well? Make the new target apply when running on either a reference assembly or a src assembly that has no reference assembly.

@mconnew
Copy link
Member Author

mconnew commented Jun 22, 2021

Why do you need a reference assembly here? In the cases of the other WCF assemblies they need to have a reference assembly since the types are in a different place at compile time (System.ServiceModel.*) than runtime (System.Private.ServiceModel) since that's not the case here, then I'd suggest you avoid it as it is unnecessary complexity and grows the package.

The reason for a reference assembly is these types exist on NetFx so we have the netstandard2.0 lib assembly for .NET Core/.NET and we need a type forwarding facade for net461 to forward to System.Web.Services.dll in the framework.

@ericstj
Copy link
Member

ericstj commented Jun 22, 2021

Please see above. I don’t think you need a reference assembly project in this case since your source assemblies have the same type location as references. If you want you can still produce reference assemblies from the compiler but you don’t need a separate project.

@mconnew
Copy link
Member Author

mconnew commented Jun 22, 2021

Are you saying I get rid of the .Ref.csproj and it can all be generated from the single project? I have too much missing context in how those changes you linked to work to be able to work out what changes I need to make this all fit together. What I gather so far is this:

  • Remove ref project
  • Add <ProduceReferenceAssembly>true</ProduceReferenceAssembly> to src project

Beyond that I'm lost as to what changes I need to make.

@ericstj
Copy link
Member

ericstj commented Jun 23, 2021

Let me make the suggested changes now that the other PR is merged.

@ericstj
Copy link
Member

ericstj commented Jun 23, 2021

So it turns out that after merging my latest changes all I needed to to was delete the reference assembly project: ericstj@9077676

A reference assembly is not needed at all, you just use the implementation assembly for reference and the same can be done for the .NETFramework façade. Note that this is the normal/best-practice way to ship packages. Reference assemblies (even when produced by the roslyn ProduceReferenceAssembly feature) are not recommended to be included in packages. The only reason we need reference assemblies in the other WCF packages is because they are different than implementation (due to typeforwarding in implementation to System.Private.ServiceModel). If you still want to add the Roslyn generated reference assembly to the package then you can set ProduceReferenceAssembly to true, then add a target that includes it, similar to

<PropertyGroup Condition="'$(HasReferenceAssembly)' == 'true'">
<TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificBuildOutput);AddReferenceAssemblyToPackage</TargetsForTfmSpecificContentInPackage>
</PropertyGroup>
<Target Name="AddReferenceAssemblyToPackage" DependsOnTargets="ResolveReferences">
<ItemGroup>
<TfmSpecificPackageFile Include="@(ResolvedMatchingContract)" PackagePath="ref/$(TargetFramework)" />
</ItemGroup>
</Target>
. Here's a sample: https://gist.github.com/ericstj/bd3aa872f0b9632d26b7b1c4f7696173.

@mconnew
Copy link
Member Author

mconnew commented Jun 23, 2021

After removing the Ref project, I also had to set EnableDefaultEmbeddedResourceItems to false for net461 as it was adding the strings as a resource. Now the project has:

  • lib\net461\System.Web.Services.Description.dll - type forwarding facade
  • lib\netstandard2.0\System.Web.Services.Description.dll - full implementation

Will the type forwarding façade work as a reference assembly if a consuming library multi-targets to net48 for example?

@ericstj
Copy link
Member

ericstj commented Jun 23, 2021

Will the type forwarding façade work as a reference assembly if a consuming library multi-targets to net48 for example?

A library targeting net48 will get the net461 asset: https://nugettools.azurewebsites.net/5.10.0-preview.2.7185/framework-precedence?framework=net48&exludedIdentifiers=
Which is what it needs in order to see the correct types (and no duplicates). Does that answer your question?

@mconnew
Copy link
Member Author

mconnew commented Jun 23, 2021

I gave it a test and looked at what's in the package and it matches what our existing packages do. The bit I don't understand is why we don't limit the set of api's visible on netfx to the api's available to netstandard2.0? When referencing System.Web.Services.Description on net48, I am able to reference in my code any types that exist in System.Web.Services in the GAC. This is also true of System.ServiceModel.Primitives which allows me to reference any types in System.ServiceModel or System.IdentityModel. I had expected a reference assembly for S.SM.Primitives for net461 to limit the set of types but it doesn't.

@ericstj
Copy link
Member

ericstj commented Jun 23, 2021

Facades aren't meant to limit the types exposed. They unify the types from one assembly with those that exist in another assembly.

In this case you're defining a new assembly System.Web.Services.Description to represent the subset of types you want to expose to netstandard2.0. When targeting netstandard2.0 the developer only sees those types, since that's all that is available to them that you've implemented against netstandard2.0. The .NETFramework project continues to see all the types which were available before (they didn't need a package or a facade for that). The facade allows them to consume a netstandard2.0 library which compiled against System.Web.Services.Description and unify these types (treat them as equivalent) to the types that are already part of the framework in System.Web.Services. This doc does a decent job at explaining it: https://github.com/dotnet/standard/blob/master/docs/history/evolution-of-design-time-assemblies.md

@mconnew mconnew marked this pull request as ready for review July 8, 2021 20:28
@mconnew mconnew removed the request for review from ericstj August 19, 2021 20:19
@mconnew
Copy link
Member Author

mconnew commented Aug 23, 2021

@HongGit, can I merge this?

@HongGit HongGit merged commit 1367356 into dotnet:main Aug 26, 2021
@mconnew mconnew deleted the SysWebServices branch October 11, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants