Add PowerShell global to docker-sdk images #960

adityapatwardhan · 2019-03-05T00:48:40Z

Add PowerShell global tool as part of the SDK image.

Download the tool from the fwlink
Validate checksum where possible
Install tool
Clean up dowloaded package
Setup symbolic on Linux or add to PATH on Windows.

Related to #360

Our release pipeline tests installation of global tool and executing some very basic commands on Windows and Linux.

dnfclas · 2019-03-05T00:48:52Z

All CLA requirements met.

adityapatwardhan · 2019-03-05T00:51:21Z

@MichaelSimons @richlander

I have a couple of questions:

Should I make pwsh the default CMD?
In most cases RUN dotnet help is not needed anymore since I use dotnet to install the tool. Should I remove it?

MichaelSimons · 2019-03-05T01:47:24Z

@adityapatwardhan - Thanks for making this PR. To answer your questions

I don't think we should be doing this.
I'm on the fence on this. Ultimately I think the CLI should have a way to explicitly trigger the first run experience. If this existed I would say definitely no. So given the intent, I lean towards leaving it.

Regarding your changes, can you describe the type of automated tests you plan on adding for PS?

3.0/sdk/nanoserver-1809/amd64/Dockerfile

MichaelSimons · 2019-03-05T02:00:49Z

3.0/sdk/alpine3.9/amd64/Dockerfile

+# Install PowerShell global tool
+ENV POWERSHELL_VERSION 6.2.0-rc.1
+
+RUN curl -SL --output PowerShell.Linux.$POWERSHELL_VERSION.nupkg 'https://go.microsoft.com/fwlink/?linkid=2080856' \


Is there a reason why the nupkg can't be pulled directly from NuGet?

The PowerShell.Linux and PowerShell.Windows are special packages with reduced size that were created for this purpose. We do not plan to ship these on NuGet. We will be shipping a package called PowerShell on NuGet which is larger since it has Windows and Linux assemblies and a shim layer.

Our Dockerfiles are more than just the source for the Docker images we produce. They often get referenced as the way to do things by our customers. I am pretty certain you will see this pattern copied and referenced as the way to install PS as a dotnet tool. This feels like a potential source of confusion. Would it make sense to have a differentiated name for these packages and publish them to NuGet?

These packages support a small set of runtimes tailored towards platforms supported by dotnet-docker images. This was done to reduce the size of the package.
Hence, even a PowerShell.Linux package would not work on all supported Linux platforms and would create confusion as a global tool. The tool we plan to publish on NuGet has all the runtimes we support.
Would adding a comment explaining this and mentioning the package is internal and not for public consumption directly be sufficient?

I have less issue with this pattern. I think it is fine for us/anyone to publish specialized to other NuGet feeds. My issue is the fwlink. I'm not at all comfortable with that. There are at least three problems with it:

It is not OSS-friendly. Users have no idea what that points to (beyond traversing the link).

It is not good for versioning. Ideally, I'd like to see each Dockerfile point to a specific version of PowerShell, just like we do with .NET Core. This means that you can go back and re-build an old Dockerfile and get the same result. That old Dockerfile might result in images with security vulnerabilities, but that's actually a good thing in this case.

It's not good for engineering but it is unclear from looking at this source what the version coupling is between .NET Core and PowerShell.

In short, fwlinks are opaque.

We have a tool called DARC that you can likely use to help with this. It enables you to publish LKG versions to a database, as part of your build, that this repo can subscribe and then versions flow automatically w/o you doing anything. That would be my recommendation.

I have updated the URLs to a publicly accessible blob.

@MichaelSimons Should I add a comment saying the packages are not for public consumption?

They are for public consumption by including them in our open source Dockerfiles. An expected use case is that there will be a set of users that will copy our Dockerfiles and build their own images for various reasons. What may be useful to clarify is what is different about this NuGet versus your general purpose NuGet package.

Agreed. I will add a comment explaing the runtimes it supports and also mention that there is global tool published on NuGet.org which supports all runtimes.

MichaelSimons · 2019-03-14T15:03:43Z

@adityapatwardhan - Can you answer this question I asked earlier?

can you describe the type of automated tests you plan on adding for PS?

In this repo we mostly test at a scenario level. It seems like it should be easy to execute a simple pwsh command to smoke test PS is installed and functional.

adityapatwardhan · 2019-03-14T18:37:41Z

@MichaelSimons We have automated tests at the end of the build to install the tool on Linux and Windows. Also run some simple validation to verify version and run a couple of cmdlets.

MichaelSimons · 2019-03-14T18:43:28Z

@adityapatwardhan - yes but we need to verify pwsh works within the scope of these Docker images. Is pwsh on the path, does the shared runtime and pwsh work together, are the required native dependencies present, etc. These are the things that should be validated. One scenario test should be able to cover this.

adityapatwardhan · 2019-03-14T21:01:11Z

@MichaelSimons Should I add similar test to validate PowerShell here:

dotnet-docker/tests/Microsoft.DotNet.Docker.Tests/ImageTests.cs

Lines 148 to 158 in 1e84193

    
           public async Task VerifyAspNetCoreRuntimeImage_AppScenario(ImageData imageData) 
        
           { 
        
               if (imageData.Version.Major == 1) 
        
               { 
        
                   _outputHelper.WriteLine("1.* ASP.NET Core images reside in https://github.com/aspnet/aspnet-docker, skip testing"); 
        
                   return; 
        
               } 
        
               ImageScenarioVerifier verifier = new ImageScenarioVerifier(imageData, _dockerHelper, _outputHelper, isWeb: true); 
        
               await verifier.Execute(); 
        
           }

MichaelSimons · 2019-03-14T21:21:00Z

@adityapatwardhan, yes. Here is an outline of what comes to mind.

        [Theory]
        [MemberData(nameof(GetImageData))]
        public void VerifySdkImage_PowerShell(ImageData imageData)
        {
        {
            if (imageData.Version.Major < 3)
            {
                _outputHelper.WriteLine("PowerShell does not exist in pre-3.0 images, skip testing");
                return;
            }

            // TODO:  Modify DockerHelper.Run method to return output by `return ExecuteWithLogging...`
            string output = _dockerHelper.Run(
                image: _imageData.GetImage(DotNetImageType.SDK, _dockerHelper),
                name: _imageData.GetIdentifier($"pwsh"),
                command: $"pwsh -c xyz"); // TODO: Replace xyz with cmd
            
            // TODO: Validate output against expected?
        }

adityapatwardhan · 2019-03-15T16:56:38Z

@MichaelSimons I have added the test. Please have a look. The CI seems stuck from last night or is the queue pretty long?

MichaelSimons

The size of the PowerShell layer is surprisingly large - 104MB on Linux amd64. What can be done to reduce the size? I see it is carrying around a copy of the nupkg (thanks NuGet 😄). I also see ~37MB of runtimes. I wasn't expecting to see the platforms other than the current.

root@d1818d9294d0:/usr/share/powershell/.store/powershell.linux/6.2.0-rc.1/powershell.linux/6.2.0-rc.1/tools/netcoreapp2.1/any/runtimes# du -sh *
272K    linux
220K    linux-arm
232K    linux-arm64
36K     linux-musl-x64
1.6M    linux-x64
1.4M    osx
3.7M    unix
6.4M    win
4.6M    win-arm
5.0M    win-arm64
4.3M    win-x64
4.2M    win-x86
708K    win10-x64
652K    win10-x86
700K    win7-x64
636K    win7-x86
700K    win8-x64
636K    win8-x86
700K    win81-x64
636K    win81-x86

tests/Microsoft.DotNet.Docker.Tests/ImageTests.cs

3.0/sdk/alpine3.9/amd64/Dockerfile

3.0/sdk/nanoserver-1803/amd64/Dockerfile

3.0/sdk/stretch/arm32v7/Dockerfile

adityapatwardhan · 2019-03-15T17:19:31Z

@MichaelSimons There is definately an error in package creation. The Linux package should not have Windows runtimes and vice versa. We have a 6.2.0 package coming out by end of month. Can this be fixed then? Filed issue PowerShell/PowerShell#9148

MichaelSimons · 2019-03-15T17:23:52Z

@adityapatwardhan - yes the size issue can be addressed at the release. I would however like to have a good idea of what the layer size will be when addressed because the current size is going to be a difficult sell.

adityapatwardhan · 2019-03-15T17:27:11Z

@MichaelSimons I should be able to create a private package fixing the packaging error by Monday. We can have a look then.

adityapatwardhan · 2019-03-15T21:18:26Z

@MichaelSimons I can potentially delete the copy of the nupkg that nuget keeps. That will save somwhere around 20 MBs in the size.

MichaelSimons · 2019-03-15T22:46:34Z

@adityapatwardhan - yes I was thinking the nupkg copy should be cleaned up.

adityapatwardhan · 2019-03-19T21:08:24Z

@MichaelSimons

I built the alpine 3.9 container locally to check the layer sizes. The reduction in the size will be part of our next release expected near end of month.

Layer size before reductions:

IMAGE	CREATED	CREATED BY	SIZE
245a2bf6f1b6	20 hours ago	/bin/sh -c wget -O PowerShell.Linux.$POWERSH…	104MB

After reductions:

IMAGE	CREATED	CREATED BY	SIZE
0a7610b47997	About a minute ago	/bin/sh -c wget -O PowerShell.Linux.$POWERSH…	35.5MB

adityapatwardhan · 2019-03-19T21:11:18Z

@MichaelSimons I have addressed all the review comments. Please have another look. I have also put some layer size information above. It is from a private of the next release of PowerShell v6.2.0, which contains further reduction in size.

MichaelSimons · 2019-03-19T21:11:52Z

3.0/sdk/alpine3.9/amd64/Dockerfile

+    # To reduce image size, remove the copy nupkg that nuget keeps.
+    && find /usr/share/powershell -print | grep -i '.*[.]nupkg$' | xargs rm \
+    # Remove unnecessary runtimes
+    && rm -fr /usr/share/powershell/.store/powershell.linux/$POWERSHELL_VERSION/powershell.linux/$POWERSHELL_VERSION/tools/netcoreapp2.1/any/runtimes/linux-arm \


I thought the plan was to create rid specific packages. These packages aren't rid specific if they contain more than one runtime.

The end result is the same. I just makes your builds easier and faster. Are having rid specific packages a must?

Even for linux-x64, we have some dependencies coming from unix and some from linux.

Assemblies from unix:
System.Drawing.Common.dll
System.Runtime.Caching.dll
System.Data.SqlClient.dll
Microsoft.Management.Infrastructure.dll
Microsoft.Management.Infrastructure.Native.dll
System.Private.ServiceModel.dll

Assemblies from linux:
System.Data.Odbc.dll

The value is the simplicity of the Dockerfile. The cleanliness makes it easy for the end user to see what the image contains. Having logic here to know what runtime folders are required is not desirable. rid specific packages are going to provide the best experience here.

@MichaelSimons I have created RID specific packages now, which will be available with the next release. I will remove the code for deleting runtime folders from the docker files. In my next PR for updating the version, I will also update the package names, is that ok?

PowerShell.Linux.Alpine.$POWERSHELL_VERSION.nupkg
PowerShell.Linux.arm32.$POWERSHELL_VERSION.nupkg
PowerShell.Linux.arm64.$POWERSHELL_VERSIONnupkg
PowerShell.Linux.x64.$POWERSHELL_VERSION.nupkg

PowerShell.Windows.arm32.$ENV:POWERSHELL_VERSION.nupkg
PowerShell.Windows.x64.$ENV:POWERSHELL_VERSION.nupkg

Yes, looks great. We can't merge however until the CLI issue is address as that is a blocker.

Yes of course.

MichaelSimons · 2019-03-28T22:57:35Z

@adityapatwardhan - it looks as thought he underlying .NET CLI issue was resolved with the latest bits. I pulled in the latest 3.0 build. Can you update this PR? FYI - the nanoserver-1709 images were recently removed (#1023).

adityapatwardhan · 2019-03-28T23:06:37Z

I am currently updating the branch for changing version to 6.2.0. We released today. I will make the change for nanoserver 1709.

…32v7

adityapatwardhan · 2019-03-28T23:26:15Z

@MichaelSimons PR updated for PowerShell v6.2.0 and updated to use the rid specific packages. Also rebased to remove nanoserver-1709. Please have a look.

3.0/sdk/nanoserver-1809/arm32v7/Dockerfile

3.0/sdk/nanoserver-1803/amd64/Dockerfile

adityapatwardhan · 2019-03-29T17:49:48Z

@MichaelSimons Updated. Please have another look.

adityapatwardhan · 2019-03-29T18:48:51Z

@MichaelSimons an unrelated test is failing. There were no changes in 2.1-2.2 docker files.

MichaelSimons · 2019-03-29T19:56:11Z

@adityapatwardhan - I am still seeing a failure while building the Windows arm32 image.

C:\Program Files\dotnet\sdk\3.0.100-preview4-010965\NuGet.targets(119,5): error : End of Central Directory record could not be found. [C:\Windows\TEMP\zcrrhos2.z54\restore.csproj]
The tool package could not be restored.‌
Tool 'powershell.windows.arm32' failed to install. This failure may have been caused by:

* You are attempting to install a preview release and did not use the --version option to specify the version.
* A package by this name was found, but it was not a .NET Core tool.
* The required NuGet feed cannot be accessed, perhaps because of an Internet connection problem.
* You mistyped the name of the tool.‌
The command &#x27;cmd &#x2F;S &#x2F;C curl -SL --output PowerShell.Windows.arm32.%POWERSHELL_VERSION%.nupkg https:&#x2F;&#x2F;pwshtool.blob.core.windows.net&#x2F;tool&#x2F;$ENV:POWERSHELL_VERSION&#x2F;PowerShell.Windows.arm32.$ENV:POWERSHELL_VERSION.nupkg     &amp;&amp; mkdir &quot;%ProgramFiles%\powershell&quot;     &amp;&amp; &quot;%ProgramFiles%\dotnet\dotnet&quot; tool install --add-source . --tool-path &quot;%ProgramFiles%\powershell&quot; --version %POWERSHELL_VERSION% PowerShell.Windows.arm32     &amp;&amp; del PowerShell.Windows.arm32.%POWERSHELL_VERSION%.nupkg     &amp;&amp; del &quot;%ProgramFiles%\powershell\.store\powershell.windows.arm32.\%POWERSHELL_VERSION%\powershell.windows.arm32\%POWERSHELL_VERSION%\powershell.windows.arm32.%POWERSHELL_VERSION%.nupkg&quot;&#x27; returned a non-zero code: 1‌

3.0/sdk/nanoserver-1809/arm32v7/Dockerfile

Replaced $ENV:POWERSHELL_VERSION with %POWERSHELL_VERSION% Co-Authored-By: adityapatwardhan <[email protected]>

MichaelSimons

The full test build has now passed! Thanks for your work on this!

FYI - we are in the process of on-boarding Alpine ~~arm32 and~~ arm64 images and will need the corresponding ~~arm32 and~~ arm64 PS nupkgs.

MichaelSimons · 2019-03-29T22:05:00Z

I updated my previous comment - I meant that we are only on-boarding Alpine arm64 images.

MichaelSimons reviewed Mar 5, 2019

View reviewed changes

adityapatwardhan marked this pull request as ready for review March 14, 2019 01:02

adityapatwardhan changed the title ~~WIP - Add PowerShell global to docker-sdk images~~ Add PowerShell global to docker-sdk images Mar 14, 2019

adityapatwardhan force-pushed the PowerShellTool branch from 56191c7 to f6ce78c Compare March 14, 2019 01:11

MichaelSimons reviewed Mar 15, 2019

View reviewed changes

MichaelSimons reviewed Mar 19, 2019

View reviewed changes

adityapatwardhan changed the title ~~Add PowerShell global to docker-sdk images~~ WIP - Add PowerShell global to docker-sdk images Mar 28, 2019

adityapatwardhan added 5 commits March 28, 2019 16:24

Add PowerShell global to docker-sdk images

46dda8d

Fix ENV variables for Windows dockerfile

61e11ab

Bug fixes

c9ee195

Update package URLs

1573176

Revert changes to 2.2 dockerfile

7faa779

adityapatwardhan added 12 commits March 28, 2019 16:24

Add comment about supported runtimes

8498d46

Add PowerShell global tool to nanoserver-1809-arm32v7

e487953

Change DockerHelper.Run to return output

1a57dec

Add tests for PowerShell global tool

8e822d3

Move test location in file

26a2a01

Remove unnecessary quotes and add pwsh to path on nanoserver-1809/arm…

ec16862

…32v7

Update URLs to use environment variable

d9740c6

Update URL and remove nupkg copy

cd601eb

Remove unnecessary runtimes

c4f7217

Remove test changes

582d4a8

Revert removing unnecessary runtimes

5466fc0

Update docker files for v6.2.0

9e99ce9

adityapatwardhan force-pushed the PowerShellTool branch from b9f3fb4 to 9e99ce9 Compare March 28, 2019 23:24

adityapatwardhan changed the title ~~WIP - Add PowerShell global to docker-sdk images~~ Add PowerShell global to docker-sdk images Mar 28, 2019

adityapatwardhan added 2 commits March 28, 2019 16:34

Revert changes to 2.1 nanoserver-1803

151e622

Reset to correct commit

4a8c230

MichaelSimons reviewed Mar 29, 2019

View reviewed changes

3.0/sdk/nanoserver-1809/arm32v7/Dockerfile Outdated Show resolved Hide resolved

3.0/sdk/nanoserver-1803/amd64/Dockerfile Outdated Show resolved Hide resolved

Remove comment and add environment variable

dd8b00b

MichaelSimons reviewed Mar 29, 2019

View reviewed changes

3.0/sdk/nanoserver-1809/arm32v7/Dockerfile Outdated Show resolved Hide resolved

Update 3.0/sdk/nanoserver-1809/arm32v7/Dockerfile

5484a96

Replaced $ENV:POWERSHELL_VERSION with %POWERSHELL_VERSION% Co-Authored-By: adityapatwardhan <[email protected]>

MichaelSimons approved these changes Mar 29, 2019

View reviewed changes

MichaelSimons merged commit 48f122d into dotnet:nightly Mar 29, 2019

adityapatwardhan deleted the PowerShellTool branch March 29, 2019 21:57

richlander mentioned this pull request Apr 18, 2019

.NET Core SDK Docker Images Contain PowerShell Core #1069

Closed

MichaelSimons mentioned this pull request Jan 31, 2020

'Exec format error' when running global tool on ARM32 dotnet/sdk#10167

Closed

Add PowerShell global to docker-sdk images #960

Add PowerShell global to docker-sdk images #960

Uh oh!

Conversation

adityapatwardhan commented Mar 5, 2019 • edited by MichaelSimons Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityapatwardhan commented Mar 5, 2019

Uh oh!

MichaelSimons commented Mar 5, 2019

Uh oh!

Uh oh!

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!

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!

MichaelSimons commented Mar 14, 2019

Uh oh!

adityapatwardhan commented Mar 14, 2019

Uh oh!

MichaelSimons commented Mar 14, 2019

Uh oh!

adityapatwardhan commented Mar 14, 2019

Uh oh!

MichaelSimons commented Mar 14, 2019

Uh oh!

adityapatwardhan commented Mar 15, 2019

Uh oh!

MichaelSimons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adityapatwardhan commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaelSimons commented Mar 15, 2019

Uh oh!

adityapatwardhan commented Mar 15, 2019

Uh oh!

adityapatwardhan commented Mar 15, 2019

Uh oh!

MichaelSimons commented Mar 15, 2019

Uh oh!

adityapatwardhan commented Mar 19, 2019

Uh oh!

adityapatwardhan commented Mar 19, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan commented Mar 5, 2019 •

edited by MichaelSimons

Loading

dnfclas commented Mar 5, 2019 •

edited

Loading

adityapatwardhan commented Mar 15, 2019 •

edited

Loading

MichaelSimons left a comment •

edited

Loading