Get '$PSVersionTable.PSVersion' from ProductVersion attribute #4863

iSazonov · 2017-09-19T09:18:49Z

Resolve #3739.
Resolve #3676.

Fix

Get a formatted product version from ProductVersion attribute.
Assign PSVersionInfo.PSVersion from a formatted product version as '6.0.0-Beta.7'
Assign PSVersionInfo.GitCommitId from a formatted product version as 'v6.0.0-beta.7-37-g0afa5250a...'
Remove dependence on 'powershell.version'.
Improve tests.

daxian-dbw · 2017-09-19T15:57:56Z

Thanks @iSazonov! Can we close #3690 now?

daxian-dbw · 2017-09-19T17:51:18Z

src/System.Management.Automation/engine/PSVersionInfo.cs

I think GetTypeInfo() is not necessary now.
Another way to get the information version string is:

var infoVersion = typeof(PSVersionInfo).Assembly.GetCustomAttributes(typeof(AssemblyInformationalVersionAttribute), inherit: false) infoVersion.InformationalVersion

GetTypeInfo() can be removed.

daxian-dbw · 2017-09-19T17:54:12Z

src/System.Management.Automation/engine/PSVersionInfo.cs

The resulting s_psV6Version will look like this: 6.0.0-beta.7 SHA: d2fe36fb4137c2a7d6dbc0df22245d14256f3a74.
I don't think the SHA or Commit info should be put in $PSVersionTable.PSVersion. I'm fine we change it from 6.0.0-beta to 6.0.0-beta.7 though. How about split the information version string to get the 6.0.0-beta.7 part and create the SemanticVersion object with that?

I think we could try to put the information in another attribute (not ProductVersion) to exclude parsing.

Yes, that could be an option if we can find a suitable attribute to store this info.
Splitting the information version string is fine by me too. And plus, we need to split that string anyway to reconstruct the value for GitCommitId.

I don't found a suitable attribute - we are limited by MSBuild's AL task.
Make parsing.
It seems it slightly changes the GitCommitId format for telemetry - we removed final "-dirty".

iSazonov · 2017-09-20T15:15:39Z

I rebuilt GitCommitId from formatted one. It looks not very good solution 😕
Currently $PSVersionTable show formatted GitCommitId and internally we use a rebuilt GitCommitId (raw).
We can use raw GitCommitId in $PSVersionTable but it seems we want user friendly formatted GitCommitId there.
Or maybe refactor and add explicitly FormattedGitCommitId and RawGitCommitId (or GitCommitIdFormatted and GitCommitIdRaw) in $PSVersionTable?

daxian-dbw · 2017-09-20T16:54:33Z

src/System.Management.Automation/engine/PSVersionInfo.cs

We shouldn't change the existing entries in $PSVersionTable. We should reconstruct the GitCommitId in the exact same format as before. The s_rawGitCommitId shouldn't be needed.

daxian-dbw · 2017-09-20T17:00:09Z

src/System.Management.Automation/engine/PSVersionInfo.cs

There is a g before the SHA value when got from git describe:
v6.0.0-beta.7-30-gf1ec922fd93cf833f32d98fcf337a51f2919b6cf

And also, when Commit: is not present, it indicates the build was from a release tag, so SHA shouldn't be put in the GitCommitId in that case, for example, it would be v6.0.0-beta.7.

daxian-dbw · 2017-09-20T17:02:24Z

src/System.Management.Automation/engine/PSVersionInfo.cs

We shouldn't use the formatted string from the attribute directly. CommitId should be in the exact same format as before.

daxian-dbw · 2017-09-20T17:15:34Z

src/System.Management.Automation/engine/PSVersionInfo.cs

The format could also be 6.0.0-beta.7 SHA: 52c6bfe1eae24dbaa1a162bffb3754ba3fdc1f4c without the Commit count. It's better to mention that too.
How about:

// Get ProductVersion of the assembly. The product version string can be one of the following format examples: // when powershell is built from a commit -- '6.0.0-beta.7 Commits: 29 SHA: 52c6b...' // when powershell is built from a release tag -- '6.0.0-beta.7 SHA: f1ec9...'

daxian-dbw · 2017-09-20T17:17:31Z

PowerShell.Common.props

It's better to be more specific about what depends on this formatted version string here. How about

Caution! PSVersion and GitCommitId from PSVersionInfo.cs depend on the format of this version string.

daxian-dbw · 2017-09-20T17:21:11Z

build.psm1

Need to remove powershell.version from .gitignore as well.

daxian-dbw · 2017-09-21T18:19:59Z

src/System.Management.Automation/engine/PSVersionInfo.cs

GetTypeInfo() can be removed.

daxian-dbw · 2017-09-21T18:25:29Z

src/System.Management.Automation/engine/PSVersionInfo.cs

Please add comments about the two formats of the product version strings. For example:

// Get ProductVersion of the assembly. The product version string can be one of the following format examples: // when powershell is built from a commit -- '6.0.0-beta.7 Commits: 29 SHA: 52c6b...' // when powershell is built from a release tag -- '6.0.0-beta.7 SHA: f1ec9...'

// Convert to '6.0.0-beta.7-29-g52c6bfe1eae24dbaa1a162bffb3754ba3fdc1f4c'.

This is not accurate. When building from a release tag, it should be converted to v6.0.0-beta.7.

Sorry - I lost the commit.

Fixed.

daxian-dbw · 2017-09-21T18:26:49Z

src/System.Management.Automation/engine/PSVersionInfo.cs

This won't work when it's built from a release tag. When Commits: is not present, SHA shouldn't be included in the reconstructed GitCommitId.

Please clarify. It seems works as expected:

> "6.0.0-beta.7".Replace(" Commits: ", "-").Replace(" SHA: ", "-g") 6.0.0-beta.7

If it is ok I'd remove rawGitCommitId variable.

fileVersionInfo.ProductVersion will never just be 6.0.0-beta.7. In case of building with a release tag, it will be like this (for beta.7):

PS> $s.VersionInfo.ProductVersion 6.0.0-beta.7 SHA: d2fe36fb4137c2a7d6dbc0df22245d14256f3a74

Please clarify.

I see v6.0.0-beta.7 in Beta.7 distributive powershell.version file. In Build.psm1 I see the pattern for ReleaseTag "^v\d+.\d+.\d+(-\w+.\d+)?$". If I run PowerShell I see:

PowerShell v6.0.0-beta.7 Copyright (C) Microsoft Corporation. All rights reserved. PS C:\Program Files\PowerShell\6.0.0-beta.7>

-- no SHA have we.

"6.0.0-beta.7 SHA: d2fe36f".Replace(" Commits: ", "-").Replace(" SHA: ", "-g")
Return: v6.0.0-beta.7-gd2fe36fb4137c2a7d6dbc0df22245d14256f3a74
-- the code works as expected.

Here is the $PSVersionTable for beta.7. Note that GitCommitId is v6.0.0-beta.7.

PS:1> $PSVersionTable Name Value ---- ----- PSVersion 6.0.0-beta PSEdition Core GitCommitId v6.0.0-beta.7 OS Microsoft Windows 10.0.15063 Platform Win32NT PSCompatibleVersions {1.0, 2.0, 3.0, 4.0...} PSRemotingProtocolVersion 2.3 SerializationVersion 1.1.0.1 WSManStackVersion 3.0

The ProductVersion of System.Management.Automation.dll is

PS:6> $s = Get-Item C:\Arena\pscore\System.Management.Automation.dll PS:7> $s.VersionInfo.ProductVersion 6.0.0-beta.7 SHA: d2fe36fb4137c2a7d6dbc0df22245d14256f3a74

And thus based on your code, GitCommitId will become:

PS> "v" + $s.VersionInfo.ProductVersion.Replace(" Commits: ", "-").Replace(" SHA: ", "-g") v6.0.0-beta.7-gd2fe36fb4137c2a7d6dbc0df22245d14256f3a74

and it's obviously not v6.0.0-beta.7.

When building from a release tag, the GitCommitId should be exactly the same as of today when using powershell.version file.

Yes, the final version should just be v6.0.0. PSVersion="6.0.0-beta.N" is correct. Soon we should have 6.0.0-ReleaseCandidate and if needed, 6.0.0-ReleaseCandidate.2

I'm not a fan of completely removing the GitCommitId field. I don't mind if it has another name, but it's very useful for our telemetry and understanding how the product is being used and in what way.

I see the GitCommitId and PSVersion being somewhat related, where GitCommitId is the most specific way to refer to the version of powershell being invoked

@daxian-dbw
Changing PSVersion to be a proper representation of any possible semantic version shouldn't break our telemetry. IIRC our telemetry is based on the GitCommitID (@SteveL-MSFT will know for sure). In any event, If it (our telemetry) doesn't support semantic versions, I would argue that we need to address that, as we should handle it.

Thanks @SteveL-MSFT and @JamesWTruher, appreciate your inputs.
@iSazonov Then let's go with PSVersion= "6.0.0-beta.n

Yes, I believe our telemetry is using GitCommitID and not PSVersion

daxian-dbw · 2017-09-21T18:27:37Z

src/System.Management.Automation/engine/PSVersionInfo.cs

fileVersionInfo.ProductVersion is used 3 times here, so I suggest to hold the value in a local variable instead.

daxian-dbw · 2017-09-21T18:29:28Z

src/System.Management.Automation/engine/remoting/commands/CustomShellCommands.cs

It seems versionTable is not used anymore. This line can be removed.

Removed.

I see TODO comment - currently we can detect that we use 'Release Tag' and automate the name selection. Make sense?

I don't really understand the intention of this comment. Maybe we should open an issue to track it (fix or remove).

daxian-dbw · 2017-09-21T18:29:42Z

src/System.Management.Automation/engine/remoting/commands/CustomShellCommands.cs

daxian-dbw · 2017-09-22T21:05:01Z

test/powershell/Host/PSVersionTable.Tests.ps1

Two things:

When it's building from a release tag, $PSVersionTable.GitCommitId will be like v6.0.0-beta.7 and this match operation will fail.

It's better to use ... | Should Match ...

This reverts commit 8e4b02c00c298e26fc6cac8015a543fb080dc89b.

This reverts commit d3d93f1197d8b00747b2ae654456666cae06836e.

This reverts commit 089e22b9146c89ee9430dcf21594e80c865f3eaf.

daxian-dbw · 2017-09-27T19:44:56Z

test/powershell/Host/PSVersionTable.Tests.ps1

+    BeforeAll {
+        $powershellProcess=Get-Process -id $pid
+        $rootPath = Split-Path -Path $powershellProcess.path -Parent
+        $sma = Get-Item (Join-Path $rootPath "System.Management.Automation.dll")


Why not just use Join-Path $PSHome System.Management.Automation.dll?

Join-Path returns string. We need FileInfo to get VersionInfo.

I know, I meant using that to replace the existing Join-Path ... so you don't need the get-process and split-path.

😄 Clear.

daxian-dbw · 2017-09-27T19:51:26Z

test/powershell/Host/PSVersionTable.Tests.ps1

+    It "GitCommitId property" {
+       $PSVersionTable.GitCommitId | Should BeOfType "System.String"
+       $PSVersionTable.GitCommitId | Should Match $expectedGitCommitIdPattern
+       $PSVersionTable.GitCommitId | Should Not Match $unexpectectGitCommitIdPattern


This not match test case is not necessary IMO since you are doing a BeExactly comparison below.

We do two checks - 1. for string values (line below). 2. for regex patterns (this line).
I split regex patterns to check GitCommitId and ReleaseTag cases and keep the patterns simple. The double "Should" is needed because ReleaseTag pattern can pass GitCommitId.

I see the point. It's to guarantee that the reconstructed rawGitCommitId to be in the expected regx pattern. #Close

daxian-dbw · 2017-09-27T19:51:49Z

@iSazonov Thanks for the update. It's almost there :)

iSazonov · 2017-09-28T05:07:10Z

@daxian-dbw The history is 4 months long - sorry that sometimes I lost the essence of the changes.

daxian-dbw · 2017-09-28T06:08:44Z

@iSazonov finally we get it ready 😄 Thanks!

iSazonov · 2017-09-28T08:39:16Z

@daxian-dbw Thank you for review and your patience!

iSazonov assigned daxian-dbw Sep 19, 2017

iSazonov requested a review from daxian-dbw September 19, 2017 09:18

iSazonov requested review from BrucePay, anmenaga, lzybkr and vors as code owners September 19, 2017 09:18

msftclas added the cla-already-signed label Sep 19, 2017

iSazonov force-pushed the GitCommitId branch 2 times, most recently from 52c6bfe to b87c639 Compare September 19, 2017 13:33

iSazonov changed the title ~~Get '$PSVersionTable.PSVersion' and '$PSVersionTable.GitCommitId' from ProductVersion attribute~~ Get '$PSVersionTable.PSVersion' from ProductVersion attribute Sep 19, 2017

daxian-dbw requested changes Sep 19, 2017

View reviewed changes

iSazonov requested review from PaulHigin, TravisEz13, adityapatwardhan, dantraMSFT and mirichmo as code owners September 20, 2017 10:41

iSazonov force-pushed the GitCommitId branch 4 times, most recently from 77e4304 to 0ad4d3d Compare September 20, 2017 14:22

daxian-dbw requested changes Sep 20, 2017

View reviewed changes

daxian-dbw requested changes Sep 21, 2017

View reviewed changes

daxian-dbw requested changes Sep 22, 2017

View reviewed changes

vors removed their request for review September 25, 2017 00:38

daxian-dbw mentioned this pull request Sep 25, 2017

$PSVersionTable.GitCommitId based on generated constant #3690

Closed

TravisEz13 removed their request for review September 25, 2017 17:51

msftgits removed the cla-already-signed label Sep 26, 2017

iSazonov force-pushed the GitCommitId branch from 5eaa425 to 35fdb4a Compare September 27, 2017 06:45

iSazonov added 16 commits September 27, 2017 09:46

Get 'PSVersion' from ProductVersion attribute

7e0e6ad

Replace versionTable["GitCommitId"] with PSVersionInfo.GitCommitId

c3bc0b8

Split GitCommitId on formatted and non-formatted

16fe5da

Remove 'powershell.version' file

04ef450

Fix Enable-PSRemoting

8c29037

Fix test

38b9827

Add test

b8a83fa

Revert "Fix test"

0a15f55

This reverts commit 8e4b02c00c298e26fc6cac8015a543fb080dc89b.

Revert "Fix Enable-PSRemoting"

ce1ed51

This reverts commit d3d93f1197d8b00747b2ae654456666cae06836e.

Revert "Split GitCommitId on formatted and non-formatted"

5ec4f94

This reverts commit 089e22b9146c89ee9430dcf21594e80c865f3eaf.

Improve tests

b57f457

Reconstruct raw GitCommitId

b85ca57

Minor fixes

fbc35bd

Remove unused code

9f93ebe

Correct comment and add local variable

9f740fa

Fix GitCommitId, comments and tests

ad92e3c

iSazonov force-pushed the GitCommitId branch from 35fdb4a to ad92e3c Compare September 27, 2017 06:50

Minor fixes to the comments

f469c0e

daxian-dbw requested changes Sep 27, 2017

View reviewed changes

Use $PSHome in tests

2dbf568

daxian-dbw approved these changes Sep 28, 2017

View reviewed changes

daxian-dbw merged commit 6e77537 into PowerShell:master Sep 28, 2017

iSazonov deleted the GitCommitId branch September 28, 2017 17:03

Get '$PSVersionTable.PSVersion' from ProductVersion attribute #4863

Get '$PSVersionTable.PSVersion' from ProductVersion attribute #4863

Uh oh!

Conversation

iSazonov commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

Uh oh!

daxian-dbw commented Sep 19, 2017

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!

daxian-dbw Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 20, 2017

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!

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!

iSazonov Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

JamesWTruher Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 19, 2017 •

edited

Loading

daxian-dbw Sep 19, 2017 •

edited

Loading

iSazonov Sep 20, 2017 •

edited

Loading

iSazonov Sep 22, 2017 •

edited

Loading

JamesWTruher Sep 26, 2017 •

edited

Loading

iSazonov Sep 28, 2017 •

edited

Loading