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

Conversation

@Anipik
Copy link

@Anipik Anipik commented Mar 18, 2019

Related PR dotnet/arcade#2221

@Anipik Anipik changed the title Changes required to make this System.Runtime.WindowsRuntime project work with the newGenfacadeModel Changes required to use newGenfacadeModel Mar 21, 2019
@Anipik
Copy link
Author

Anipik commented Apr 2, 2019

cc @tannergooding

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

Looks like there are some problems with the aliases in manual mscorlib shim. Getting close.

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

References for manual shims are here:

<ReferencePath
Include="$(_RuntimePath)System.*.dll;$(_RuntimePath)Microsoft.Win32.*.dll;$(_RuntimePath)netstandard.dll"
Exclude="$(_RuntimePath)System.dll;$(_RuntimePath)System.Data.dll" />

None have aliases. You'll need to add references with aliases.

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

@Anipik and I looked at this. It's happening because System.Private.CoreLib had InternalsVisibleTo mscorlib in 2.0, but the GenFacades source isn't considering InternalsVisibleTo when detecting seed preferences (nor did genfacades, but it didn't try to run typeforwards through the compiler).

To fix this he can flow assembly name into the task (simple name only to avoid cracking the SNK), then check the seed assembly for an InternalsVisibleTo attribute containing that assembly name.

@Anipik
Copy link
Author

Anipik commented Apr 5, 2019

@ericstj i looked at the failure

D:\a\1\s\artifacts\obj\mscorlib\uap-Release\mscorlib.Forwards.cs(73,97): error CS0012: The type 'Enum' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'. [D:\a\1\s\src\shims\manual\mscorlib.csproj]
D:\a\1\s\artifacts\obj\mscorlib\uap-Release\mscorlib.Forwards.cs(73,12): error CS0012: The type 'Enum' is defined in an assembly that is not referenced. You must add a reference to assembly 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'. [D:\a\1\s\src\shims\manual\mscorlib.csproj]
    0 Warning(s)

The public key token for the system.private.corelib(one getting passed in reference paths) is b03f5f7f11d50a3a and it needs 7cec85d7bea7798e

any idea what should we do here ?

@ericstj
Copy link
Member

ericstj commented Apr 5, 2019

7cec is the silverlight key. I wouldn’t expect that to show up at all. You may want to see where that’s coming from by scanning the references of all assemblies being passed to the compiler. I have a tool that does this and can help later tonight.

@ericstj
Copy link
Member

ericstj commented Apr 5, 2019

The type 'System.Reflection.Emit.ParameterBuilder' is defined in multiple seed assemblies. If this is intentional, specify the alias for this type and project reference

This message should list the assemblies which contain the duplicate type.

@ericstj
Copy link
Member

ericstj commented Apr 5, 2019

You shouldn't be emitting the source file when errors are encountered. This is causing subsequent builds to skip the target which results in different errors upon incremental build.

@ericstj
Copy link
Member

ericstj commented Apr 5, 2019

Ok, so I've found the issue with the UAPAOT build. The problem here is that we have facades that are building against the coerclr System.Private.CoreLib and running against the .NETNative System.Private.CoreLib and the two have different keys. .NETNative corelib is b03f5f7f11d50a3a, CoreCLR is 7cec85d7bea7798e

The following are the problem projects:

System.Collections.Concurrent.dll
System.Diagnostics.TraceSource.dll
System.Globalization.Calendars.dll
System.Memory.dll
System.Text.Encoding.Extensions.dll
System.Threading.Tasks.Extensions.dll

We should add uap-aot configurations to these projects to fix this issue.

@jkotas are you aware of this difference? Seems like it could be something we'd want to better share forelimb surface area (eg: a System.Private.CoreLib contract between the runtimes).

@jkotas
Copy link
Member

jkotas commented Apr 5, 2019

It is left over from days when ProjectN CoreLib was very different from CoreCLR CoreLib. IIRC, the different strong name was intentional so that it is harder to mix one and the other by accident. I agree that it would be nice to fix. The workaround you have suggested sounds fine to me.

@ericstj
Copy link
Member

ericstj commented Apr 5, 2019

The workaround you have suggested sounds fine to me.

Yeah, I almost want to write validation to catch this issue. VerifyClosure ignores pkt today because the coeclr binder does, but maybe I should change that.

@Anipik
Copy link
Author

Anipik commented Apr 5, 2019

You shouldn't be emitting the source file when errors are encountered. This is causing subsequent builds to skip the target which results in different errors upon incremental build.

I am working on adding a new pr in arcade to add this as well all other features mentioned in the pr and discussed offline

@ericstj
Copy link
Member

ericstj commented Apr 5, 2019

Sounds good. Maybe we can work together next week to push this change in so we don't have to keep undoing the arcade insertions.

@Anipik
Copy link
Author

Anipik commented Apr 5, 2019

Sounds good. Maybe we can work together next week to push this change in so we don't have to keep undoing the arcade insertions.

sure :)

@Anipik
Copy link
Author

Anipik commented Apr 8, 2019

@ericstj @safern
when we are targeting the build of the corefx against uapaot, the TargetGroup property is getting reassigned to uap and targetsAot is always false. is this a bug or am i missing something here ?

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for pushing this feature through!

@Anipik
Copy link
Author

Anipik commented Apr 8, 2019

LGTM. Thanks for pushing this feature through!

Thanks for the help :)

@ericstj
Copy link
Member

ericstj commented Apr 8, 2019

when we are targeting the build of the corefx against uapaot, the TargetGroup property is getting reassigned to uap and targetsAot is always false. is this a bug or am i missing something here ?

Not a bug. It sounds like you are missing a configuration for uapaot. In that case the config system will find closest compatible configuration which is uap.

@Anipik Anipik closed this Apr 8, 2019
@Anipik Anipik deleted the temp branch April 8, 2019 18:50
@Anipik Anipik restored the temp branch April 8, 2019 18:50
@Anipik Anipik reopened this Apr 8, 2019
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

This is OK to merge once green. Please be sure we have an issue tracking the cleanup of the manual facades build.

@Anipik Anipik merged commit 52206f1 into dotnet:master Apr 8, 2019
@Anipik Anipik deleted the temp branch April 8, 2019 23:01
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* making corefx compatible with newGenfacadesTask

correcting the new runtime variable

updating genfacades version to latest

fixes uap build and removes extra seed ype preferences for uapaot

* adding build configuration, comments and updating genfacadwa version

* fixing build  for netcoreapaot

* removing netcoreappaot build from all configurations build


Commit migrated from dotnet/corefx@52206f1
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.

7 participants