Set assembly versions by MSBuild #3917

iSazonov · 2017-06-02T11:59:03Z

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.

mirichmo · 2017-06-06T15:59:34Z

...t.Management.Infrastructure.CimCmdlets/Microsoft.Management.Infrastructure.CimCmdlets.csproj

-    <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>


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.

mirichmo · 2017-06-07T00:53:54Z

...t.Management.Infrastructure.CimCmdlets/Microsoft.Management.Infrastructure.CimCmdlets.csproj

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


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.

@powercode Could you please give an example?

I agree with @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.

iSazonov · 2017-06-14T06:43:34Z

@mirichmo @daxian-dbw Could you please continue with the PR?

mirichmo · 2017-06-22T00:18:49Z

PowerShellCommon.props

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


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.

daxian-dbw · 2017-06-23T15:50:57Z

src/System.Management.Automation/AssemblyInfo.cs

 using System.Resources;

-[assembly: AssemblyFileVersionAttribute("3.0.0.0")]
-[assembly: AssemblyVersion("3.0.0.0")]


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.

daxian-dbw · 2017-06-23T15:51:52Z

...t.Management.Infrastructure.CimCmdlets/Microsoft.Management.Infrastructure.CimCmdlets.csproj

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


<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

daxian-dbw · 2017-06-23T15:54:02Z

@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.

daxian-dbw · 2017-06-23T16:07:35Z

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

daxian-dbw · 2017-06-23T16:18:23Z

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

This reverts commit c549925.

daxian-dbw · 2017-06-23T17:26:30Z

This merge has been reverted via #4091. Thanks @mirichmo for the quick turnaround.
@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.

iSazonov · 2017-06-23T17:40:19Z

😕 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?

daxian-dbw · 2017-06-23T17:53:23Z

@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.

iSazonov · 2017-06-23T17:58:03Z

@daxian-dbw Should I restore the branch for you?

iSazonov · 2017-06-23T18:00:59Z

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

Ok!

Update assemblies versions by MSBuild

bbf4d01

iSazonov requested a review from daxian-dbw June 2, 2017 11:59

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

mirichmo self-assigned this Jun 6, 2017

mirichmo suggested changes Jun 7, 2017

View reviewed changes

Move import on first place

0e2a51c

mirichmo reviewed Jun 22, 2017

View reviewed changes

Move to "6.0.0-beta.3"

eee06ac

mirichmo approved these changes Jun 23, 2017

View reviewed changes

mirichmo merged commit c549925 into PowerShell:master Jun 23, 2017

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

daxian-dbw reviewed Jun 23, 2017

View reviewed changes

mirichmo added a commit that referenced this pull request Jun 23, 2017

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

f1ff798

This reverts commit c549925.

mirichmo mentioned this pull request Jun 23, 2017

Revert "Set assembly versions by MSBuild" #4091

Merged

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

iSazonov mentioned this pull request Jun 26, 2017

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

Merged

Set assembly versions by MSBuild #3917

Set assembly versions by MSBuild #3917

Uh oh!

Conversation

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!

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!

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!

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!

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!

daxian-dbw commented Jun 23, 2017

Uh oh!

daxian-dbw commented Jun 23, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

iSazonov commented Jun 23, 2017

Uh oh!

daxian-dbw commented Jun 23, 2017

Uh oh!

iSazonov commented Jun 23, 2017

Uh oh!

iSazonov commented Jun 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

iSazonov commented Jun 2, 2017 •

edited

Loading

iSazonov Jun 7, 2017 •

edited

Loading

iSazonov Jun 7, 2017 •

edited

Loading

daxian-dbw Jun 23, 2017 •

edited

Loading

daxian-dbw commented Jun 23, 2017 •

edited

Loading

daxian-dbw commented Jun 23, 2017 •

edited

Loading