Rebase onto .NET Core 1.1#2737
Rebase onto .NET Core 1.1#2737daxian-dbw merged 5 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom randomvariable:dotnet11Copy head branch name to clipboard
Conversation
|
Hi @randomvariable, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
That should not really matter, although, I think it could be acceptable to revert back for consistency. |
There was a problem hiding this comment.
Please, use separate PRs (or at least separate commits) for such formatting cleanups. Mixing them makes review much harder.
There was a problem hiding this comment.
Thanks. Missed when diffing.
|
Appears the build in #2738 that I've ended up with doesn't allocate a tty, so is pretty unusable as an interactive shell. Not sure why. |
|
TTY bug seems unrelated to .NET 1.1, have filed issue at #2745 |
|
Not sure why the DSC tests are failing. Any ideas? I realise that the package isn't getting installed, but not sure how it's supposed to get installed either. |
Modify `Build.psm1` and `project.json` files to use .NET Core 1.1. .NET Core 1.1 ships with an older dotnet/cli than has currently been used, so we revert to use case-sensitive directory names for dependencies. .NET Core 1.1 is a pre-requisite for supporting Fedora 24.
|
Think this is ready to merge. |
daxian-dbw
left a comment
There was a problem hiding this comment.
This is an awesome PR! Thank you very much!
I left a few comments, can you please take a look?
| Start-NativeExecution { | ||
| curl -sO $obtainUrl/$installScript | ||
| bash ./$installScript -c $Channel -v $Version | ||
| bash ./$installScript -c $Channel |
There was a problem hiding this comment.
I think I had removed this whilst trying to get the 1.1 install script working, but the defaults are now set correctly elsewhere in build.psm1, so it can be restored.
| "net451": {}, | ||
| "netstandard1.6": { | ||
| "imports": ["dnxcore50", "portable-net45+win8"], | ||
| "imports": ["netcoreapp1.1", "portable-net45+win8"], |
There was a problem hiding this comment.
Why do you change dnxcore50 to netcoreapp1.1 here?
There was a problem hiding this comment.
That's an error. Will revert.
There was a problem hiding this comment.
Thanks for the quick response. Please add new commits to address comments, it's easier to review the incremental changes this way 😄 I will squash the commits when merging the PR.
| @@ -13,10 +13,10 @@ | ||
| }, |
There was a problem hiding this comment.
The dependency to Microsoft.PowerShell.PSReadLine should be "6.0.0-*". It's an oversight from the change that bounced the project versions. Would you mind update it in this PR?
There was a problem hiding this comment.
Done. Commits upcoming.
| "dependencies": { | ||
| "Microsoft.CodeAnalysis.CSharp": "1.3.1-beta1-20160616-03", | ||
| "System.Diagnostics.TextWriterTraceListener": "4.3.0-preview1-24530-04" | ||
| "Microsoft.CodeAnalysis.CSharp": "1.3.2", |
There was a problem hiding this comment.
Since we are using the latest version on nuget.org for other packages, shall we use the 2.0.0-rc version for Microsoft.CodeAnalysis.CSharp? The 2.0.0-rc was published on Nov 18.
There was a problem hiding this comment.
The build and tests succeeded against 2.0.0-rc, so this looks fine to go in.
…t.CodeAnalysis.Csharp
|
Commits added for each of the review comments. |
|
The Travis CI failure is a known issue, not related to your PR. I have restarted it. I will merge this PR once it succeeds. |
|
@randomvariable Thanks again for your contribution! |
Build.psm1andproject.jsonfiles modified to use .NET Core 1.1..NET Core 1.1 ships with an older dotnet/cli than has currently been used,
so we revert to use case-sensitive directory names for dependencies.
.NET Core 1.1 is a pre-requisite for supporting Fedora 24.