Set assembly versions by MSBuild by iSazonov · Pull Request #3917 · PowerShell/PowerShell · GitHub

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 PowerShellCommon.props including version related properties.
Import PowerShellCommon.props in all csproj files.

Follow-Up Work

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

Where did all of these come from? We need to document the justification for all warnings that we ignore.

I'm surprised, too. We do not receive these warnings with the original csproj files, but they appear after we add "*.props".
I guess this is a "feature" of Sdk="Microsoft.NET.Sdk" templates.
I believe that some of these warnings can be easily corrected in separate PRs.

I reviewed the warnings - all belong to XML comments and we have TONs warnings - It takes a lot of work to fix it.

I'm fine with disabling the xml doc warnings.

All of the GenerateAssembly* entries are missing. I'm assuming it was intentional. Can you please explain?

Do you mean "<GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>" ?

We explicitly had AssemblyInfo attrubutes in AssemblyInfo.cs files and we had to disable auto generating its by "<GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>".
Now we use auto generating and should remove the "false".

My comment applied to all of those flags as well since I referred to them as "GenerateAssembly*". I am not particularly familiar with them and would like to make sure that we are not inadvertently dropping a flag that is necessary for PowerShell.

All these attributes were statically installed in the AssemblyInfo.cs files and that their automatic generation was turned off in csproj files. Now we've removed static attributes and allowed them to be automatically generated. In other words, attributes are present, but their values have changed - now they are always up-to-day. If you build the branch you can find the attributes in auto generated *.AssemblyInfo.cs files in OBJ directory.

I would put the import of the PowerShell.common.props before the property section in the csproj files.

That way you can do per project overrides if there is something that needs to be project specific.

Maybe even have .before.props and .after.props (or with some other naming).

Before contains defaults, and After contains common things you want to have done, but parameterized by each project properties.

Staffan Gustafsson (@powercode) Could you please give an example?

I agree with Staffan Gustafsson (@powercode) - simply move the Import so that it's before any PropertyGroup.

One example - we might move NoWarn into the PowerShellCommon.props, but in some project, we want to more/less warnings disabled.

Thanks for clarify. It's good.
Done.

Mike Richmond (@mirichmo) Dongbo Wang (@daxian-dbw) Could you please continue with the PR?

beta.3 please since we created the release today. Otherwise, it looks fine to me.

Why is 6.0.0-beta.2 hard coded?

The PR only targets assemblies and make multiple cleanups. I didn't want to complicate the PR and hard coded the tag.
In next PR I plan use git to get the tag dynamically. We discussed this previously in #3690.

Change the assembly version from 3.0.0.0 to 6.0.0.0 might break existing assemblies that reference to S.M.A 3.0.0.0. We need to evaluate the implication before making this change.

I think bumping assembly version number needs to be reviewed by powershell committee before the change.

<VersionPrefix> tag is needed to generate NuGet packages with the version like 6.0.0-alpha.3,

Here are the NuGet packages generated:

PS:7> dir -Name
Microsoft.PowerShell.Commands.Diagnostics.1.0.0-beta.4.nupkg
Microsoft.PowerShell.Commands.Diagnostics.1.0.0-beta.4.symbols.nupkg
Microsoft.PowerShell.Commands.Management.1.0.0-beta.4.nupkg
Microsoft.PowerShell.Commands.Management.1.0.0-beta.4.symbols.nupkg
Microsoft.PowerShell.Commands.Utility.1.0.0-beta.4.nupkg
...

They becomes 1.0.0

Mike Richmond (@mirichmo) I don't think this PR is ready to merge yet. Please revert this merge.
@PowerShell/powershell-maintainers I didn't get the time to thoroughly review all PRs related to .csproj changes. Let's hold on merging them.

Mike Richmond (@mirichmo) hold on the revert. Let's see if we can quickly fix the issues.

Mike Richmond (@mirichmo) I don't have a quick fix. I suggest reverting this merge for now and submit another PR later.
Steve Lee (@SteveL-MSFT) what do you think?

This reverts commit c549925.

This merge has been reverted via #4091. Thanks Mike Richmond (@mirichmo) for the quick turnaround.
Ilya (@iSazonov) Sorry for the troubles ☹️ Lesson learned: if you think a PR is risky but don't yet have the time to thoroughly review, leave a comment to point out that you need more time so that everyone is on the same page. I will open a new PR for this change.

😕 It's a very strange side effect. I thought PowerShell Core assembles is not Windows PowerShell assemblies, and we could be free in the change. CoreFX assemblies don't keep old versions too. Maybe a right way is to be based on packages versions?

Ilya (@iSazonov) With netstandard2.0 we expect most of the existing Windows powershell modules to work on powershell core without change. They were built against 3.0.0.0 S.M.A.dll, so we want to make sure nothing will break there.

By the way, I think you have to submit a new PR for this. If I submit it, it will be from my fork and thus you won't be able to make changes. It's very easy to check out a PR with the tool hub. Once you get that tool, you can do things like this to get the changes from this PR:

## It will get the changes from this PR to a local branch named iSazonov-csproj-common-props
hub checkout https://github.com/PowerShell/PowerShell/pull/3917
## Then rebase this branch and push it to your fork
## Then submit a new PR with that remote branch

Again, sorry for the troubles and thanks for your understanding.

Dongbo Wang (@daxian-dbw) Should I restore the branch for you?

I think you have to submit a new PR for this.

Ok!

* Update assemblies versions by MSBuild and create a new PowerShellAssembly.props file to consolidate properties. * Move import on first place * Move to "6.0.0-beta.3"

Update assemblies versions by MSBuild

bbf4d01

Ilya (iSazonov) requested a review from Dongbo Wang (daxian-dbw) June 2, 2017 11:59

Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Jun 2, 2017

Mike Richmond (mirichmo) self-assigned this Jun 6, 2017

Mike Richmond (mirichmo) suggested changes Jun 7, 2017

View reviewed changes

Move import on first place

0e2a51c

Mike Richmond (mirichmo) reviewed Jun 22, 2017

View reviewed changes

Move to "6.0.0-beta.3"

eee06ac

Mike Richmond (mirichmo) approved these changes Jun 23, 2017

View reviewed changes

Mike Richmond (mirichmo) merged commit c549925 into PowerShell:master Jun 23, 2017

Ilya (iSazonov) deleted the csproj-common-props branch June 23, 2017 05:05

Dongbo Wang (daxian-dbw) reviewed Jun 23, 2017

View reviewed changes

Mike Richmond (mirichmo) added a commit that referenced this pull request Jun 23, 2017

Revert "Set assembly versions by MSBuild (#3917 )"

f1ff798

This reverts commit c549925.

Mike Richmond (mirichmo) mentioned this pull request Jun 23, 2017

Revert "Set assembly versions by MSBuild" #4091

Merged

Ilya (iSazonov) restored the csproj-common-props branch June 23, 2017 18:01

Ilya (iSazonov) mentioned this pull request Jun 26, 2017

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

Merged

-                  <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
-                  <DelaySign>true</DelaySign>
+                  <Description>PowerShell Core's Microsoft.Management.Infrastructure.CimCmdlets project</Description>
+                  <NoWarn>$(NoWarn);CS1570;CS1572;CS1573;CS1574;CS1584;CS1587;CS1591</NoWarn>

                   <AssemblyName>Microsoft.Management.Infrastructure.CimCmdlets</AssemblyName>
-                  <AssemblyOriginatorKeyFile>../signing/visualstudiopublic.snk</AssemblyOriginatorKeyFile>
-                  <SignAssembly>true</SignAssembly>
-                  <GenerateAssemblyFileVersionAttribute>false</GenerateAssemblyFileVersionAttribute>

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

               using System.Resources;
-              [assembly: AssemblyFileVersionAttribute("3.0.0.0")]
-              [assembly: AssemblyVersion("3.0.0.0")]

+                <Import Project="..\..\PowerShellCommon.props"/>
                 <PropertyGroup>
-                  <VersionPrefix>6.0.0</VersionPrefix>

Set assembly versions by MSBuild by iSazonov · Pull Request #3917 · PowerShell/PowerShell · GitHub

Uh oh!

Set assembly versions by MSBuild - #3917

Set assembly versions by MSBuild#3917Mike Richmond (mirichmo) merged 3 commits intoPowerShell:masterPowerShell/PowerShell:masterfrom iSazonov:csproj-common-propsiSazonov/PowerShell:csproj-common-propsCopy head branch name to clipboard

Conversation

Ilya (iSazonov) commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Fix

Follow-Up Work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Jun 7, 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!

Ilya (iSazonov) Jun 7, 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!

Ilya (iSazonov) commented Jun 14, 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!

Dongbo Wang (daxian-dbw) Jun 23, 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!

Dongbo Wang (daxian-dbw) commented Jun 23, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 23, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jun 23, 2017

Uh oh!

Dongbo Wang (daxian-dbw) commented Jun 23, 2017

Uh oh!

Ilya (iSazonov) commented Jun 23, 2017

Uh oh!

Ilya (iSazonov) commented Jun 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Set assembly versions by MSBuild#3917
Mike Richmond (mirichmo) merged 3 commits into
PowerShell:masterfrom
iSazonov:csproj-common-props

Ilya (iSazonov) commented Jun 2, 2017 •
edited
Loading

Ilya (iSazonov) Jun 7, 2017 •
edited
Loading

Ilya (iSazonov) Jun 7, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) Jun 23, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) commented Jun 23, 2017 •
edited
Loading

Dongbo Wang (daxian-dbw) commented Jun 23, 2017 •
edited
Loading