Skip to content

Conversation

clairernovotny
Copy link

@clairernovotny clairernovotny commented Feb 18, 2017

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
Copy link
Member

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
Copy link
Author

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

@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 clairernovotny force-pushed the multi-tfm-support branch 6 times, most recently from 7989d2d to be0d40b Compare February 26, 2017 21:21
@clairernovotny
Copy link
Author

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
Copy link
Contributor

nguerrera commented Feb 27, 2017

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
Copy link
Contributor

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
Copy link
Author

clairernovotny commented Feb 27, 2017

@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
Copy link
Contributor

nguerrera commented Feb 27, 2017

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
Copy link
Contributor

nguerrera commented Feb 27, 2017

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
Copy link
Author

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
Copy link
Author

clairernovotny commented Feb 27, 2017

@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
Copy link
Contributor

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
Copy link
Author

@srivatsn what about WPA81/Win81?

@Nirmal4G
Copy link
Contributor

Nirmal4G commented Apr 15, 2017

@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
Copy link
Author

@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