Add props/targets for PCL, Windows and Xamarin TFM's

[clairernovotny]

This is to implement #491

• Targets implemented
• Add tests

The project I added to test works when restored/built locally and overriding the MSBuildSdksPath to point to the locally built version. Not sure yet why the test in the harness is failing.

The build server will need the Xamarin and UWP workloads installed and very likely VS 2015 to get the SL5 and WP8 targets.

[dsplaisted]

I haven't had a chance to look at this in detail yet, but one thing I'd suggest is that instead of having a big cross-targeting project that tests targeting everything, just test each target individually. That will make it simpler to see what's wrong and fix it if something is failing.

[clairernovotny]

@dsplaisted Ok, I've added one test per TFM in a new test class GivenThatWeWantToBuildALibraryWithTfm. They still all fail in the test harness but build locally using the environment variable path override. There has to be something simple in the test methods/harness themselves that's causing this. Would appreciate the help to see why they're not building.

[clairernovotny]

Okay update -- got the tests working and passing. There were a few bug fixes and one really important thing -- only works with FullMSBuild.

So the tests must be run with .\build.ps1 -FullMSBuild to pass. Not sure how the tests should detect if they're on CoreCLR and not run/fail.

[nguerrera]

I still need to review all the details, but two main thoughts from cursory review:

1. For the individual TFM tests, you can have just one test asset and use a [Theory] with inlinedata for each TFM and use WithProjectChanges to insert the TFM for each. There are several like that already so search for the pattern an emulate it.

2. For the portable matching, the if contains("net45") but not contains("net451") patterns seems quite error prone. Can you normalize the thing you're matching against to always have leading and trailing separator and no whitespace or redundant separators, then simply say if contains("[separator]net45[separator]")?

[nguerrera]

I think we'll need to detect cases where language targets as missing and go with inconclusive/skipped result. It's not sustainable to have to have tons of VS workloads and VS 2015 installed for tests to pass. We should make arrangements to run in that environment often enough as an integration test, but we can't have unit tests depending on it always.

[clairernovotny]

For the portable matching, the if contains("net45") but not contains("net451") patterns seems quite error prone. Can you normalize the thing you're matching against to always have leading and trailing separator and no whitespace or redundant separators, then simply say if contains("[separator]net45[separator]")?

Hmm...maybe? So like put a + in the beginning and end of the strings. Yes, I can try that. I agree that the pattern was annoying.

[clairernovotny]

I think we'll need to detect cases where language targets as missing and go with inconclusive/skipped result. It's not sustainable to have to have tons of VS workloads and VS 2015 installed for tests to pass. We should make arrangements to run in that environment often enough as an integration test, but we can't have unit tests depending on it always.

How do you think the rest of the world feels about not having a true "xcopyable" build-server definition ;)

That said, I agree and am happy to update the tests to accommodate however that needs to be done.

[nguerrera]

How do you think the rest of the world feels about not having a true "xcopyable" build-server definition ;)

Fair. Not saying this is a situation to be proud of, just what we need to do to test given today's reality. ;)

[clairernovotny]

@nguerrera I simplified the contains searches by including the separator char on either end to be more explicit and avoid partial matches.

As for using a theory for those tests, the one issue there is that the output isn't necessarily the same across all TFM's. Some include a .dll.mdb file (iOS, etc), some include .pri files. A good indicator the build used the correct targets is to verify those particular files are present for those TFM's as they wouldn't get created otherwise.

Is there any harm in having an explicit test for each TFM rather than use a theory?

[clairernovotny]

Okay, tests are working as expected. The CI server needs to have VS 2015 with the Win8, SL5 and WP workloads installed to complete. The VS 2017 install needs the Xamarin and UWP workloads installed. Once those are on the FullFramework CI box, the tests will pass.

[nguerrera]

Even if we use distinct facts, we should avoid having dozens of nearly identical test projects to maintain. I think WithSourceChanges(... /* set TFM */) is still a good idea.

Note that as of right now, each test is copying the entire LibraryWithTfm folder leading to O(N^2) cspojs in bin\Debug\Tests where N is number of TFMs being tested. At a minimum, we need to just copy the test project that is used, but I strongly want to dedup them in source control too. It should be easy to add new TFMs moving forward without having to copy a bunch of files. You can also group tests which have the same expectations in to theories.

[nguerrera]

As I said earlier, we need build -FullMSBuild to pass locally without the Dev15 and Xamarin requirements. Can the tests check if the language targets exist and if not expect the missing language targets failure? We can then start a separate conversation about how to maximimize the coverage, but I'm not keen on a hard ordering of a huge dependency increase for CI to this PR.

[clairernovotny]

@nguerrera I can dedup the tests, but checking for the targets existence is hard since MSBuildExtensionsPath isn't set today in the current way the tests are built. The tests are built and run on CoreCLR and then exec MSBuild to do the build of the test project itself based on the -FullMSBuild flag.

How do you suggest we get that variable from?

[nguerrera]

How do you suggest we get that variable (MSBuildExtensionsPath) from?

For test purposes, we can get it relative from the DOTNET_SDK_TEST_MSBUILD_PATH environment variable that locates msbuild.exe.

[nguerrera]

I'm getting concerned about the TFMs that reach in to $(MSBuildProgramFiles32)\MSBuild\. This is bringing in target frameworks that aren't supported by VS 2017.

See https://www.visualstudio.com/en-us/productinfo/vs2017-compatibility-vs

Silverlight

Silverlight projects are not supported in this version of Visual Studio. To maintain Silverlight applications, continue to use Visual Studio 2015.

Windows Store and Windows Phone apps

Projects for Windows Store 8.1 and 8.0, and Windows Phone 8.1 and 8.0 are not supported in this release. To maintain these apps, continue to use Visual Studio 2015. To maintain Windows Phone 7.x projects, use Visual Studio 2012.

We need to draw a line somewhere and I think it is this: If I can't target a platform in VS 2017 in any way other than this change, then it should be excluded from this change.

[clairernovotny]

The reality is that people still need to be able to build these. Either it's provided in-box by the SDK or with extra's like my NuGet package. The downside with my NuGet package is that people need to know to install it and also need extra hooks because I can't get into the right place in the pipeline.

This whole thing is really about "it just works" for developers.

[clairernovotny]

@nguerrera still getting errors because even when the targets exist, as they do for Win8/Win81/UAP/WPA XAML targets, the reference assemblies are missing.
https://ci.dot.net/job/dotnet_sdk/job/master/job/release_windows_nt_fullframework_prtest/115/

[srivatsn]

Regarding SL and WP8 support, I agree with @nguerrera. VS made a decision to not support creating\building those projects with VS 2017. I don't think we should be changing that decision in this PR. While I agree that it'll be nice to have it work out of the box, doing it in this PR will only complicate servicing and contribute to support nightmares. I'd suggest filing a uservoice item for that so that the right folks get the data to reconsider that decision as appropriate.

[clairernovotny]

@srivatsn what about WPA81/Win81?

[Nirmal4G]

@onovotny I have seen and used your MS Build SDK Extras that's when I decided to learn MSBuild and build my own common build system for the projects I'm building, they are still in Incubation and not open source yet, since only recently started with some concept code and build pipeline design, so it would take a while before I open source them.

Then I saw this whole story about Sdk's using other Sdk's not possible at this current state, So here we are now...

At least I'm expecting these issues may be resolved before I open source my projects, It would be easy to manage build tasks. For now I'm learning from CoreFxLab and ZeroConf style build tasks and creating my own with .NET.Sdk as the base.

Until I know MSBuild to an extent, I'm sorry for any ignorant questions and statements I'll post in the future.

So one more Question: may be not the best place to ask this
Any other open source projects you might know sort of using layering build tasks like in above mentioned repos apart from ServiceStack, ASP.NET, Json.NET and also using with .NET.Sdk?

[clairernovotny]

@Nirmal4G there's many -- here's a couple of good ones to look at:

https://github.com/Reactive-Extensions/Rx.NET/tree/develop/Rx.NET/Source
https://github.com/MiniProfiler/dotnet
https://github.com/StackExchange/Dapper/tree/vs2017

[jahmai-ca]

What is the status of this pull request?

[nguerrera]

@jahmai You can use the https://github.com/onovotny/MSBuildSdkExtras to get the functionality now.

[jahmai-ca]

@nguerrera I can't get it working.

I've converted a Xamarin.iOS project to the new format, then added the MSBuild.Sdk.Extras package like this:

<PackageReference Include="MSBuild.Sdk.Extras" Version="1.1.0-preview.2.build.8" PrivateAssets="All" />

Then changed the TargetFramework element to (I also tried xamarinios):

Xamarin.iOS

But get the compile error:

Severity Code Description Project File Line Suppression State
Error The TargetFramework value 'Xamarin.iOS' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly. Intrinsic.Platform-iOS C:\Program Files\dotnet\sdk\2.0.0\Sdks\Microsoft.NET.Sdk\build\Microsoft.NET.TargetFrameworkInference.targets 96

Same error for xamarinios. The old format project builds fine.

[nguerrera]

@jahmai See https://github.com/onovotny/MSBuildSdkExtras#using-the-package

You need to add that Import too.

[jahmai-ca]

@nguerrera I realised that I needed to change the TFM to xamarinios10. And yes I added that import too but it still doesn't work.

Firstly MSBuildSDKExtrasTargets isn't defined which is strange because it defined in MSBuild.Sdk.Extras.props which is imported in the .csproj.nuget.g.props file.

Manually importing the MSBuild.Sdk.Extras.props does define it and then the import works, but it doesn't seem to successfully import the Platform.targets file because none of the default assemblies are (eg. Xamarin.iOS.dll) are included.

Directly importing the platforms/Xamarin.iOS.targets file also doesn't include the assembly list, even if I modify the file and remove the DisableImplicitFrameworkReferences condition. However copy-pasting the block directly into my project file does work even with the condition.

All in all I'm pretty confused about what's going on.

[clairernovotny]

@jahmai do you have a repro somewhere? Sometimes you need to close/reload the solution to make sure VS picks up the changes (just the first time). What version's of what tools are you using? Do you have the Xamarin workload installed?

[jahmai-ca]

Ok I've narrowed it down to the presence of this line in one of our props files:

<BaseIntermediateOutputPath>obj\$(Configuration)\iOS\</BaseIntermediateOutputPath>

The reason it exists is because of NuGet/Home#4463. This project sits side-by-side with other projects because they compile to platform specific binaries off the same code base and sharing the obj/ dir causes issues.

[clairernovotny]

@jahmai yeah, the new project system is finicky about the base intermediate directories.

[nguerrera]

@jahmai See #1518 (comment)

[jahmai-ca]

@nguerrera Thanks. The Directory.Build.props technique was enough for Visual Studio 2017, but not for Rider for some reason. But all I needed to do was manually add the following to the project file in addition to Directory.Build.props and it all works:

  <ItemGroup>
    <Reference Include="System" />
    <Reference Include="System.Net.Http" />
    <Reference Include="System.Xml" />
    <Reference Include="System.Xml.Linq" />
    <Reference Include="System.Core" />
    <Reference Include="Xamarin.iOS" />
  </ItemGroup>

So I call this a win, thanks!

[jahmai-ca]

But now that I've cleaned out the obj dir it no longer works in Rider. Rider fails to do a nuget restore when the TFM is xamarinios10. But it does work ok if Visual Studio already did the restore and generated the .nuget.targets/props. Argh! I'll do some more poking around...

[jahmai-ca]

Alright I figured out that if I define TargetFrameworkIdentifier and TargetFrameworkVersion manually in the project file then it keeps Rider happy and everything else works.

I actually went a step farther and managed to get multi-targetting working too:

[clairernovotny]

I would recommend filing an issue with JetBrains as those properties shouldn't be required inside your project file. They are set by the MSBuild.Sdk.Extras targets that are brought in.

[jahmai-ca]

@onovotny I will. The problem appears to be that Rider refuses to do a nuget restore if the TFM is unrecognized. Once that is done, it successfully imports MSBuild.Sdk.Extras and those properties are set correctly.

[jahmai-ca]

@onovotny Do you think that it is possible to convert Xamarin application projects to the new format or am I wasting my time trying?

[mungojam]

I tried and got it building, but debugging didn't work in VS as it didn't recognise it as debuggable

…

On Wed, 18 Oct 2017, 01:25 jahmai, ***@***.***> wrote: @onovotny <https://github.com/onovotny> Do you think that it is possible to convert Xamarin application projects to the new format or am I wasting my time trying? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#889 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADAiyxJSbf_YDB6F-RdYkY0jrB08xBcoks5stUVrgaJpZM4MFLdJ> .

[jahmai-ca]

@mungojam Thanks Mark, I don't suppose you kept the .csproj of any of the attempts?

[clairernovotny]

I'm fairly certain that you won't be able to debug the application projects. Similar issue with ASP.NET 4.6 projects too; the project system needs to know how to start the right debugger for those project types and that's not available yet.

[jahmai-ca]

Is that usually based off the OutputType and ProjectTypeGuids property in the old project format? What happens if you specify it in the new project format?

[livarcocc]

This is really something that we are not prepared to take. Plus, the extras SDK is now something that people know and are comfortable with.

Thanks for the PR in any case @onovotny