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

Fix netstd2 issue on XM full by expanding facades the same as Modern #2731

Merged
merged 1 commit into from
Sep 21, 2017

Conversation

chamons
Copy link
Contributor

@chamons chamons commented Sep 20, 2017

  • The idea is to force Full and Modern to expand facades the same way. That way, we get the same, working behavior.
  • f79f2e4 was not sufficient, even though it matched XI, because of the difference between XI (and Modern) and what Full was doing.

Copy link
Member

@rolfbjarne rolfbjarne left a comment

Choose a reason for hiding this comment

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

Do you have a test for this?

@chamons
Copy link
Contributor Author

chamons commented Sep 20, 2017

Yes, but they are disabled due to https://bugzilla.xamarin.com/show_bug.cgi?id=53164

@monojenkins
Copy link
Collaborator

Build failure

@radical
Copy link
Contributor

radical commented Sep 20, 2017

I would add to the commit message, that
f79f2e4 did #2685 (comment)
and ..

And that was problematic because it was expanding the netstandard facades from `Microsoft.NET.Build.Extensions`
in the `ImplicitlyExpandNETStandardFacades` target.
But we want to build against XM's bundled facades *only*. So we disable the ns facades completely
by setting `$(ImplicitlyExpandNETStandardFacades) = false`.

But now we are in the situation where a XM/Full project referencing a ns project might fail to build
because of a missing `netstandard.dll` reference! And this same case was fixed for XM/Modern projects in
https://github.com/xamarin/xamarin-macios/pull/2643 . So, we enable the use of that for XM/Full projects too
through `Xamarin.Mac.msbuild.targets`.

@chamons
Copy link
Contributor Author

chamons commented Sep 21, 2017

@chamons
Copy link
Contributor Author

chamons commented Sep 21, 2017

@chamons chamons merged commit b905719 into xamarin:master Sep 21, 2017
@chamons chamons deleted the xm_full_netstd_fix_2 branch September 21, 2017 18:40
chamons added a commit to chamons/xamarin-macios that referenced this pull request Sep 27, 2017
…amarin#2731)

- https://bugzilla.xamarin.com/show_bug.cgi?id=59474
- The idea is to force Full and Modern to expand facades the same way. That way, we get the same, working behavior.
- f79f2e4 was not sufficient, even though it matched XI, because of the difference between XI (and Modern) and what Full was doing.
- Some context:

PR xamarin#2685

And that was problematic because it was expanding the netstandard facades from `Microsoft.NET.Build.Extensions`
in the `ImplicitlyExpandNETStandardFacades` target.
But we want to build against XM's bundled facades *only*. So we disable the ns facades completely
by setting `$(ImplicitlyExpandNETStandardFacades) = false`.

But now we are in the situation where a XM/Full project referencing a ns project might fail to build
because of a missing `netstandard.dll` reference! And this same case was fixed for XM/Modern projects in
xamarin#2643 . So, we enable the use of that for XM/Full projects too
through `Xamarin.Mac.msbuild.targets`.
jonpryor pushed a commit to dotnet/android that referenced this pull request Feb 27, 2018
Context: #1154

This PR brings in changes from xamarin/xamarin-macios#2643 and
xamarin/xamarin-macios#2731 to improve our .NET Standard support.
While this does not fix the packaging problem in #1154 it will give
us parity with the iOS code base.
jonpryor pushed a commit to dotnet/android that referenced this pull request Feb 27, 2018
Context: #1154

This PR brings in changes from xamarin/xamarin-macios#2643 and
xamarin/xamarin-macios#2731 to improve our .NET Standard support.
While this does not fix the packaging problem in #1154 it will give
us parity with the iOS code base.
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.

8 participants