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

iSazonov · 2017-06-26T17:35:31Z

Reopen #3917
Related #3400

Motivation

We inherited old assembly file versions from Windows PowerShell.
Currently we shouldn't set versions of assemblies statically in AssemblyInfo.cs files, we should set versions of assemblies dynamically by MSBuild as discussed in #3690.

Fix

Remove AssemblyVersion and AssemblyFileVersionAttribute attributes from AssemblyInfo.cs files.
Put all common properties in PowerShell.Common.props including version related properties.
Import PowerShell.Common.props in all csproj files.

Follow-Up Work

In the PR we temporarily hard code assembly versions as "6.0.0-beta.3".
Later we should set a dll version based on GitCommitId.

Additional considerations

In previous PR #3917 we caught some problems and we should fix it here:

package versions now corrupted (1.0.0) (for help): [Continue to use 'VersionPrefix' tag. 'Version' will be inferred from this tag and all other versions, such as AssemblyVersion, FileVesion and PackageVersion and etc, will be inferred from 'Version']
- set PackageVersion?
- set Version?
SMA.dll backward compatibility: [Reviewed by powershell-committee and agreed to bump AssemblyVersion to 6.0.0.0]
- statically set AssemblyVersion 3.0.00 (AssemblyFileVersionAttribute?)
- dynamically set AssemblyVersion by MSBuild
- any tests?

Note

Quoted from @SteveL-MSFT:

@PowerShell/powershell-committee reviewed this and is fine with taking the assembly version to 6.0.0.0 and taking it now to discover customer impact

rkeithhill · 2017-06-26T23:33:14Z

👍 for following DRY principle.

iSazonov · 2017-06-27T14:31:36Z

@daxian-dbw Commit to get PS version from Git is ready. If you want I can add it to the PR. It is only add one new Target.

daxian-dbw · 2017-06-27T15:59:51Z

@iSazonov If the new Target is not huge then add it in and let's review it together.

iSazonov · 2017-06-28T06:36:14Z

The new target is very simple.

Also I tried to build a package by

Start-PSBuild -Clean -CrossGen -Runtime win10-x64 -Configuration Release
Start-PSPackage -Type msi

and got PowerShell-6.0.0-beta.3-win10-win2016-x64.msi.

daxian-dbw · 2017-06-28T17:30:56Z

It's good that you verified the package generation. But it's the NuGet package that I'm worried. Without <VersionPrefix>6.0.0</VersionPrefix>, you will get NuGet packages with 1.0.0 in the name.

iSazonov · 2017-06-28T19:10:39Z

How can I build the NuGet package?

daxian-dbw · 2017-06-28T19:24:36Z

@iSazonov see document here: https://github.com/PowerShell/PowerShell/blob/master/docs/maintainers/releasing.md#nuget-packages

daxian-dbw · 2017-06-28T21:36:44Z

PowerShell.Common.props

+    <PS6AdditionalCommits>$([System.Text.RegularExpressions.Regex]::Match($(PowerShellVersion), $(RegexGitVersion)).Groups[2].Value)</PS6AdditionalCommits>
+    <PS6CommitSHA>$([System.Text.RegularExpressions.Regex]::Match($(PowerShellVersion), $(RegexGitVersion)).Groups[3].Value)</PS6CommitSHA>
+
+    <RegexSymVer>^(\d+).(\d+).(\d+)-(.+)</RegexSymVer>


This regular expression will fail when we finally reach the official v6.0.0 (no -beta.N part anymore).

No plans -rtm?
In any case I can make the part as optional.

daxian-dbw · 2017-06-28T21:39:08Z

PowerShell.Common.props

+      <Output TaskParameter="ConsoleOutput" PropertyName="PowerShellVersion" />
+    </Exec>
+
+  <PropertyGroup>


The whole <PropertyGroup> block should be indented since it's part of the <Target>

We reverted the commit.

daxian-dbw · 2017-06-28T21:42:26Z

PowerShell.Common.props

+    <PS6PatchVersion>$([System.Text.RegularExpressions.Regex]::Match($(PS6BuildVersion), $(RegexSymVer)).Groups[3].Value)</PS6PatchVersion>
+    <PS6LabelVersion>$([System.Text.RegularExpressions.Regex]::Match($(PS6BuildVersion), $(RegexSymVer)).Groups[4].Value)</PS6LabelVersion>
+
+    <PS6FormattedVersion Condition="'$(PS6AdditionalCommits)' == ''">$(PS6BuildVersion) SHA: $(PS6CommitSHA)</PS6FormattedVersion>


I think $(PS6AdditionalCommits) will never be an empty string, because when there is no additional commit, it should be 0.

Good catch!

Will fix in next PR.

daxian-dbw · 2017-06-28T22:10:59Z

PowerShell.Common.props

+    <AssemblyVersion>6.0.0.0</AssemblyVersion>
+
+    <ProductVersion>6.0.0-beta.3</ProductVersion>
+    <InformationalVersion>6.0.0-beta.3</InformationalVersion>


I guess you don't need to hard code them now?

The commit was reverted.

daxian-dbw · 2017-06-28T22:25:54Z

PowerShell.Common.props

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


with --long, even if it's right on a tag, you still will get the long version like v6.0.0-beta.3-0-gxxxxxxxxx. However, when we are right on a tag, we are expecting the GitCommitId to be v6.0.0-beta.3.

daxian-dbw · 2017-06-28T22:35:13Z

PowerShell.Common.props

+
+    <ProductVersion>$(PS6BuildVersion)</ProductVersion>
+    <!-- PackageVersion>$(PS6BuildVersion)</PackageVersion -->
+    <InformationalVersion>$(PS6FormattedVersion)</InformationalVersion>


This is what I get from Microsoft.Win32.Registry.AccessControl.dll:

PS:156> $o | fl ProductVersion, FileVersion, FileName, ProductName ProductVersion : 4.6.25428.01 built by: dlab-DDVSOWINAGE031. Commit Hash: 644b3ba1fec90828fc31353461b7d1a4aac7ea7f FileVersion : 4.6.25428.01 FileName : C:\Users\dongbow\Downloads\Microsoft.Win32.Registry.AccessControl.dll ProductName : Microsoft® .NET Framework

AssemblyInformationalVersion maps to ProductVersion, while AssemblyFileVersion maps to FileVersion, maybe we should do similar thing in powershell assemblies.

daxian-dbw · 2017-06-28T22:42:13Z

src/Microsoft.PowerShell.Commands.Diagnostics/AssemblyInfo.cs

@@ -1,9 +1,6 @@
 using System.Reflection;
 using System.Resources;

-[assembly: AssemblyFileVersionAttribute("3.0.0.0")]
-[assembly: AssemblyVersion("3.0.0.0")]
-
 [assembly: AssemblyCulture("")]
 [assembly: NeutralResourcesLanguage("en-US")]


I don't think [assembly: AssemblyCulture("")] is necessary. We can remove it.
As for [assembly: NeutralResourcesLanguage("en-US")] it can be replaced by <NeutralLanguage>en-US</NeutralLanguage> in csproj, and I think this tag can go in PowerShell.Common.props

daxian-dbw · 2017-06-28T23:01:32Z

src/Microsoft.PowerShell.Commands.Management/AssemblyInfo.cs

@@ -1,8 +1,5 @@
 using System.Reflection;
 using System.Resources;

-[assembly: AssemblyFileVersionAttribute("3.0.0.0")]
-[assembly: AssemblyVersion("3.0.0.0")]
-
 [assembly: AssemblyCulture("")]
 [assembly: NeutralResourcesLanguage("en-US")]


Same here and in other files. So for this one, the AssemlbyInfo.cs file can be deleted probably.

The some AssemlbyInfo.cs files is empty now.
What is the probability that we will not add personal attributes?

daxian-dbw · 2017-06-28T23:24:04Z

Get the tag commit meta info from the target is good, but it raises a problem -- in our release process, we need to be able to build and package powershell core targeting a tag label, say v6.0.0-beta.4, without actually creating the tag. We added a new parameter -ReleaseTag to Start-PSBuild and Start-PSPackage so that currently we can build and package powershell without a tag but still have the correct $PSVersionTable.GitCommitId. (see https://github.com/PowerShell/PowerShell/blob/master/docs/maintainers/releasing.md#release-steps for more information)

However, with this taget change, the FileVersion, ProductVersion won't be correct without the tag being pushed, so it will break our release process.

The reason to create the tag until finishing preparing the release is that issues may come up when or after build/package, and in that case we need check in fixes. If tag is already pushed, then it's super hard to do the fix.

Given this, I suggest to separate the GetPS6VersionFromGit target out from this PR, let's get the common.props part in first.

SteveL-MSFT · 2017-06-28T23:42:30Z

@PowerShell/powershell-committee reviewed this and is fine with taking the assembly version to 6.0.0 and taking it now to discover customer impact

iSazonov · 2017-06-29T03:15:45Z

@SteveL-MSFT Could we add @PowerShell/powershell-committee argumentations to the PR description if in future customers catch any problems with the change?

Also my question is - what is a best practice to ensure assembly backward compatibility? If we follow the best practice, we can recommend the same users to avoid the problem.

@daxian-dbw Well, I revert last commit. We can easy pass a ReleaseTag value by ```dotnet build ... /p:"PS6BuildVersion=$ReleaseTag" in Build.psm1. I'll add this in next PR.

This reverts commit 7d2b520.

daxian-dbw · 2017-06-29T04:33:02Z

I revert last commit. We can easy pass a ReleaseTag value by ```dotnet build ... /p:"PS6BuildVersion=$ReleaseTag" in Build.psm1. I'll add this in next PR.

Thanks @iSazonov! Appreciate your quick action.

daxian-dbw · 2017-06-29T04:44:48Z

PowerShell.Common.props

+    <NeutralLanguage>en-US</NeutralLanguage>
+
+    <ProductVersion>6.0.0-beta.3</ProductVersion>
+    <InformationalVersion>6.0.0-beta.3</InformationalVersion>


I think <ProductVersion> and InformationalVersion are probably not needed in this PR. The target change can bring them back.

daxian-dbw · 2017-06-29T04:46:44Z

PowerShell.Common.props

+
+    <FileVersion>6.0.0.0</FileVersion>
+    <AssemblyVersion>6.0.0.0</AssemblyVersion>
+    <NeutralLanguage>en-US</NeutralLanguage>


I think we need the <VersionPrefix>6.0.0</VersionPrefix> to represent the project version, so that the NuGet package versions are correct.

I agree.
Done.

For clarify. I only found the VersionPrefix in Microsoft.NET.DefaultAssemblyInfo.targets where it define Version property.

Currently, Version composition works as this: If Version is unset, use VersionPrefix (defaults to 1.0.0 if unset) and - if present - append VersionSuffix.
All other version are then defaulted to whatever Version is.

Quoted from StackOverflow

AssemblyVersion and FileVesion are inferred by Version by default. But if they are set, it is considered an override of the default inference.
Not only these 2 versions, ProductVersion and InformationalVersion are also inferred from Version if they are not set. (and ProductVersion is by default the same as InformationVersion)

daxian-dbw · 2017-06-29T04:47:38Z

src/Microsoft.PowerShell.Commands.Diagnostics/AssemblyInfo.cs

@@ -1,9 +1,2 @@
 using System.Reflection;
 using System.Resources;


We probably can remove this file now.

daxian-dbw · 2017-06-29T04:48:26Z

src/Microsoft.PowerShell.Commands.Diagnostics/Microsoft.PowerShell.Commands.Diagnostics.csproj

-    <AssemblyOriginatorKeyFile>../signing/visualstudiopublic.snk</AssemblyOriginatorKeyFile>
-    <SignAssembly>true</SignAssembly>
-    <GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
-    <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>
    <GenerateNeutralResourcesLanguageAttribute>false<
10669
/GenerateNeutralResourcesLanguageAttribute>


<GenerateNeutralResourcesLanguageAttribute> should be removed now.

daxian-dbw · 2017-06-29T04:48:38Z

src/Microsoft.PowerShell.Commands.Management/AssemblyInfo.cs

@@ -1,8 +1,2 @@
 using System.Reflection;
 using System.Resources;


This file can be removed now.

daxian-dbw · 2017-06-29T04:58:00Z

src/Microsoft.PowerShell.Security/Microsoft.PowerShell.Security.csproj

    <NoWarn>$(NoWarn);CS1570</NoWarn>
-    <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
-    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <DelaySign>true</DelaySign>


This tag is now needed now.

not needed now 😄

daxian-dbw · 2017-06-29T04:58:27Z

src/Microsoft.PowerShell.Security/Microsoft.PowerShell.Security.csproj

-    <AssemblyOriginatorKeyFile>../signing/visualstudiopublic.snk</AssemblyOriginatorKeyFile>
-    <SignAssembly>true</SignAssembly>
-    <GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
-    <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>
    <GenerateNeutralResourcesLanguageAttribute>false</GenerateNeutralResourcesLanguageAttribute>


This tag can be removed now.

daxian-dbw · 2017-06-29T04:58:46Z

src/Microsoft.WSMan.Management/AssemblyInfo.cs

@@ -1,4 +1 @@
 using System.Reflection;


This file can be removed now.

daxian-dbw · 2017-06-29T04:59:12Z

src/Microsoft.WSMan.Runtime/AssemblyInfo.cs

@@ -1,4 +1 @@
 using System.Reflection;


This file can be removed now.

daxian-dbw · 2017-06-29T04:59:59Z

src/System.Management.Automation/System.Management.Automation.csproj

-    <AssemblyOriginatorKeyFile>../signing/visualstudiopublic.snk</AssemblyOriginatorKeyFile>
-    <SignAssembly>true</SignAssembly>
-    <GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>
-    <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute>
    <GenerateNeutralResourcesLanguageAttribute>false</GenerateNeutralResourcesLanguageAttribute>


This tag can be removed now.

daxian-dbw

LGTM. @iSazonov Thanks for your hard work on this and your patience as well.

iSazonov · 2017-06-29T17:11:51Z

@daxian-dbw Thanks for review! I am ready to continue the work in new PR.

Update assemblies versions by MSBuild

f178d17

iSazonov requested a review from daxian-dbw June 26, 2017 17:35

msftclas added the cla-already-signed label Jun 26, 2017

daxian-dbw self-assigned this Jun 26, 2017

iSazonov force-pushed the csproj-common-props branch from eee06ac to f178d17 Compare June 26, 2017 17:43

Add GetPS6VersionFromGit target

7d2b520

iSazonov force-pushed the csproj-common-props branch from 2ab1735 to 7d2b520 Compare June 28, 2017 07:30

SteveL-MSFT added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jun 28, 2017

daxian-dbw reviewed Jun 28, 2017

View reviewed changes

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jun 28, 2017

iSazonov added 2 commits June 29, 2017 06:16

Revert "Add GetPS6VersionFromGit target"

9e1ba46

This reverts commit 7d2b520.

Move to '<NeutralLanguage>en-US</NeutralLanguage>'

d059267

daxian-dbw reviewed Jun 29, 2017

View reviewed changes

Remove unneeded files and properties

4f2e4c4

daxian-dbw approved these changes Jun 29, 2017

View reviewed changes

daxian-dbw merged commit a8e3d89 into PowerShell:master Jun 29, 2017

daxian-dbw changed the title ~~Update assemblies versions by MSBuild~~ Refactor MSBuild project files by adding the common property file Jun 29, 2017

iSazonov mentioned this pull request Jul 4, 2017

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

Merged

iSazonov deleted the csproj-common-props branch July 20, 2017 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

		@@ -1,9 +1,2 @@
		using System.Reflection;
		using System.Resources;

		@@ -1,8 +1,2 @@
		using System.Reflection;
		using System.Resources;

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

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

Conversation

Motivation

Fix

Follow-Up Work

Additional considerations

Note

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

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