Skip to content

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

Merged
chamons merged 1 commit into
dotnet:masterfrom
chamons:xm_full_netstd_fix_2
Sep 21, 2017
Merged

Fix netstd2 issue on XM full by expanding facades the same as Modern#2731
chamons merged 1 commit into
dotnet:masterfrom
chamons:xm_full_netstd_fix_2

Conversation

@chamons

@chamons chamons commented Sep 20, 2017

Copy link
Copy Markdown
Contributor
  • 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.

@rolfbjarne rolfbjarne left a comment

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.

Do you have a test for this?

@chamons

chamons commented Sep 20, 2017

Copy link
Copy Markdown
Contributor Author

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

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure

@radical

radical commented Sep 20, 2017

Copy link
Copy Markdown
Member

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

chamons commented Sep 21, 2017

Copy link
Copy Markdown
Contributor Author

@chamons

chamons commented Sep 21, 2017

Copy link
Copy Markdown
Contributor Author

@chamons
chamons merged commit b905719 into dotnet: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
…otnet#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 dotnet#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
dotnet#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 dotnet/macios#2643 and
dotnet/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 dotnet/macios#2643 and
dotnet/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