Initial PowerShelll kernel prototype by TylerLeonhardt · Pull Request #9 · dotnet/interactive

TylerLeonhardt · 2020-01-23T21:54:24Z

Original PR: dotnet/try#751 (all feedback addressed)

This handles the 3 basic messages:

HandleSubmitCode
HandleRequestCompletion
HandleCancelCurrentCommand (implemented but waiting until dotnet-interactive supports it better)

This change also includes tests 🙂

Example PowerShell notebook that came from this very change.

cc @SteveL-MSFT @daxian-dbw

TylerLeonhardt · 2020-01-23T23:58:07Z

Ok I believe I've addressed all feedback from the original PR and cleaned up some copy-paste relics.

daxian-dbw · 2020-01-24T00:14:58Z

...Modules/Microsoft.DotNet.Interactive.PowerShell/Microsoft.DotNet.Interactive.PowerShell.psd1

+# Generated by: tyleonha
+#
+# Generated on: 1/23/2020
+#


Replace this comment section with the following?

// // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. //

The copyright and license we're using elsewhere is:

// Copyright (c) .NET Foundation and contributors. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information.

updated 👍 Not sure what you want the AuthorName and CompanyName to be in the psd1 so I left it at Microsoft for now. Let me know if you prefer something else.

Should probably be .NET Foundation instead of Microsoft

TylerLeonhardt · 2020-01-24T19:01:16Z

Not really sure what to do about the CI failure:

.packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.20073.17\tools\Sign.proj(81,5): error SIGN001: Signing 3rd party library 'D:\a\1\s\artifacts\tmp\Release\ContainerSigning\200\tools/netcoreapp3.1/any/Markdig.Signed.dll' with Microsoft certificate 'Microsoft400'. The library is considered 3rd party library due to its copyright: 'Alexandre Mutel'.

brettfo · 2020-01-24T19:52:02Z

@TylerLeonhardt For the Markdig.Signed.dll issue, you'll have to add an entry to this file.

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs

...oft.DotNet.Interactive.PowerShell.Tests/Microsoft.DotNet.Interactive.PowerShell.Tests.csproj

Microsoft.DotNet.Interactive.PowerShell/Microsoft.DotNet.Interactive.PowerShell.csproj

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs

daxian-dbw

LGTM

Microsoft.DotNet.Interactive.PowerShell/Commands/TracePipelineObjectCommand.cs

TylerLeonhardt · 2020-01-29T01:12:11Z

rebased and added better support for parse errors:

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs

TylerLeonhardt · 2020-01-29T02:30:01Z

Can someone help me understand the CI failure?

.packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.20077.7\tools\Publish.proj(283,5): error : Invalid ELF BuildID '<null>' for tools/netcoreapp3.1/any/runtimes/linux-x64/native/libmi.so

jonsequitur · 2020-01-29T16:35:30Z

@brettfo Does this build failure ring any bells?

Microsoft.DotNet.Interactive.PowerShell/PowerShellExtensions.cs

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs

colombod

I am concerned with the lack of tests.

brettfo · 2020-01-29T21:18:29Z

@jonsequitur The build failure is due to dotnet/symstore#81. I've sent email internally to figure out what the workaround is and how to apply it here. I'll reply once I have something actionable.

TylerLeonhardt

I've added powershell to a few tests and added a test to test all the different streams and their order.

Any other tests you'd like?

jonsequitur · 2020-01-30T17:08:07Z

Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

                e => e.Should().BeOfType<CommandHandled>());
        }
+
+        [Fact(Timeout = 45000)]


You don't need to add the timeouts.

All the other tests in this file have timeouts for similar tests. Should remove those as well?

If you want to, feel free. If not, we'll do it. These were an artifact of an investigation into timeout and memory issues that were plaguing us but have been resolved.

I removed them just from my added tests.

brettfo · 2020-01-30T18:26:42Z

@TylerLeonhardt Can you try merging/cherry-picking/manually-applying the head commit in the snupkg branch? It may help with the Invalid ELF BuildID '<null>' issue.

TylerLeonhardt · 2020-01-30T18:53:03Z

@brettfo doesn't look like that did the trick :(

brettfo · 2020-01-30T23:45:29Z

I just added a commit that manually strips the offending files from the symbols packages. Build worked locally, now we'll see what happens in the PR.

brettfo · 2020-01-31T00:31:17Z

The recent MSBuild hackery gets us past the symbol uploading and the Linux leg now passes.

I'll pass this back to @TylerLeonhardt now to look into the test failure(s) in Windows and add any extra features/tests you may want.

Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs

…t to look like

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs

Microsoft.DotNet.Interactive.PowerShell/StreamHandler.cs

TylerLeonhardt · 2020-01-31T23:10:45Z

🎉🎉🎉🎉

brettfo · 2020-02-01T19:18:50Z

@TylerLeonhardt The full-signed build with the PowerShell kernel failed due to signing issues (link here). Do you know who owns the publishing of the Microsoft.Management.Infrastructure package? Looking at the NuGet packages, there are different public keys used for Unix vs. Windows binaries, and not everything is consistent. I'm not aware of a workaround on our end that could avoid checking those assemblies, but I'll keep looking.

I tried using the 3PartySHA2 key in eng/Signing.props, but that doesn't pass build, while the None key currently specified passes build but not official signing validation.

My guess is that this package (and it's Unix and Windows sub-packages) will have to be re-published with consistent signing/keys.

TylerLeonhardt · 2020-02-01T19:23:15Z

@adityapatwardhan @TravisEz13 @SteveL-MSFT can one of you guys offer some advice here? I'm not familiar with this package.

TravisEz13 · 2020-02-01T22:15:16Z

@brettfo The Windows and non-Windows packages are separate packages from separate sources and policy says we should sign them with different Authenticode signatures.

What kind of signing are we talking about here? Authenticode or strong name?
Update: the tool appears to be checking Authenticode signatures.

brettfo · 2020-02-03T21:55:23Z

After speaking with some people, I found a way to exclude just that one file from the signing checks due to the different signatures. See PR #110.

TylerLeonhardt mentioned this pull request Jan 23, 2020

WIP Initial PowerShelll kernel prototype dotnet/try#751

Closed

TylerLeonhardt requested a review from jonsequitur January 23, 2020 23:58

daxian-dbw reviewed Jan 24, 2020

View reviewed changes

TylerLeonhardt marked this pull request as ready for review January 24, 2020 19:31

TylerLeonhardt changed the title ~~WIP Initial PowerShelll kernel prototype~~ Initial PowerShelll kernel prototype Jan 24, 2020

daxian-dbw reviewed Jan 25, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs Show resolved Hide resolved

daxian-dbw reviewed Jan 25, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs Outdated Show resolved Hide resolved

daxian-dbw approved these changes Jan 27, 2020

View reviewed changes

TylerLeonhardt force-pushed the add-powershell-subkernel branch from ac96a71 to 3c05dbc Compare January 27, 2020 22:55

daxian-dbw reviewed Jan 28, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/Commands/TracePipelineObjectCommand.cs Outdated Show resolved Hide resolved

TylerLeonhardt force-pushed the add-powershell-subkernel branch from a1c0b3e to fdf28a3 Compare January 29, 2020 01:05

daxian-dbw reviewed Jan 29, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs Outdated Show resolved Hide resolved

colombod requested changes Jan 29, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/PowerShellExtensions.cs Show resolved Hide resolved

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs Outdated Show resolved Hide resolved

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs Outdated Show resolved Hide resolved

colombod requested changes Jan 29, 2020

View reviewed changes

TylerLeonhardt commented Jan 29, 2020

View reviewed changes

TylerLeonhardt force-pushed the add-powershell-subkernel branch from 8c13d8d to 2a08ea9 Compare January 29, 2020 22:57

jonsequitur reviewed Jan 30, 2020

View reviewed changes

colombod requested changes Jan 31, 2020

View reviewed changes

Microsoft.DotNet.Interactive.Tests/LanguageKernelTests.cs Show resolved Hide resolved

TylerLeonhardt and others added 16 commits January 31, 2020 10:45

remove manditory from InputObject

0802ffe

handle parse errors and fix trace-pipelineobjectcommand param

6c8095e

signing file change

bcf0295

whitespace

24b0ced

one more signing

dd2a0c6

change to none

d4b3018

remove IExtensibleKernel for now until we understand what we want tha…

3bc49d7

…t to look like

some tests and small changes to formatting

8d61e78

refactor tests

1913ea2

powershell telemetry & hook up forgotten Progress stream

e3839bb

updating progresses!

9ae44d5

add progress test

807977c

help for ci failures

dfcded8

exclude files from symbols packages

9318b4e

rewrite package file stripper in pure MSBuild for cross-plat

bc99793

workaround PowerShell bug

1cb3994

TylerLeonhardt force-pushed the add-powershell-subkernel branch from 248f653 to 1cb3994 Compare January 31, 2020 18:49

jonsequitur reviewed Jan 31, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/PowerShellKernel.cs Outdated Show resolved Hide resolved

jonsequitur reviewed Jan 31, 2020

View reviewed changes

Microsoft.DotNet.Interactive.PowerShell/StreamHandler.cs Outdated Show resolved Hide resolved

jonsequitur approved these changes Jan 31, 2020

View reviewed changes

TylerLeonhardt added 2 commits January 31, 2020 11:21

Jon feedback

ed4dac7

remove timeout on new tests

d9f25ab

jonsequitur merged commit d880b2a into dotnet:master Jan 31, 2020

TylerLeonhardt deleted the add-powershell-subkernel branch January 31, 2020 23:09

brettfo mentioned this pull request Feb 3, 2020

don't check authenticode signature for third-party assemly #110

Merged

Conversation

TylerLeonhardt commented Jan 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TylerLeonhardt commented Jan 23, 2020

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!

TylerLeonhardt commented Jan 24, 2020

Uh oh!

brettfo commented Jan 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TylerLeonhardt commented Jan 29, 2020

Uh oh!

Uh oh!

TylerLeonhardt commented Jan 29, 2020

Uh oh!

jonsequitur commented Jan 29, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

colombod left a comment

Choose a reason for hiding this comment

Uh oh!

brettfo commented Jan 29, 2020

Uh oh!

TylerLeonhardt left a comment

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!

brettfo commented Jan 30, 2020

Uh oh!

TylerLeonhardt commented Jan 30, 2020

Uh oh!

brettfo commented Jan 30, 2020

Uh oh!

brettfo commented Jan 31, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TylerLeonhardt commented Jan 31, 2020

Uh oh!

brettfo commented Feb 1, 2020

Uh oh!

TylerLeonhardt commented Feb 1, 2020

Uh oh!

TravisEz13 commented Feb 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettfo commented Feb 3, 2020

Uh oh!

Reviewers

TylerLeonhardt commented Jan 23, 2020 •

edited

Loading

TravisEz13 commented Feb 1, 2020 •

edited

Loading