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.ComponentModel.Composition doesn't work on UWP #33434

Open
eerhardt opened this issue Nov 12, 2018 · 15 comments

Comments

Projects
None yet
6 participants
@eerhardt
Copy link
Member

commented Nov 12, 2018

ML.NET has recently started using System.ComponentModel.Composition to enable some extensiblity scenarios. However, using that is now breaking ML.NET on UWP apps. See: dotnet/machinelearning#1595.

I looked into this and it appears that https://www.nuget.org/packages/System.ComponentModel.Composition/ has a placholder (_._) for lib/uap10.0.16299. However, that assembly doesn't appear to ship inbox on UWP.

So something should change here. Either we should ship that assembly inbox in UWP, or we should remove the placeholder in the package.

/cc @ericstj @joperezr @danmosemsft

@joperezr

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

What happened here is that if you take a look at older versions of the UWP metapackage you will find that it indeed have System.ComponentModel.Composition.dll in it. The difference is that this was only a shim, because we were not building a contract for it yet in corefx, so it was only intended to be a shim (similar to System.dll or mscorlib.dll). After that, we decided to add this contract into corefx, so we removed the build of this shim, but never really added it inbox again to UAP which is what broke this.

@joperezr

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

FWIW, this is when we added the contract for this assembly #24921 and it is also when we stopped building the shim.

@zamont I think that in order to fix this, we need to add a <IsUAP>True</IsUAP> to release/uwp6.2 branch of corefx here: https://github.com/dotnet/corefx/blob/release/uwp6.2/src/System.ComponentModel.Composition/dir.props

@JRAlexander

This comment has been minimized.

Copy link

commented Nov 27, 2018

Is there a resolution for this?

@joperezr joperezr added this to the UWP6.1 milestone Nov 27, 2018

@joperezr

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

@zamont do you mind taking a look at this?

@zacharycmontoya

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

Yep I'll take a look at this today.

zacharycmontoya pushed a commit to zacharycmontoya/corefx that referenced this issue Dec 11, 2018

Zach Montoya
Fixes dotnet#33434: System.ComponentModel.Composition doesn't work on…
… UWP

Include System.ComponentModel.Composition and System.Security.Permissions
in-box for uap10.0.16299 to be shipped with the UWP 6.2.x metapackage.
System.ComponentModel.Composition previously shipped in-box as a facade but
once the facade was removed we didn't replace it with a full implementation,
which caused a regression.

zacharycmontoya pushed a commit that referenced this issue Dec 19, 2018

Zach Montoya
Make System.ComponentModel.Composition and System.Security.Permission…
…s in-box for uap10.0.16299 (#33980)

Include System.ComponentModel.Composition and System.Security.Permissions
in-box for uap10.0.16299 to be shipped with the UWP 6.2.x metapackage.
System.ComponentModel.Composition previously shipped in-box as a facade but
once the facade was removed we didn't replace it with a full implementation,
which caused a regression.

Fixes #33434: System.ComponentModel.Composition doesn't work on UWP
@MichalStrehovsky

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@zamont can this be closed?

@JRAlexander

This comment has been minimized.

Copy link

commented Jan 21, 2019

@eerhardt - fyi

@zacharycmontoya

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

The required CoreFx change was made but we haven't shipped the fix yet. We intend to release a UWP update this month.

@eerhardt

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

This hasn't been fixed, re-opening.

@eerhardt eerhardt reopened this Jan 30, 2019

@MichalStrehovsky

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

This hasn't been fixed, re-opening.

@eerhardt Could you clarify what you mean? Are you using a pre-release build that has Zach's change and see the issue is still present?

@eerhardt

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

@MichalStrehovsky - can you point me to Zach's change? Nevermind, I see it linked above.

The reason this isn't fixed is because it requires a new TFM, which didn't happen for 6.2 from my understanding.

@MichalStrehovsky

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

It's linked to the bug above in one of the entries above: #33980

GitHub would have autoclosed this when it got merged, but it's waiting for the change to reach master. It won't reach master because it's a servicing only change.

@zacharycmontoya

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2019

Eric is correct that shipping this change would change the CoreFx surface area, which is a breaking change and requires a new TFM, which is work we have not done for the 6.2 update. Taking this change will have to wait.

@zacharycmontoya

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

@tommcdon can you self-assign / re-assign this particular issue?

@JRAlexander

This comment has been minimized.

Copy link

commented May 9, 2019

Is there a resolution for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.