Skip to content

Conversation

@mandel-macaque
Copy link
Contributor

We need to import the iOS common after the core f# ones to ensure they do
not override the wrong ones.

Fixes: #3684

We need to import the iOS common after the core f# ones to ensure they do
not override the wrong ones.

Fixes: dotnet#3684
Copy link
Contributor

@chamons chamons left a comment

Choose a reason for hiding this comment

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

Changes like this scare me, as they can have non-trivial consequences. I'm not saying it is wrong, but it is significantly more risky that it appears at face value.

@mandel-macaque
Copy link
Contributor Author

@chamons tested locally, device tests should pick it up, but AFAIK works. @jstedfast is the master that found the issue to be honest, all merits should go his way

@jstedfast
Copy link
Member

@chamons what this patch does is make it such that the Xamarin.iOS.AppExtension.Common.targets correctly override the core Microsoft F# targets.

When the targets are imported in the original order, the Microsoft F# targets override the iOS targets which essentially destroys the whole purpose of the iOS targets.

@chamons
Copy link
Contributor

chamons commented Apr 10, 2019

Yeah, I got that. I think its right, but due to how msbuild orders things it can introduce larger behavior changes than the diff suggests.

@spouliot
Copy link
Contributor

Since it's limited to F# extensions and those do not work right now (if I understand correctly) then I don't think it's very risky (even to backport to 16.1).

@monojenkins
Copy link
Collaborator

Build success
Build succeeded
API Diff (from stable)
API Diff (from PR only) (no change)
Generator Diff (no change)
Test run succeeded

@mandel-macaque mandel-macaque merged commit 5cfc33e into dotnet:master Apr 10, 2019
@mandel-macaque mandel-macaque deleted the fix-fsharp-extensions branch April 10, 2019 22:44
@mandel-macaque
Copy link
Contributor Author

@monojenkins backport to d16-1

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