Creates a single package for Windows #4540

adityapatwardhan · 2017-08-10T00:15:43Z

The updates to build.psm1 and packaging.psm1 create a single package (per bitness), which works on all the Windows OS versions, namely Windows 7 / Windows Server 2008 R2, Windows 8.1 / Windows Server 2012 R2, Windows 10 / Windows Server 2016.

I am in process of testing the packages on the various OS versions, hence please DO NOT MERGE. I will update this PR when testing is complete.

[daxian edited] This PR has been approved by @TravisEz13 and @iSazonov, so I merged it.

Fixes #4315 and #2608

iSazonov · 2017-08-10T05:31:22Z

build.psm1

    # set output options
    $OptionsArguments = @{
        CrossGen=$CrossGen
        Output=$Output
        FullCLR=$FullCLR
-        Runtime=$Runtime
+        Runtime=$RuntimeRID


I'm confused by the reuse of the name Runtime - Now in Start-PSBuild it is used as "Generic" runtime. Here as "minimal level" runtime. I believe we should rename first or second.

I think this is consistent with dotnet's portable runtime concept. unfortunately, they have defined the portable runtimeid for windows to be win7-x86 and win7-x64

@iSazonov we have to use win7-* to make sure the package works on all supported Windows OS versions. It would be weird for cmdline usage to specify win7-x86 or win7-x64, hence I changed it to win-x64 and win-x86. The script changes it to win7-*.

I meant that the variable name "Runtime" accept different values 'win-x64' -> 'win7-x64' , 'win-x86' -> 'win7-x86' - Can we use win7-* with Start-PSBuild?

@adityapatwardhan I think I agree, How about we make the RID you use win7-* and translate the suffix to win-* in the packaging module?

My feedback here was that it's better for transparency's sake that we use the same RIDs as dotnet-cli as opposed to doing black magic over their unfortunate naming. Anyone poking into RID documentation will just be confused if we try to normalize away the 7.

Had the same opinion as @TravisEz13 on the package naming, though. 👍

TravisEz13 · 2017-08-10T22:01:21Z

build.psm1

@@ -616,8 +620,6 @@ function New-PSOptions {
                     "fedora.24-x64",
                     "win7-x86",


I think the RIDs in New-PSOption should be consistent with Start-PSBuild

adityapatwardhan · 2017-08-14T22:01:19Z

@joeyaiello @TravisEz13 @iSazonov Thanks for the feedback. To summarize, keep the value for -Runtime as win7-x86 and win7-x64 and remove other win* values. During packaging change the package name to win-x86 and win-x64. I will update PR soon.

adityapatwardhan · 2017-08-14T23:15:38Z

@TravisEz13 @iSazonov @joeyaiello I have updated the PR. Please have a look.

iSazonov · 2017-08-15T05:25:35Z

build.psm1

@@ -696,19 +690,12 @@ function New-PSOptions {
    }

    if (-not $Runtime) {
-        $RuntimeFromDotNet = dotnet --info | ForEach-Object {
+        $Runtime = dotnet --info | ForEach-Object {
            if ($_ -match "RID") {
                $_ -split "\s+" | Select-Object -Last 1


dotnet --info returns win10-64 for me - we need to add a convertion here.

I have 2 options, first do the conversion here. Other is to make runtime parameter mandatory. I prefer the first one. So if user does not specify -runtime we auto-select win7-x86/x64 on windows. On Linux/Mac, we don't change anything. What do you think?

We have an Issue to make a single package for Linux too and seems the same problem too. So I believe we should use the same logic for both.
If we continue to use dotnet --info we could move the logic in separate function for Windows and Linux.

FYI, Linux will require two packages because of packaging format differences (RPM vs DEB).

@TravisEz13 which of the two options you prefer?

Do the conversion from Win10-x64 -> Win7-x64 in build.psm1

Make -Runtime a mandatory parameter.

daxian-dbw · 2017-08-15T17:08:02Z

build.psm1

@@ -293,8 +293,6 @@ function Start-PSBuild {
                     "fedora.24-x64",
                     "win7-x64",
                     "win7-x86",
-                     "win81-x64",
-                     "win10-x64",


Even though win81-x64 and win10-x64 are removed from this list, when running Start-PSBuild we will still see the publish folder path is netcoreapp2.0\win10-x64\ on a win10 machines and netcoreapp2.0\win81-x64\ on a win81 machine. However, we are not allowed to specify win10-x64 or win81-x64 for the -Runtime parameter here. The user experience seems broken to me.

My opinion is to keep Start-PSBuild and New-PSOptions unchanged, and only make some changes to the packaging scripts. After all, we only care about win7-* RID when doing the universal packaging for windows.

I agree with this suggestion. I will make the updates.

daxian-dbw · 2017-08-15T17:11:53Z

docs/maintainers/releasing.md

@@ -160,20 +160,14 @@ On Windows, the `-Runtime` parameter should be specified for `Start-PSBuild` to
 # Install dependencies
 Start-PSBootstrap -Package

-# Build for v6.0.0-beta.1 release targeting Windows 10 and Server 2016
-Start-PSBuild -Clean -CrossGen -PSModuleRestore -Runtime win10-x64 -Configuration Release -ReleaseTag v6.0.0-beta.1
+# Build for v6.0.0-beta.1 release targeting Windows


On Windows, the -Runtime parameter should be specified for Start-PSBuild to indicate what version of OS the build is targeting.

This sentence needs to be updated to explain that -Runtime needs to be win7-* to build windows universal package.

Build for v6.0.0-beta.1 release targeting Windows

Here we still should mention that targeting Windows 7.

daxian-dbw · 2017-08-15T17:33:50Z

tools/packaging/packaging.psm1

    } else {
        New-PSOptions -Configuration "Release" -WarningAction SilentlyContinue | ForEach-Object { $_.Runtime, $_.Configuration }
    }
+
+


Two new lines here. Maybe one would be sufficient.

daxian-dbw · 2017-08-15T17:36:56Z

docs/maintainers/releasing.md

-# the parameter '-WindowsDownLevel' must be specified.
-Start-PSPackage -Type msi -ReleaseTag v6.0.0-beta.1 <# -WindowsDownLevel win81-x64 #>
-Start-PSPackage -Type zip -ReleaseTag v6.0.0-beta.1 <# -WindowsDownLevel win81-x64 #>
+# Create packages for v6.0.0-beta.1 release targeting Windows.


Maybe this should be updated to Create Windows universal packages for v6.0.0-beta.1 release

dotnet uses the term portable instead of universal.

I believe portable is used for cross platform, this is universal across all windows versions.

iSazonov · 2017-08-16T03:48:49Z

.spelling

@@ -353,6 +353,7 @@ Ronn
 Toolset
 v6
 WiX
+Runtimewin7-x64


iSazonov · 2017-08-16T03:56:08Z

tools/packaging/packaging.psm1

@@ -22,11 +22,6 @@ function Start-PSPackage {
        [ValidateSet("deb", "osxpkg", "rpm", "msi", "zip", "AppImage", "nupkg")]
        [string[]]$Type,


Maybe add
[ValidateScript({!$Environment.IsWindows})]

Cannot add this, as it is valid for Windows and Non-windows.

iSazonov · 2017-08-16T03:58:12Z

tools/packaging/packaging.psm1

-            'win7-x64'  {$NameSuffix = 'win7-win2008r2-x64'}
-            Default {$NameSuffix = $Runtime}
-        }
+        $NameSuffix = $Runtime -replace 'win\d+', 'win'


Maybe move to Line 37?

adityapatwardhan · 2017-08-16T07:36:06Z

@daxian-dbw @TravisEz13 @iSazonov @joeyaiello Please have a look. All feedback is addressed.

iSazonov · 2017-08-16T08:00:49Z

No new ideas yet :-)

TravisEz13 · 2017-08-16T17:00:07Z

tools/packaging/packaging.psm1

@@ -22,11 +22,6 @@ function Start-PSPackage {
        [ValidateSet("deb", "osxpkg", "rpm", "msi", "zip", "AppImage", "nupkg")]
        [string[]]$Type,

-        # Generate windows downlevel package


How do we package x86 now?

My bad, I forgot that we are doing cross compilation so the actual RID will still be *-x64.
@adityapatwardhan and I just talked and agreed that we should bring back the WindowsRutnime parameter here. And that parameter probably should be made mandatory.

How do you make it mandatory? What happens when you are producing a deb, pkg, etc?

Travis is correct, we cannot make it mandatory.

Changes to packaging.psm1 to create a single package to work on all versions of Windows. The runtime used is win7-x86/x64. The package that is created has a name suffix win-x64 or win-x86 to signify it works on any version on Windows.

New commits were pushed

daxian-dbw · 2017-08-22T16:39:40Z

We will have the beta-6 release on this Thursday (8/24), and we don't have enough time to stabilize this change. Travis and I chatted offline and agreed that we should hold off this change till after this release.

adityapatwardhan · 2017-08-22T16:45:56Z

I agree, this should be merged after the release. We do not have the ability to test a package in an automated way. We should get that working first. Get Outlook for Android<https://aka.ms/ghei36>

…

________________________________ From: Dongbo Wang <notifications@github.com> Sent: Tuesday, August 22, 2017 9:39:54 AM To: PowerShell/PowerShell Cc: Aditya Patwardhan; Mention Subject: Re: [PowerShell/PowerShell] Creates a single package for Windows (#4540) We will have the beta-6 release on this Thursday (8/24), and we don't have enough time to stabilize this change. Travis and I chatted offline and agreed that we should hold off this change till after this release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FPowerShell%2FPowerShell%2Fpull%2F4540%23issuecomment-324083453&data=02%7C01%7Cadityap%40microsoft.com%7Cdcfeca2cbbde41e722e908d4e97c6b24%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636390167972135774&sdata=GcVq9IzmSrapBfddMHIUl7wg3KOnWvWTRDdcsL3i0dQ%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAMOhvRZqIxURR3aQjRDHbIGQeoyz9GjNks5sawRZgaJpZM4Oyyz4&data=02%7C01%7Cadityap%40microsoft.com%7Cdcfeca2cbbde41e722e908d4e97c6b24%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636390167972135774&sdata=RhRENrF%2F95PZIs%2FYdAEZFCxEe2pfLuZ09MvHR7c20gc%3D&reserved=0>.

daxian-dbw · 2017-08-24T19:26:11Z

There is some logic in the MSI package that seems to check the Windows C Runtime and VS Studio 2015 C++ redistributables are present and return error message if not (see (54e892a)). It seems this logic is enabled in downlevel MSI package (@mirichmo please correct me if I'm wrong).

I bring this up because this caused an issue that causes Win81 packages fail to install on Server2012R2. I'm not sure if this will have any impact when running win7 MSI packages on Win10/Server2016.

mirichmo · 2017-08-24T22:27:52Z

I filed #4665 to cover the installer issue. The check needs to be more generic to cover the scenario where the dependencies are installed via Windows Update

daxian-dbw · 2017-08-28T23:12:30Z

@TravisEz13 I have merged this PR. Do you think any other scripts need to be changed accordingly? (e.g. powershell/psrelease ??)

TravisEz13 · 2017-08-28T23:24:44Z

@daxian-dbw I'll look at the builds tomorrow and see if everything went correctly.

adityapatwardhan self-assigned this Aug 10, 2017

adityapatwardhan requested review from daxian-dbw, TravisEz13 and iSazonov August 10, 2017 00:15

msftclas added the cla-not-required label Aug 10, 2017

adityapatwardhan force-pushed the OneWinPackage branch from a77f135 to 7495e9a Compare August 10, 2017 00:20

iSazonov reviewed Aug 10, 2017

View reviewed changes

TravisEz13 reviewed Aug 10, 2017

View reviewed changes

adityapatwardhan force-pushed the OneWinPackage branch from 7495e9a to 6b6cc78 Compare August 14, 2017 23:14

iSazonov reviewed Aug 15, 2017

View reviewed changes

daxian-dbw previously requested changes Aug 15, 2017

View reviewed changes

daxian-dbw reviewed Aug 15, 2017

View reviewed changes

adityapatwardhan force-pushed the OneWinPackage branch from 6b6cc78 to bb36145 Compare August 15, 2017 20:12

adityapatwardhan requested a review from JamesWTruher as a code owner August 15, 2017 20:12

adityapatwardhan force-pushed the OneWinPackage branch 2 times, most recently from bdf3d88 to 6258b37 Compare August 16, 2017 00:29

iSazonov reviewed Aug 16, 2017

View reviewed changes

adityapatwardhan force-pushed the OneWinPackage branch from 6258b37 to aa14847 Compare August 16, 2017 07:01

TravisEz13 reviewed Aug 16, 2017

View reviewed changes

TravisEz13 approved these changes Aug 17, 2017

View reviewed changes

adityapatwardhan added 6 commits August 18, 2017 09:48

Create one package for windows

6d412f5

Changes to packaging.psm1 to create a single package to work on all versions of Windows. The runtime used is win7-x86/x64. The package that is created has a name suffix win-x64 or win-x86 to signify it works on any version on Windows.

Fix spellings

0e690d3

Fix typo in releasing.md and remove it from .spellings file

0b5be2b

Addressed CR feedback

fce18d9

Re-add -WindowsRuntime parameter

fae14a7

Fixed releasing.md

a838db6

adityapatwardhan force-pushed the OneWinPackage branch from 9bcce95 to a838db6 Compare August 18, 2017 17:27

adityapatwardhan changed the title ~~[DO NOT MERGE] Creates a single package for Windows~~ Creates a single package for Windows Aug 18, 2017

adityapatwardhan assigned daxian-dbw and unassigned adityapatwardhan Aug 18, 2017

iSazonov approved these changes Aug 21, 2017

View reviewed changes

TravisEz13 approved these changes Aug 21, 2017

View reviewed changes

daxian-dbw merged commit fd047a8 into PowerShell:master Aug 28, 2017

daxian-dbw mentioned this pull request Aug 29, 2017

Need check OS version in installation packages and maybe binary #4315

Closed

adityapatwardhan deleted the OneWinPackage branch September 13, 2017 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creates a single package for Windows #4540

Creates a single package for Windows #4540

		@@ -616,8 +620,6 @@ function New-PSOptions {
		"fedora.24-x64",
		"win7-x86",

		@@ -22,11 +22,6 @@ function Start-PSPackage {
		[ValidateSet("deb", "osxpkg", "rpm", "msi", "zip", "AppImage", "nupkg")]
		[string[]]$Type,

Creates a single package for Windows #4540

Creates a single package for Windows #4540

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment