Refactor MSBuild project files to get PowerShell version from git tag by iSazonov · Pull Request #4182 · PowerShell/PowerShell

iSazonov · 2017-07-04T10:25:21Z

Related #3400
Continue #3690 and #4106.

Extract information about the release tag, number of commits since the tag and the hash of the latest commit within a MSBuild target, and bake that information into version properties of the assemblies appropriately.

New target GetPSCoreVersionFromGit depends on (BeforeTargets) Restore and Nuget steps so now we get packages with the right version.
Added generating a version without suffix (for release tag).
Now ProductVersion is the same as AssemblyInformationalVersion

iSazonov · 2017-07-14T10:11:57Z

@daxian-dbw Could you please review?

daxian-dbw · 2017-07-14T15:12:29Z

@iSazonov Sure, next item on my list :)

mi-hol · 2017-08-18T15:29:33Z

@daxian-dbw 4 weeks pass by quickly :) , hence I wonder if this PR getting any closer to the top of your list?

daxian-dbw · 2017-08-18T22:18:45Z

Sorry for leaving this PR unattended for such a long time ... The conflicts were resolved.

The changes in PowerShell.Common.props is pretty clean and well organized. The comments are good and very helpful -- some minor touch-ups can be made but we can do that when we finalize the changes. I have a few concerns here about the change:

We need to be able to produce a release build without actually pushing the release tag. For example, we need to build for v6.0.0-beta.6 without pushing the git tag v6.0.0-beta.6. This is needed because pushing a release tag should be the last step for a release so that any issues we discovered during the preparation of the release can still be fixed before having the release tag pushed. Currently, this is achieved by passing -ReleaseTag to Start-PSBuild and Start-PSPackage. Take Start-PSBuild for an instance, when -ReleaseTag is specified, it will use that as the release tag instead of turning to git describe. Is it possible to continue allowing this with the GetPSCoreVersionFromGit target? (maybe by passing the to-be-used tag name to a property and in case that property is not empty, use that instead of running git describe)
The target GetPSCoreVersionFromGit seems to be executed for multiple times (once per csproj). Can we make it run only once?
When the target runs git, the output shows on console (see the screenshot below). Is it possible to suppress it while still capturing the output in Exec tasks?

iSazonov · 2017-08-19T21:32:14Z

I believe we can fix this. We can call MSBuild with explicit tag value and don't re-evaluate it in the target by condition.
I guess it is "by design" - MSBuild create new context for every csproj and re-run targets. I'll be happy if I'm wrong. (We can use this as an advantage if we'll want to assign the dll versions more precisely and independently.)
The same. It seems MSBuild can catch output external program only from console. Currently I don't know how suppress the noise.

TravisEz13 · 2017-08-21T17:53:36Z

PowerShell.Common.props

why include the git-dir if you are just getting it from running git in the current directory anyway?

During the release process, we manually specify the version. How do you pick that up?

It is seems a best practice for csproj to use variables everywhere. Currently we can assign the dir externally dotnet build /v:GitInfoBaseDir=....
We can use the same method to assign a version during the release process - @daxian-dbw ask the same and I'll check this and fix if need.

I would suggest assigning the git directory externally. The reason I've added it to all the git commands I've touch is that sometimes git cannot find the directory, which the current solution will not solve.

Anybody can assign GitInfoBaseDir externally - it is supported in the target.

daxian-dbw · 2017-08-22T23:16:59Z

PowerShell.Common.props

+
+    <PropertyGroup>
+      <GitExe>git</GitExe>
+    </PropertyGroup>


Is this property necessary? dotnet/cli use 'git' directly -- see https://github.com/dotnet/cli/blob/45a1e9e56cc47e3973a49f164e98d41442cedebd/build/GitCommitInfo.targets#L3

It seems a best practice for csproj is to use variables everywhere.
Here we can implement the idea - use another git version that is out off PATH. To get this we should do:

<GitExe Condition = " GitExe =='' ">git</GitExe>

What is your thoughts? Should we make our csproj-s power?

I think using git directly makes the xml file a little simpler. I don't think we need to support using a different git that is not in PATH.

daxian-dbw · 2017-08-22T23:17:50Z

PowerShell.Common.props

+      <GitExe>git</GitExe>
+    </PropertyGroup>
+
+    <Exec Command='$(GitExe) rev-parse --show-cdup' ConsoleToMSBuild="true">


Add StandardOutputImportance="Low", and it seems to suppress the log of the output.

Line removed.

daxian-dbw · 2017-08-22T23:24:23Z

PowerShell.Common.props

+
+    <Exec Command='$(GitExe) --git-dir="$(GitInfoBaseDir)" describe --abbrev=60 --long'
+          ConsoleToMSBuild="true"
+          EchoOff="true">


Is EchoOff necessary? I don't see a difference when building with or without it ...

Replaced with StandardOutputImportance="Low"

daxian-dbw · 2017-08-22T23:24:40Z

PowerShell.Common.props

+    </PropertyGroup>
+
+    <Exec Command='$(GitExe) --git-dir="$(GitInfoBaseDir)" describe --abbrev=60 --long'
+          ConsoleToMSBuild="true"


Add StandardOutputImportance="Low", and it seems to suppress the log of the output.

Good catch!

daxian-dbw · 2017-08-22T23:37:42Z

PowerShell.Common.props

+
+    <Exec Command='$(GitExe) rev-parse --show-cdup' ConsoleToMSBuild="true">
+      <Output TaskParameter="ConsoleOutput" PropertyName="GitCDUP" />
+    </Exec>


I think we don't need this step. When running git describe below, just set the WorkingDirectory=$(MSBuildProjectDirectory), then it's guaranteed that the git command executes from the directory where .csproj locates, so --git-dir would be unnecessary.

It seems we can remove WorkingDirectory too because MSBuild make cd to project directory.

so even I'm running dotnet build <path-to-s.m.a.csproj> from a totally different location, it still guarantees that the git command will run in the src\s.m.a folder?

I run Publish-NuGetFeed (it don't change the current directory) from non repo directory and it works. (I can not find link to MSDN docs although I read about this. It seems it was an article about MSBuild targets).
Also we may have an issue here when later we'll implement the building from Visual Studio.

Maybe related https://github.com/dotnet/cli/issues/4473#issuecomment-256502969

Also we may have an issue here when later we'll implement the building from Visual Studio.

What is the issue? Is it that the VS may use a different default working directory?
I feel this 'default working directory' is too implicit and most of us wouldn't know this. I think we should use WorkingDirectory=$(MSBuildProjectDirectory) to make it explicit.

Yes, VS can. Mentioned CLI Issue imply about this.

Will fix.

daxian-dbw · 2017-08-22T23:47:15Z

PowerShell.Common.props

+
+    <PropertyGroup Condition = "'$(ReleaseTag)' != ''">
+      <PSCoreBuildVersion>$(ReleaseTag)</PSCoreBuildVersion>
+      <PSCoreAdditionalCommits>$([System.Text.RegularExpressions.Regex]::Match($(PowerShellVersion), $(RegexGitVersion)).Groups[2].Value)</PSCoreAdditionalCommits>


This seems wrong -- so the PSCoreAdditionalCommits will be set only if ReleaseTag is specified, however, it's used only if ReleaseTag is not specified.

Sorry. Fixed.

daxian-dbw · 2017-08-22T23:53:08Z

PowerShell.Common.props

+    </PropertyGroup>
+
+    <PropertyGroup>
+      <PSCoreBuildVersion Condition = "'$(PSCoreBuildVersion)' == ''">$([System.Text.RegularExpressions.Regex]::Match($(PowerShellVersion), $(RegexGitVersion)).Groups[1].Value)</PSCoreBuildVersion>


It seems to me RegexGitVersion can be moved to this PropertyGroup.

Yes, I tried minimize Condition-s.
Fixed.

daxian-dbw · 2017-08-23T00:14:11Z

build.psm1

+        [Parameter(ParameterSetName = "ReleaseTag")]
+        [ValidatePattern("^v\d+\.\d+\.\d+(-\w+\.\d+)?$")]
+        [ValidateNotNullOrEmpty()]
+        [string]$ReleaseTag


Let's make this parameter mandatory. -VersionSuffix was mandatory.

Are we going to do daily builds? If so we shouldn't make it mandatory.

yes we generate nuget packages from daily build, and you need to change the related code in appveyor.psm1: https://github.com/PowerShell/PowerShell/blob/master/tools/appveyor.psm1#L452

The existing code has predefined suffix for daily build, you need to think how to make it continue to work, or how to alter it according to this PR. /cc @TravisEz13 for input.

I see there we call Start-PSPackage - we need fix it too. And in Start-PSPackage we have another code path for nuget packages - I wonder why we have two code path to generate nuget packages? Because Publish-NuGetFeed was not moved in Packaging.psm1 I thought it is not used on CI but it is used. Please explain the package build process. It seems we need (1) release (2) daily and (3) manual builds - the last only work in the PR. Should we fix all in the PR or can cut off?

The nuget package created in Start-PSPackage is not individual assembly package to be used for development, but a package that contains all powershell core bits -- it's similar to the .zip packages we generated for release and can be installed directly by using PackageManagement module.

daxian-dbw · 2017-08-23T00:17:48Z

build.psm1

+    ## Workaround:
+    ##   dotnet restore /p:VersionSuffix=<suffix> # Bake the suffix into project.assets.json
+    ##   dotnet pack --version-suffix <suffix>
+    ## We update 'project.assets.json' files with new version tag value by 'GetPSCoreVersionFromGit' target.


The comments is outdated now, as we don't use --version-suffix anymore.

Left only my last comment.

daxian-dbw · 2017-08-23T00:18:11Z

build.psm1

+    ##   dotnet pack --version-suffix <suffix>
+    ## We update 'project.assets.json' files with new version tag value by 'GetPSCoreVersionFromGit' target.
+    $TopProject = (New-PSOptions).Top
+    if ($ReleaseTag) {


Let's make -ReleaseTag mandatory. So the if/else is unnecessary now.

daxian-dbw · 2017-08-23T05:46:42Z

build.psm1

        [string]$OutputPath = "$PSScriptRoot/nuget-artifacts",
-        [Parameter(Mandatory=$true)]
-        [string]$VersionSuffix
+        [Parameter(ParameterSetName = "ReleaseTag")]


This parameter attribute seems not necessary.

I added this because we could return [string]$VersionSuffix to temporary fix daily builds.

Fix appveyor.psm1 Add WorkingDirectory=$(MSBuildProjectDirectory)

Dismiss my previous review because of new commits

daxian-dbw · 2017-08-25T16:04:08Z

PowerShell.Common.props

It's still using the default working directory. I think we agreed to explicitly use WorkingDirectory=$(MSBuildProjectDirectory)

Yes. Fixed.

daxian-dbw · 2017-08-28T17:11:34Z

PowerShell.Common.props

The prefixversion, majorversion, minorversion and labelversion are not used anywhere, so maybe we should remove them here.

Yes, currently we don't use but can in future. I added its because we do "strange" 😄 manipulations with version strings in build scripts

I think we only used the individual major, minor and patch numbers when generating MSI package, so it's hard for me to see we will somehow use them in .csproj files. I tend to make our build faster, but probably it won't make much difference. So I will leave this decision up to you. #Closed

We can comment this so that we don't waste time on creating this code if we'll need it in future.

That sounds good to me :)

daxian-dbw · 2017-08-28T17:28:49Z

PowerShell.Common.props

The <ProductVersion> tag seems unnecessary -- the ProductVersion of the generated assembly uses the same as InformationalVersion. I found this when playing with the tag. You can double check, and if you see the same, we should remove this tag here.

InformationalVersion is explicitly defined in Microsoft.NET.GenerateAssemblyInfo.targets from Version. ProductVersion is seems hard coded in AL task. So I explicitly set it here to exclude unpredictable behavior if AL task logic will be changed in any next dotnet update.

I guess it's better to be explicit, so let's keep the <ProductVersion>. But can we move this tag next to <InformationalVersion> instead of having them separated by the comments?

daxian-dbw · 2017-08-28T17:38:14Z

PowerShell.Common.props

Maybe also mention that Version is used as the default value for many other version properties, like AssemblyVersion.

It seems it will be obvious comment. Or you meant we should explicitly enumerate such properties which we use?

I mean if you mention FileVersion in the comment here, then better mention that version properties that use <Version> as the default value. Otherwise, the comment means more like FileVersion is the only reason we explicitly define <version> here.

I thought there is many properties that depended on <Version> but it's not.
Added AssemblyVersion in the comment.

daxian-dbw · 2017-08-28T17:39:02Z

PowerShell.Common.props

I think they can be removed now.

daxian-dbw · 2017-08-28T17:46:33Z

build.psm1

Shall we merge the two /p? Like --output $OutputPath "/p:IncludeSymbols=true;ReleaseTag=$ReleaseTag1"

Fixed. (I can not test this.)

Why can't this be tested? IncludeSymbols=true means symbol packages is demanded as well, so if you use Publish-NuGetFeed -ReleaseTag1 v6.0.0-beta.10, the expected result is both beta.10.nupkg and beta.10.symbols.nupkg are generated.

daxian-dbw · 2017-08-28T17:51:11Z

PowerShell.Common.props

Once we removed some of the unnecessary properties, this debug section need to be cleaned up to remove them as well.

This debug section still needs to be cleaned up, for example, GitCDUP is gone now.

Cleaned up.

daxian-dbw · 2017-08-28T17:52:40Z

build.psm1

Do we need to explicitly run dotnet restore when the release tag is not specified? It's not necessary as we don't need to bake any customized version strings into the build.

If we set new version git tag we should explicitly run dotnet restore before locally build nuget packages.

I see your point. You mean dotnet pack may use the existing artifacts produced from previous dotnet restore run and thus use stale version info. It makes sense. #Closed

daxian-dbw · 2017-08-28T18:50:41Z

tools/appveyor.psm1

Isn't the tag name already has 'v' prefixed?

I don't know - I can not test. I am only trying to follow previous logic.

@TravisEz13 can you comment on this one? What would $env:APPVEYOR_REPO_TAG_NAME be like?
@iSazonov I think the next commit you submit should be marked with '[Feature]', so that we can see how the nupkg generation works in full build.

[Feature] - done.

contains tag name for builds started by tag; otherwise this variable is undefined

I don't think we ever do this, unless AppVeyor counts a push with a tag.

I see. So if an AppVeyor build is triggered by pushing a tag, then this environment variable will be set to be the tag.
So we don't need to prefix with a 'v' here,

daxian-dbw · 2017-08-28T18:51:55Z

tools/appveyor.psm1

-ReleaseTag "/property:ReleaseTag=$preReleaseVersion"

This seems wrong ...

Sorry. Fixed.

daxian-dbw · 2017-08-31T22:19:45Z

The changes to build.psm1 and PowerShell.Common.props are mostly good. I asked @TravisEz13 to review the changes in AppVeyor.psm1 as he is more experienced in the full build.

I did find that the full build run in CI doesn't generate the powershell packages (msi, nupkg and zip) like our daily build (look in the artifacts tab). I don't know how to check the individual assembly nuget packages.

TravisEz13 · 2017-09-01T17:07:55Z

Packaging is broken: See the very end of https://ci.appveyor.com/project/PowerShell/powershell/build/6.0.0-beta.6-4928#L9098

VERBOSE: Zipping C:\projects\powershell\src\powershell-win-core\bin\Release\netcoreapp2.0\win7-x64\publish into C:\projects\powershell\PowerShell_6.0.0-beta.6-19-ga8f22c7f.zip
Cannot validate argument on parameter 'ReleaseTag'. The argument "v6.0.0-daily-beta6-4928" does not match the "^v\d+\.\d+\.\d+(-\w+\.\d+)?$" pattern. Supply an argument that matches "^v\d+\.\d+\.\d+(-\w+\.\d+)?$" and try the command again.
Build success

daxian-dbw · 2017-09-04T19:18:25Z

@TravisEz13 Now that Publish-NugetFeed has -VersionSuffix replaced by -ReleaseTag, the value provided when running Publish-NugetFeed has to comply with the release tag format pattern '^v\d+\.\d+\.\d+(-\w+\.\d+)?$'. So we cannot use the current daily build suffix now. I changed that to the format like this v6.0.0-dailybeta5.3453. Let me know if you have concerns about that.

TravisEz13 · 2017-09-05T19:42:24Z

No concerns with that change

TravisEz13 · 2017-09-05T19:45:54Z

Windows artifacts look good.

daxian-dbw · 2017-09-05T20:01:26Z

Thanks @TravisEz13. Then I will consider this PR to be almost ready. I will update some of the comments, and then it would be good to go.

daxian-dbw · 2017-09-05T23:29:46Z

PowerShell.Common.props

+        because:
+          1. By default the version properties are defined in global 'PropertyGroup' (not target!) in 'Microsoft.NET.DefaultAssemblyInfo.targets'
+          2. Properties in global 'PropertyGroups' are assigned before any targets
+          3. and cannot be conditionally excecuted as targets (after this target).


and cannot be conditionally excecuted as targets (after this target).

First, a typo I didn't notice in my last change: excecuted -> executed.
Second, I have trouble understanding this sentence accurately. Can you please reword this sentence a bit to make it easier to understand?
And one more thing, what does global PropertyGroup mean? Is it different from the PropertyGroup we used in this file?

Typo fixed.
Under "global" I meant that they affect the entire project. Removed.
I tryed to reword but ... - feel free to rewrite.

Comment fixed

daxian-dbw · 2017-09-06T05:36:04Z

@iSazonov Can you please take a look and see if my re-write reflects what you think?

iSazonov · 2017-09-06T06:20:04Z

Yes, thanks! Look very good.

iSazonov · 2017-09-06T06:22:38Z

Get-ComputerInfo test failed in AppVeyor CI.

daxian-dbw · 2017-09-06T19:40:15Z

The test failure is not related to this change. Comparing with the last successful full build CI, the only changes we have are comment changes. So it's OK to merge this PR.

iSazonov · 2017-09-07T03:08:18Z

@daxian-dbw @TravisEz13 Thanks for review and approvement!

Should we continue to improve csproj files? I think it's worth moving a stable logic into them.

daxian-dbw · 2017-09-07T15:39:57Z

@iSazonov Thanks for your hard work on it!
I'm a little hesitating about aggressively moving build logic to .csproj files. I think #3690 deserves higher priority -- now that we have version attributes embedded in the assemblies, we can set up the CommitId and PSVersion in C#.

iSazonov · 2017-09-07T17:58:04Z

@daxian-dbw My question was more about #3690 - I'll continue.

iSazonov · 2017-09-15T14:59:25Z

@daxian-dbw What is your thoughts - should we set up the CommitId and PSVersion based on assembly version attributes or generate strong typed constant?
The first method is simpler and more approved but not protected like powershell.version file.
Second is more strong but can result in frequent recompilation of SMA.

daxian-dbw · 2017-09-15T15:49:05Z

I vote for getting the needed info from the attributes 😄

iSazonov requested a review from daxian-dbw July 4, 2017 10:25

msftclas added the cla-already-signed label Jul 4, 2017

daxian-dbw self-assigned this Jul 5, 2017

daxian-dbw force-pushed the cproj-psversion-from-git branch from cab3688 to f27883d Compare August 18, 2017 21:24

iSazonov requested review from TravisEz13 and adityapatwardhan as code owners August 18, 2017 21:24

daxian-dbw removed the request for review from adityapatwardhan August 18, 2017 21:31

iSazonov force-pushed the cproj-psversion-from-git branch from f27883d to e4afa43 Compare August 21, 2017 11:12

iSazonov mentioned this pull request Aug 21, 2017

Minor formatting commits for Build.psm1 and csproj files #4630

Merged

TravisEz13 reviewed Aug 21, 2017

View reviewed changes

Get PowerShell version from git tag

b74c493

iSazonov force-pushed the cproj-psversion-from-git branch from e4afa43 to b74c493 Compare August 22, 2017 09:11

Add support ReleaseTag in Start-PSBuild and Publish-NuGetFeed

840c18a

daxian-dbw previously requested changes Aug 22, 2017

View reviewed changes

daxian-dbw reviewed Aug 23, 2017

View reviewed changes

Remove unneeded comments and codes.

890e435

Fix appveyor.psm1 Add WorkingDirectory=$(MSBuildProjectDirectory)

daxian-dbw reviewed Aug 28, 2017

View reviewed changes

Minor fixes

39e04f1

iSazonov force-pushed the cproj-psversion-from-git branch from 06e2998 to 39e04f1 Compare August 29, 2017 08:11

daxian-dbw added the Review - Waiting on Author label Aug 31, 2017

iSazonov mentioned this pull request Aug 31, 2017

'PackageVersion' is reset to '1.0.0' dotnet/sdk#1557

Closed

iSazonov force-pushed the cproj-psversion-from-git branch from 0835d32 to 92b1ec4 Compare August 31, 2017 08:56

[Feature] Minor cleanups

76a7e6f

iSazonov force-pushed the cproj-psversion-from-git branch from 92b1ec4 to 76a7e6f Compare August 31, 2017 10:41

iSazonov and others added 2 commits September 4, 2017 08:19

Fix previewLabel in AppVeyor.psm1

44101fc

[Feature] Fix nuget package publish in daily build

953fb6b

daxian-dbw removed the Review - Waiting on Author label Sep 5, 2017

Update some comments in 'PowerShell.Common.props'

048ab75

daxian-dbw previously requested changes Sep 5, 2017

View reviewed changes

iSazonov and others added 2 commits September 6, 2017 06:59

Fix comment

5bae6ba

[Feature] Reword the comments

fb867bd

daxian-dbw approved these changes Sep 6, 2017

View reviewed changes

Remove an unnecessary empty line

0a09b2d

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

iSazonov deleted the cproj-psversion-from-git branch September 7, 2017 05:02

Conversation

iSazonov commented Jul 4, 2017 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jul 14, 2017

Uh oh!

daxian-dbw commented Jul 14, 2017

Uh oh!

mi-hol commented Aug 18, 2017

Uh oh!

daxian-dbw commented Aug 18, 2017

Uh oh!

iSazonov commented Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Aug 21, 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!

TravisEz13 Aug 21, 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!

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!

daxian-dbw Aug 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!

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 Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

iSazonov commented Jul 4, 2017 •

edited by daxian-dbw

Loading

iSazonov Aug 21, 2017 •

edited

Loading

TravisEz13 Aug 21, 2017 •

edited

Loading

daxian-dbw Aug 22, 2017 •

edited

Loading

iSazonov Aug 23, 2017 •

edited

Loading

iSazonov Aug 23, 2017 •

edited

Loading

daxian-dbw Aug 31, 2017 •

edited

Loading