Skip to content

Port Microsoft.DotNet.XUnitRunnerUap#355

Merged
ViktorHofer merged 26 commits into
dotnet:masterfrom
ViktorHofer:xunituap
Aug 14, 2018
Merged

Port Microsoft.DotNet.XUnitRunnerUap#355
ViktorHofer merged 26 commits into
dotnet:masterfrom
ViktorHofer:xunituap

Conversation

@ViktorHofer
Copy link
Copy Markdown
Member

@ViktorHofer ViktorHofer commented Jul 22, 2018

The Directory.Build.props and Directory.Build.targets don't play nice with the UWP project therefore I created empty ones in the project to disable the root ones. Tips how to condition the root ones correctly are welcome 😁

Changes:

  • RemoteExecutor service removed as the runner now supports multiple instances
  • Output logging removed as we can now directly log into the console and capture the output
  • Dependencies updated & code cleanup
  • Added README instructions for the runner and launcher

AlexGhiondea and others added 19 commits March 29, 2017 13:17
…ts runs and a script to produce the runner.

This should allow anyone to build a runner folder with all the right things.
* Add Remote Execution to UAP runner

* Remove System.Diagnostics dependency

* Use StringBuilder

* Disable building the UAP WinRT Component project
* Improve UAP Runner logging and enable showprogress flag

* PR Feedback and update stdout file location

* Fix typo and add new line in output
* Improve UAP Runner logging and enable showprogress flag

* PR Feedback and update stdout file location

* Fix typo and add new line in output

* Fix deadlock caused in ui thread
@ViktorHofer ViktorHofer self-assigned this Jul 22, 2018
@ViktorHofer ViktorHofer requested a review from safern July 22, 2018 21:06
@ViktorHofer
Copy link
Copy Markdown
Member Author

@tarekgh I updated the app to a UWP console application targeting the latest version 1803. I tried to updated your script but it seems the dependencies have changed since you wrote it. Do you know which files are necessary and must included? Or do you know somebody to find that out?

@tarekgh
Copy link
Copy Markdown
Member

tarekgh commented Jul 23, 2018

@joperezr should be the one who is familiar with the script and the UWP dependency.

@tmat
Copy link
Copy Markdown
Member

tmat commented Jul 23, 2018

The Directory.Build.props and Directory.Build.targets don't play nice with the UWP project

What do you mean? Do you see any build errors? If so please file an issue and share a binlog.

@tmat
Copy link
Copy Markdown
Member

tmat commented Jul 23, 2018

OK two possible issues:

  1. C++ project - Arcade.SDK doesn't support C++ projects currently
  2. The UAP project doesn't use SDK-style projects
    @nguerrera Is it possible to use .NET Core SDK projects for UAP?

@nguerrera
Copy link
Copy Markdown
Contributor

@nguerrera Is it possible to use .NET Core SDK projects for UAP?

Nothing is built-in, but there is https://www.nuget.org/packages/MSBuild.Sdk.Extras/ from the community.

Also, it doesn't have to be necessary to import Microsoft.NET.Sdk to use the Arcade SDK.

@markwilkie
Copy link
Copy Markdown
Member

I can imagine needing UWP projects like this one, as well as other .NET Framework type projects that currently aren't supported by the SDK (ex: WinForms/WPF). We could certainly use another SDK that builds on top of the Arcade SDK to augment things if we felt that was easier then just adding the conditions in the places we discover we need. In the meantime we can let those projects override what they need and we can try and find the pattern we need/want to share if and when we hit critical mass for those project types.

I agree with this thinking - the "line" as I see it is for packages that are required. For those, they need to conform.

However, for different repos that would like to build packages for their purposes, I don't see a problem with them building and publishing in the arcade repo. Of course, the farther away these packages "stray" from the "standard" arcade sdk style projects, the less they get to participate in the value that Arcade brings. However, that value is a sliding scale...

@markwilkie
Copy link
Copy Markdown
Member

@tmat and @weshaggard - I'm ok w/ this going into Arcade and I think it makes sense, even if it's not a SDK-style project. You guys have any hard pushback? The way I see it is that so long as the Arcade SDK itself is not affected, the guidelines for packages that teams want to bring on is looser.

@weshaggard
Copy link
Copy Markdown
Member

I'm good with these going into arcade.

I do want to call out that my comments were not just about this project going into arcade but also about any projects in any repo that consumes arcade. We need to be able to provide some level of support for those projects if they cannot be SDK style projects.

@tmat
Copy link
Copy Markdown
Member

tmat commented Jul 25, 2018

I'm fine as long as the issue that was hit by this PR is investigated.

@ViktorHofer
Copy link
Copy Markdown
Member Author

The PR isn't in a mergeable state yet, I first need to update the UWP appx script (buildAndUpdateRunner.bat).

@weshaggard
Copy link
Copy Markdown
Member

I'm fine as long as the issue that was hit by this PR is investigated.

I'm not sure what you want investigated. If I understand the issue correctly it is caused by the auto importing of Directory.builds.props/targets in the root of Arcade into a non SDK project which I wouldn't expect to work in general because of the assumptions being made in those props/targets about being an SDK project. So unless your expectation is that the auto-imported props/targets should work for any project type, which that isn't my expectation, then having empty versions of those to black the importing of the root versions seems like a reasonable thing.

@tmat
Copy link
Copy Markdown
Member

tmat commented Jul 25, 2018

Yes, I would expect it to mostly work. There are certainly some dependencies on .NET SDK - I am aware of a few. However, the errors above indicate an issue that I am not aware of. It would be useful to find out what it is, so that we can

provide some level of support for those projects if they cannot be SDK style projects.

as you suggested.

@ViktorHofer
Copy link
Copy Markdown
Member Author

ViktorHofer commented Aug 11, 2018

@joperezr @safern this is now ready to be merged. PTAL

@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">

This comment was marked as spam.

This comment was marked as spam.

@@ -0,0 +1 @@
# Microsoft.DotNet.XUnitRunnerUap No newline at end of file

This comment was marked as spam.

@joperezr
Copy link
Copy Markdown
Member

@ViktorHofer can you confirm that all of the code from the Xunit Launcher (C++) was copied exactly as it is on buildtools without any changes? Same goes for the actual Unit Test runner. If there were some changes required in order to get stuff to build, it would be nice to see which changes were required (apart from the need to add the Directory.Build.* files)

@@ -0,0 +1,147 @@
<?xml version="1.0" encoding="utf-8"?>
<Project ToolsVersion="15.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

This comment was marked as spam.

This comment was marked as spam.

@ViktorHofer
Copy link
Copy Markdown
Member Author

Only look at my commits, not the others ported over from buildtools. I didn't touch the WindowsStoreAppLauncher at all, you can skip reviewing that completely. The runner of course was rewritten as a UWP console app. I didn't change any logic involved with that.

@ViktorHofer
Copy link
Copy Markdown
Member Author

ViktorHofer commented Aug 14, 2018

I would like to merge this today to unblock the xunit work in corefx / other repos and will follow-up with what needs to be done to have a minimal Directory.Build.props and Directory.Build.targets support.

Copy link
Copy Markdown
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM.

joperezr and others added 6 commits August 14, 2018 20:50
* Improve UAP Runner logging and enable showprogress flag

* PR Feedback and update stdout file location

* Fix typo and add new line in output
* Improve UAP Runner logging and enable showprogress flag

* PR Feedback and update stdout file location

* Fix typo and add new line in output

* Fix deadlock caused in ui thread
Update runner and add static manifest

Add runner and launcher instructions

Delete unused file
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.