[XM] Improve XM msbuild support and add roslyn netstandard test case #1846

chamons · 2017-03-08T21:01:23Z

Significant changes to target file under msbuild, ImplicitFacade processing in particular
Tests are disabled due to https://bugzilla.xamarin.com/show_bug.cgi?id=53164 where we can't tests local target files only global
Requires a mono msbuild with mono/msbuild@95ab657 for tests to pass
Until then, this is a workaround:
sudo cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/msbuild/15.0/bin/Roslyn/System.Reflection.Metadata.dll /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/msbuild/15.0/bin/

- Significant changes to target file under msbuild, ImplicitFacade processing in particular - Tests are disabled due to https://bugzilla.xamarin.com/show_bug.cgi?id=53164 where we can't tests local target files only global - Requires a mono msbuild with mono/msbuild@95ab657 for tests to pass - Until then, this is a workaround: sudo cp /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/msbuild/15.0/bin/Roslyn/System.Reflection.Metadata.dll /Library/Frameworks/Mono.framework/Versions/Current/lib/mono/msbuild/15.0/bin/

monojenkins · 2017-03-08T22:18:58Z

Build success

rolfbjarne · 2017-03-09T06:28:00Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.ImplicitFacade.msbuild.targets

+-->
+
+<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+	<!-- Unfortuntly Microsoft.NETFramework.CurrentVersion.targets defines this and is not included in Mobile/Modern builds so we'll add it here -->


Unfortuntly?

Unfortunately my spelling is as bad as it looks...

rolfbjarne · 2017-03-09T06:35:02Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.msbuild.targets

+Copyright (C) 2015 Xamarin Inc. All rights reserved.
+***********************************************************************************************
+-->
+<!-- Rename file -->


Maybe delete this comment?

rolfbjarne · 2017-03-09T06:38:28Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.ObjCBinding.CSharp.targets

 	<Import Project="$(MSBuildBinPath)\Microsoft.CSharp.targets" />
 	<Import Project="Xamarin.Mac.ObjCBinding.Common.targets" />

+	<PropertyGroup>


Will this work with F# projects too (does XM even have support for F#)?

It does in theory. I did not look into getting it working with msbuild, but let me see how bad it is...

So I tested it out and with msbuild fsharp works with this change, I did not dig into why.

Now, that file in particular is with binding project, but they really don't have F# support (you can bind the libs and use them in F# though).

monojenkins · 2017-03-09T17:02:39Z

Build success

radical · 2017-03-13T18:10:14Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.ImplicitFacade.msbuild.targets

+	<!-- Implicitly references all portable design-time facades if the user is referencing a System.Runtime-based portable library -->
+	<Target Name="ImplicitlyExpandDesignTimeFacades" DependsOnTargets="$(ImplicitlyExpandDesignTimeFacadesDependsOn)">
+
+	<PropertyGroup>


Indent all the elements under the Target.

radical · 2017-03-13T18:12:00Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.ImplicitFacade.msbuild.targets

+-->
+
+<Project DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+	<!-- Microsoft.NETFramework.CurrentVersion.targets defines this and that target file is not pulled in to Mobile/Modern builds -->


IMHO, it would be useful to make this more prominent/visible for future readers, especially the part that the default targets file will not be pulled in for Mobile/Modern builds.

Also, mention or mark the portions of this file which are from Microsoft.NETFramework.CurrentVersion.targets, and point out any changes from that. It will be useful for debugging in future or sync'ing up changes from that file.

radical · 2017-03-13T18:15:26Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.ImplicitFacade.msbuild.targets

+this file. This file also defines targets to produce an error if the specified targets
+file does not exist, but the project is built anyway (command-line or IDE build).
+
+Copyright (C) 2015 Xamarin Inc. All rights reserved.


Copyright needs an update?

radical · 2017-03-13T18:19:54Z

msbuild/Xamarin.Mac.Tasks/Xamarin.Mac.Common.targets

+		Condition="'$(UseXamMacFullFramework)' == 'true' And '$(IsXBuild)' == 'true' "/>
+
+	<Import Project="$(MSBuildThisFileDirectory)Xamarin.Mac.msbuild.targets"
+		Condition="'$(IsXBuild)' == 'false' "/>


The usual pattern is - '$(IsXBuild)' != 'true', that way the default/empty value "means" false. Just a nitpick.

chamons · 2017-03-13T21:59:56Z

The was marked dont-merge because it started failing tests locally for me, but it turns out after research that I reinstalled mono and "lost" the System.Reflection.Metadata.dll hack noted above.

That meant it would fail first build and pass second.

With the hack it works first and second time.

chamons · 2017-03-13T22:02:47Z

@radical 's code review requests are in.

radical · 2017-03-13T22:06:58Z

LGTM! 👍

monojenkins · 2017-03-13T23:18:14Z

Build success

chamons · 2017-03-14T13:33:58Z

While reviewing the diff one last time, I noticed my (commented out for now) tests were not executing the built project. While technically it isn't needed, seeing if the app actually works seems like a great idea.

Landed change in PR. Will submit when it builds green.

monojenkins · 2017-03-14T14:48:31Z

Build failure

chamons · 2017-03-14T16:06:49Z

Test failure was a random one:

https://bugzilla.xamarin.com/show_bug.cgi?id=53312

dnfclas added the cla-already-signed label Mar 8, 2017

rolfbjarne reviewed Mar 9, 2017

View reviewed changes

Fix silly comment mistakes

899f763

rolfbjarne approved these changes Mar 10, 2017

View reviewed changes

rolfbjarne requested a review from jstedfast March 10, 2017 11:40

jstedfast approved these changes Mar 10, 2017

View reviewed changes

radical reviewed Mar 13, 2017

View reviewed changes

chamons added the do-not-merge Do not merge this pull request label Mar 13, 2017

chamons removed the do-not-merge Do not merge this pull request label Mar 13, 2017

Add code review changes. Comments, tabbing and inverting logic

0c8eeca

Extend Roslyn smoke tests to actually run the generated project

c581544

chamons merged commit abf0f4a into master Mar 14, 2017

chamons deleted the xm_netstd_msbuild branch March 14, 2017 16:08

chamons mentioned this pull request Mar 29, 2017

[macos] Fix msbuild issues with shprojj/projitem library #1936

Merged

[XM] Improve XM msbuild support and add roslyn netstandard test case #1846

[XM] Improve XM msbuild support and add roslyn netstandard test case #1846

Uh oh!

Conversation

chamons commented Mar 8, 2017

Uh oh!

monojenkins commented Mar 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chamons commented Mar 13, 2017

Uh oh!

chamons commented Mar 13, 2017

Uh oh!

radical commented Mar 13, 2017

Uh oh!

monojenkins commented Mar 13, 2017

Uh oh!

chamons commented Mar 14, 2017

Uh oh!

monojenkins commented Mar 14, 2017

Uh oh!

chamons commented Mar 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants