Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Load execution assembly for System.ServiceModel.Primitives.dll instead of reference assembly in Sgen - #32403

Merged
jiayi11 merged 2 commits into
dotnet:masterfrom
jiayi11:sgenassembly
Nov 1, 2018
Merged

Load execution assembly for System.ServiceModel.Primitives.dll instead of reference assembly in Sgen#32403
jiayi11 merged 2 commits into
dotnet:masterfrom
jiayi11:sgenassembly

Conversation

@jiayi11

@jiayi11 jiayi11 commented Sep 22, 2018

Copy link
Copy Markdown
Member

Fixes #32017
@mconnew @huanwu @Lxiamail

@jiayi11 jiayi11 added this to the 3.0 milestone Sep 22, 2018
@jiayi11 jiayi11 self-assigned this Sep 22, 2018
@jiayi11
jiayi11 requested review from huanwu and mconnew September 22, 2018 00:06

@Lxiamail Lxiamail left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use non-case sensitive comparison?

Comment thread src/Microsoft.XmlSerializer.Generator/src/Sgen.cs Outdated
string reference = s_referencedic[assemblyname];

//for System.ServiceModel.Primitives, we need to load its execution assembly rather than reference assembly
if (assemblyname.Equals("System.ServiceModel.Primitives"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if there is third party assembly having the same name as System.ServiceModel.Primitives?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is a concern as it's understood that only framework libraries start with System. Even Microsoft will often release packages beginning with "Microsoft." to indicate that while it's created by Microsoft, it's not a core system library. Basically it's a reserved namespace by well understood convention. We only stuck with System for WCF because of the pre-existence of the code on .NET Framework and it would end up being a rename. Otherwise we likely wouldn't have used System as a prefix.


In reply to: 219902748 [](ancestors = 219902748)

Comment thread src/Microsoft.XmlSerializer.Generator/src/Sgen.cs Outdated
}

if(s_referencedic.ContainsKey(assemblyname))
if (s_referencedic.ContainsKey(assemblyname))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know much about the context when this is executed, but would it be possible that nuget has downloaded and added reference to the project? (or if runtime load is needed then perhaps at least some library nuget perhaps provides)

Comment thread src/Microsoft.XmlSerializer.Generator/src/Sgen.cs
{
string reference = s_referencedic[assemblyname];

//for System.ServiceModel.Primitives, we need to load its execution assembly rather than reference assembly

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we understand why we were trying to load the reference assembly before?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We need to reflect over the developers classes to generate the code for the serializer. A serializer needed for a WCF service needs to examine attributes which are defined in S.SM.Primitives. The reflection API's for attributes cause the attribute to be instantiated which means we can't load the reference assembly. For the rest of the data types referenced, a reference assembly is sufficient as there's no instantiation required.


In reply to: 220660138 [](ancestors = 220660138)

@jiayi11
jiayi11 merged commit d76a744 into dotnet:master Nov 1, 2018
EgorBo pushed a commit to EgorBo/corefx that referenced this pull request Nov 4, 2018
@jiayi11
jiayi11 deleted the sgenassembly branch November 19, 2018 21:58
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants