$PSVersionTable.GitCommitId based on generated constant #3690

iSazonov · 2017-05-03T12:34:34Z

Generate C# file with a constant containing current version GitCommitId.
Correct a test for GitCommitId.

lzybkr

Maybe in another PR, or this one if you prefer, but it would be nice to also include the commit hash in the version info similar to what the clr does:

@ C:\Program Files\PowerShell\6.0.0-alpha.18
#293 PS> (dir .\System.Collections.dll).VersionInfo.ProductVersion
4.6.24705.01. Commit Hash: 4d1af962ca0fede10beb01d197367c2f90e92c97

lzybkr · 2017-05-03T17:11:54Z

build.psm1

@@ -158,7 +158,30 @@ function Start-PSBuild {
    }

    # save Git description to file for PowerShell to include in PSVersionTable
-    git --git-dir="$PSScriptRoot/.git" describe --dirty --abbrev=60 > "$psscriptroot/powershell.version"
+    $powershellVersion = git --git-dir="$PSScriptRoot/.git" describe --dirty --abbrev=60
+    $powershellVersion > "$PSScriptRoot/powershell.version"


We should stop generating this file.

I use the file in a test. Can we leave the file in repo but exclude it from release and publish?

I don't see that file being used in any tests. i do see the string powershell.version in a test, but it sort of looks like a broken test to me.

Yes, it is a broken test and I added a fix in the PR.

I see. It might be fine if you generated the file alongside other test artifacts, but I would not create it in $PSHome.

iSazonov · 2017-05-04T04:22:40Z

@lzybkr Please clarify - currently git --git-dir=".git" describe --dirty --abbrev=60 return v6.0.0-alpha.18-60-gb09e9b570c757b9869f454276aa708eac307f5b1 where last part is a hash - do you want change formatting? v6.0.0-alpha.18-60 Commit Hash: gb09e9b570c757b9869f454276aa708eac307f5b1

lzybkr · 2017-05-04T17:44:25Z

I'm always a fan of better formatting.

I suggest reading the documentation on describe, and we might consider using --long to always get the hash.

Note that the g is not part of the hash, the format (with --long) is {tag}-{# of commits past tag}-g{hash of head}.

iSazonov · 2017-05-04T18:05:32Z

We would parse the long formatted "git describe" and output in the desired form, ex.:
Version: v6.0.0-alpha.18 Commits: 60 Commit Hash: b09e9b570c757b9869f454276aa708eac307f5b1

lzybkr · 2017-05-04T19:02:23Z

Sounds good, though I would omit Commits if it is 0 (which will be the common case) and maybe use the phrase Additional Commits.

iSazonov · 2017-05-05T14:48:56Z

@lzybkr powershell.version is still used in Install-PowerShellRemoting.ps1.

I see #2671 Should we fix the script here? If yes how can we fix this? Can we use there PSVersionTable.PSVersion?

/cc @mirichmo

iSazonov · 2017-05-10T12:02:36Z

I added commit to fix #3739

daxian-dbw · 2017-05-10T17:10:27Z

@iSazonov thanks for fixing this. The hard-coded alpha string caused me huge effort in beta.1 release.
I have 2 comments:

Could you please resolve the conflicts?
If you search the usage of powershell.version, you will find it appears in following files:

E:\PowerShell\.gitignore
E:\PowerShell\.spelling
E:\PowerShell\build.psm1
E:\PowerShell\build.sh
E:\PowerShell\build.sh
E:\PowerShell\src\powershell-native\Install-PowerShellRemoting.ps1
E:\PowerShell\src\powershell-unix\powershell-unix.csproj
E:\PowerShell\src\powershell-win-core\powershell-win-core.csproj
E:\PowerShell\src\System.Management.Automation\engine\PSVersionInfo.cs
E:\PowerShell\src\System.Management.Automation\engine\PSVersionInfo.cs
E:\PowerShell\test\powershell\Host\PSVersionTable.Tests.ps1

Some of those files are not updated in this PR. I think all appearances of powershell.version need to be reviewed and updated.

iSazonov · 2017-05-10T18:36:42Z

This is the third conflict over the past 24 hours in build.psm1 😄 - I'll resolve tomorrow.

I use powershell.version file in tests. So you might have to leave it in .gitignore

I'm afraid that I won't be able to fix build.sh - can I delegate this anybody?

daxian-dbw · 2017-05-10T19:28:37Z

test/powershell/Host/PSVersionTable.Tests.ps1

@@ -18,7 +18,9 @@ Describe "PSVersionTable" -Tags "CI" {

    }
    It "GitCommitId property should not contain an error" {
-       $PSVersionTable.GitCommitId | Should not match "powershell.version"
+       $powershellVersionFile = Join-Path -Path $PSScriptRoot -ChildPath "assets/powershell.version"


I'm not sure if this test is really useful. The content contained in powershell.version is guaranteed (by the build script) to be the same as in PSVersionInfo.generated.cs. The only possible failure I can see for this test is that we are running tests outside of where powershell was being built and thus powershell.vesrion is not available.

I believe it's more useful to check if both GitCommitId and PSVersion are nonempty and GitCommitId starts with PSVersion, or something like that.

I tried to do the test with direct checking. It seems indirect validation will catch a bug too.

Fixed.

daxian-dbw · 2017-05-10T19:32:39Z

build.psm1

-    # temporary hack to resolve the version issue
-    "v6.0.0-beta.1" > "$psscriptroot/powershell.version"
+    $null = New-Item -Path "$PSScriptRoot\src\System.Management.Automation" -Name "gen" -ItemType Directory -ErrorAction SilentlyContinue
+    Set-Content -Path "$PSScriptRoot\src\System.Management.Automation\gen\PSVersionInfo.generated.cs" -Value $template


This section should be moved to a separate function (better to make the function invisible from outside the module, but not required) so that Start-PSBuild won't keep expanding its size 😄
And also, move the call to that function after we check/do ResGen, so that the System.Management.Automation\gen folder is guaranteed to exist and the New-Item line can be removed.

daxian-dbw · 2017-05-10T19:39:32Z

@iSazonov Sorry that I have to change build.psm1 many times in the past day -- obviously our release process and test infrastructure are still far away from perfect 😦

As for build.sh, writing an equivalent bash script to do the same would be (1) not easy (2) hard to maintain 2 scripts for the same thing in future. One option is to extract this logic and write C# code to do it, like the existing ResGen and TypeCatalogGen. Then we can just call dotnet run from both powershell script and bash script.

I'm not sure how many people are using build.sh honestly, and not really sure if this worth the effort.
@SteveL-MSFT @joeyaiello @lzybkr do we have any data to show how important build.sh is? Can we just remove that support?

New commits were pushed.

iSazonov · 2017-05-11T04:44:25Z

appveyor temporaryly failed - I restart.

iSazonov · 2017-05-11T04:46:37Z

I only fix but don't test 'Install-PowerShellRemoting.ps1'.

lzybkr · 2017-05-11T15:57:53Z

@iSazonov - I ran across this relevant blog post which suggests an assembly attribute that we should also use.

I think we can ignore the part in that blog post about finding the sha without git - we've been using git to do this already with no complaints.

rkeithhill · 2017-05-11T16:06:11Z

This is exactly what we use. Used to use GitVersion but now we're doing our own computation of build metadata:

Get-FileVersionInfo P:\Repositories\BuildResults\Foo_Master_CompleteBuild\5.2.0.105\64BitDevelopment
\Release\Foo.dll | ForEach-Object ProductVersion

5.2.0+105.Branch.master.Sha.31e45082cd1093133b6687dd172daa04b2a10940.Built.20170505T1739

And we do use git to get the SHA1, branch, etc. This is the function we use:

function GetGitBuildMetadata([string]$SemVer, [string]$Revision) {
    $timestamp = Get-Date -Format yyyyMMddTHHmm
    $sha1   = git rev-parse --verify HEAD
    $branch = git rev-parse --abbrev-ref HEAD
    if (!$branch -or ($branch -eq 'HEAD')) {
        if ($env:BUILD_SOURCEBRANCH) {
            $branch = $env:BUILD_SOURCEBRANCH.Trim().Replace("refs/heads/", "").Replace("refs/","")
        }
        else {
            $branch = 'unknown'
        }
    }

    $prereleaseTag = if ($branch -notmatch 'master|release') { '-prerelease' } else { '' }

    $metadata = "${SemVer}${prereleaseTag}+${Revision}.Branch.${branch}.Sha.${sha1}.Built.${timestamp}"
    $metadata
}

daxian-dbw · 2017-05-12T05:23:36Z

Closed the PR by accident 🤕

iSazonov · 2017-05-12T07:24:54Z

@daxian-dbw GetVersion isn't still ported 😕 GitTools/GitVersion#1175

PaulHigin · 2017-05-12T15:16:55Z

@daxian-dbw I don't believe that tampering with version attributes of a file by itself is a security concern, since are many other ways of file tampering for malicious purposes. The way to guard against file tampering is with application white listing (and black listing for deprecated versions) where approved file versions are validated through file signing or catalog signing.

iSazonov · 2017-05-12T18:34:20Z

I took the next step. Now all projects have common properties and assemblies get version from Start-PSBuild.

daxian-dbw · 2017-05-16T18:33:29Z

This PR is getting too complex, and I think we'd better break it down.
We need a conclusion about how to dynamically generate PSVersion and GitCommitId. There are three proposals so far:

Add script in Start-PSBuild to parse git describe string, and generate c# source file to keep the information. Pros: easy to implement. Cons: (1) hard to bring build.sh up-to-date (e.g. hard to write the equivalent bash script, hard to maintain two scripts that do the same thing) (2) make it harder to eventually build the powershell core in Visual Studio as we have another necessary step burnt into the script.
Add a C# project, maybe named PSVersionGen, like ResGen and TypeCatalogGen to do the code generation. Pros: (1) easy to update build.sh, and only one implementation to maintain. (2) it may be easier to eventually build the powershell core in Visual Studio 2017. Cons: a bit harder to implement comparing using the script.
Dynamically generate AssemblyFileVersionAttribute and AssemblyInformationalVersionAttribute via MSBuild, and change code in PSVersionInfo.cs to parse the FileVersion string to get PSVersion and GitCommitId when PSVersionTable is generated for the first time. Pros: clean, no magical code generation at build time, maybe the easiest to move to Visual Studio 2017 in future. Cons: hard to implement -- csproj changes + more robust logic to parse FileVersion in production code. The MSBuild part definitely need extra research and trial/error (build + NuGet package geneation both depend on csproj. A side topic, .NET Core team use different csproj files for build and package, maybe we should go that direction.)

We need to draw to a conclusion first to know what work items would be. Then we break down the work items so that they can be addressed in multiple PRs, instead of one PR with too many changes.

I vote for NO.3 proposal. @lzybkr @iSazonov please share your opinions.

iSazonov · 2017-05-17T10:18:00Z

In brief I agree with NO.3

Perhaps we should create a Meta Issue to split the work on small pieces:
Meta Issue - "Move Start-PSBuild to MSBuild":

Clean up Start-PSBuild from FullClr code and old code.
Generate MSBuild variables with version strings based on Git tags
Move common csproj properties to a common targets file.
Unify AssemblyFileVersionAttribute and AssemblyInformationalVersionAttribute for all assemblies.
Fix PSVersionInfo.cs to parse the FileVersion string or/and use MSBuild variables with version.
Use different csproj files for build and package.
???

Generate C# file with a constant containing current version GitCommitId. Correct test for GitCommitId.

+ Format GitCommitId version string

Fix Install-PowerShellRemoting.ps1 Fix test Fix build.psm1

andyleejordan · 2017-06-30T18:47:56Z

build.psm1

+        if ($($matchVersion[1]) -ne "0") {
+            $Global:formattedVersion += "  Additional commits: $($matchVersion[2])"
+        }
+        $Global:formattedVersion += "  Commit Hash: $($matchVersion[3])"


@JamesWTruher the telemetry you're receiving is from @iSazonov running a build of PowerShell with this (not yet committed to master) change.

JamesWTruher

the new version string format breaks our telemetry. I'm not really sure that the additional data provides more benefit. Is is possible to go back to the original format?

JamesWTruher · 2017-06-30T20:27:20Z

build.psm1

@@ -150,6 +150,42 @@ function Start-PSBuild {
        Stop-Process -Verbose
    }

+    # Generate version constant for $PSVersionTable


This change in formatting breaks our telemetry

iSazonov · 2017-07-01T17:53:45Z

The PR shouldn't be merged - it is only for tracking multiple changes. We split the PR on some light-weight PRs.
There we are trying set assembly attributes by MSBuild based on git version tag and also expose the version to PSVersion table and PowerShell startup banner.
Some Microsoft projects (Roslin, CoreFX/CoreCLR ...) use such version format.

@lzybkr @daxian-dbw Could you please comment? How can we resolve the conflict with telemetry?

daxian-dbw · 2017-09-25T16:49:45Z

The work is covered b #4863. So close this PR.

msftclas added the cla-already-signed label May 3, 2017

iSazonov force-pushed the gitcommitid branch 3 times, most recently from 0c8b5c6 to 5dabec4 Compare May 3, 2017 13:33

lzybkr suggested changes May 3, 2017

View reviewed changes

iSazonov force-pushed the gitcommitid branch from 77f6185 to 63687e3 Compare May 5, 2017 13:17

iSazonov force-pushed the gitcommitid branch from 255c5d9 to b07e87c Compare May 10, 2017 12:00

iSazonov mentioned this pull request May 10, 2017

PSVersion in $PSVersionTable is hard-coded in PSVersionInfo.cs #3739

Closed

iSazonov force-pushed the gitcommitid branch from 2c57162 to 72f404b Compare May 10, 2017 13:14

lzybkr previously approved these changes May 10, 2017

View reviewed changes

lzybkr assigned daxian-dbw May 10, 2017

daxian-dbw reviewed May 10, 2017

View reviewed changes

iSazonov force-pushed the gitcommitid branch from 72f404b to a1823ae Compare May 11, 2017 04:26

iSazonov force-pushed the gitcommitid branch from a1823ae to ff7134c Compare May 11, 2017 04:43

iSazonov force-pushed the gitcommitid branch from ff7134c to 55b438d Compare May 11, 2017 05:32

daxian-dbw closed this May 12, 2017

daxian-dbw reopened this May 12, 2017

msftclas added the cla-already-signed label May 12, 2017

iSazonov force-pushed the gitcommitid branch from 55b438d to 20b3cd1 Compare May 12, 2017 09:14

iSazonov force-pushed the gitcommitid branch from 103ccfb to 31d2ae3 Compare May 12, 2017 16:58

This was referenced May 26, 2017

Move Start-TypeGen logic from Build.psm1 to SDK csproj file #3870

Closed

Add version to PowerShell banner #3916

Merged

iSazonov mentioned this pull request Jun 2, 2017

Set assembly versions by MSBuild #3917

Merged

iSazonov added 5 commits June 23, 2017 11:51

$PSVersionTable.GitCommitId based on generated constant

acc80be

Generate C# file with a constant containing current version GitCommitId. Correct test for GitCommitId.

Remove "powershell.version" file

90e090f

+ Format GitCommitId version string

Generate PSVersion

aaf27a9

Remove 'powershell.version' from all files

d46752b

Fix Install-PowerShellRemoting.ps1 Fix test Fix build.psm1

Update assemblies versions by MSBuild

6ec07b9

iSazonov force-pushed the gitcommitid branch from 31d2ae3 to 6ec07b9 Compare June 23, 2017 08:58

iSazonov mentioned this pull request Jun 26, 2017

Refactor MSBuild project files by adding the common property file #4106

Merged

andyleejordan reviewed Jun 30, 2017

View reviewed changes

andyleejordan requested a review from JamesWTruher June 30, 2017 18:48

JamesWTruher suggested changes Jun 30, 2017

View reviewed changes

iSazonov mentioned this pull request Jul 4, 2017

Refactor MSBuild project files to get PowerShell version from git tag #4182

Merged

daxian-dbw mentioned this pull request Sep 19, 2017

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

Merged

daxian-dbw closed this Sep 25, 2017

iSazonov deleted the gitcommitid branch October 25, 2017 11:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

$PSVersionTable.GitCommitId based on generated constant #3690

$PSVersionTable.GitCommitId based on generated constant #3690

$PSVersionTable.GitCommitId based on generated constant #3690

$PSVersionTable.GitCommitId based on generated constant #3690

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