Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 6, 2017

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Jul 4, 2017

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 iSazonov requested a review from daxian-dbw July 4, 2017 10:25
@daxian-dbw daxian-dbw self-assigned this Jul 5, 2017
@iSazonov
Copy link
Collaborator Author

@daxian-dbw Could you please review?

@daxian-dbw
Copy link
Member

@iSazonov Sure, next item on my list :)

@mi-hol
Copy link

mi-hol commented Aug 18, 2017

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

@daxian-dbw daxian-dbw force-pushed the cproj-psversion-from-git branch from cab3688 to f27883d Compare August 18, 2017 21:24
@daxian-dbw daxian-dbw removed the request for review from adityapatwardhan August 18, 2017 21:31
@daxian-dbw
Copy link
Member

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:

  1. 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)

  2. The target GetPSCoreVersionFromGit seems to be executed for multiple times (once per csproj). Can we make it run only once?

  3. 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?

image

@iSazonov
Copy link
Collaborator Author

  1. 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.
  2. 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.)
  3. The same. It seems MSBuild can catch output external program only from console. Currently I don't know how suppress the noise.

<GitInfoBaseDir Condition="'$(GitInfoBaseDir)' == ''">$(MSBuildProjectDirectory)/$(GitCDUP)/.git</GitInfoBaseDir>
</PropertyGroup>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@iSazonov iSazonov Aug 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@TravisEz13 TravisEz13 Aug 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

<PropertyGroup>
<GitExe>git</GitExe>
</PropertyGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

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

<Exec Command='$(GitExe) rev-parse --show-cdup' ConsoleToMSBuild="true">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line removed.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with StandardOutputImportance="Low"

</PropertyGroup>

<Exec Command='$(GitExe) --git-dir="$(GitInfoBaseDir)" describe --abbrev=60 --long'
ConsoleToMSBuild="true"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!


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

@daxian-dbw daxian-dbw Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Will fix.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Fixed.

</PropertyGroup>

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I tried minimize Condition-s.
Fixed.

build.psm1 Outdated
[Parameter(ParameterSetName = "ReleaseTag")]
[ValidatePattern("^v\d+\.\d+\.\d+(-\w+\.\d+)?$")]
[ValidateNotNullOrEmpty()]
[string]$ReleaseTag
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@iSazonov iSazonov Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

build.psm1 Outdated
## 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@iSazonov iSazonov Aug 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left only my last comment.

build.psm1 Outdated
## 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

build.psm1 Outdated
@@ -1320,20 +1325,26 @@ function Publish-NuGetFeed
{
param(
[string]$OutputPath = "$PSScriptRoot/nuget-artifacts",
[Parameter(Mandatory=$true)]
[string]$VersionSuffix
[Parameter(ParameterSetName = "ReleaseTag")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter attribute seems not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix  appveyor.psm1
Add WorkingDirectory=$(MSBuildProjectDirectory)
@daxian-dbw daxian-dbw dismissed their stale review August 25, 2017 16:01

Dismiss my previous review because of new commits

<Exec Command='git describe --abbrev=60 --long'
ConsoleToMSBuild="true"
StandardOutputImportance="Low">
<Output TaskParameter="ConsoleOutput" PropertyName="PowerShellVersion" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Fixed.

<PSCoreMajorVersion>$([System.Text.RegularExpressions.Regex]::Match($(PSCoreBuildVersion), $(RegexSymVer)).Groups[2].Value)</PSCoreMajorVersion>
<PSCoreMinorVersion>$([System.Text.RegularExpressions.Regex]::Match($(PSCoreBuildVersion), $(RegexSymVer)).Groups[3].Value)</PSCoreMinorVersion>
<PSCorePatchVersion>$([System.Text.RegularExpressions.Regex]::Match($(PSCoreBuildVersion), $(RegexSymVer)).Groups[4].Value)</PSCorePatchVersion>
<PSCoreLabelVersion>$([System.Text.RegularExpressions.Regex]::Match($(PSCoreBuildVersion), $(RegexSymVer)).Groups[5].Value)</PSCoreLabelVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@daxian-dbw daxian-dbw Aug 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me :)

<PSCoreFormattedVersion Condition = "'$(ReleaseTag)' != '' or '$(PSCoreAdditionalCommits)' == '0'">$(PSCoreBuildVersion) SHA: $(PSCoreCommitSHA)</PSCoreFormattedVersion>
<PSCoreFormattedVersion Condition = "'$(PSCoreFormattedVersion)' == ''">$(PSCoreBuildVersion) Commits: $(PSCoreAdditionalCommits) SHA: $(PSCoreCommitSHA)</PSCoreFormattedVersion>

<ProductVersion>$(PSCoreFormattedVersion)</ProductVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved.


<ProductVersion>$(PSCoreFormattedVersion)</ProductVersion>
<!--
Here we define explicitly 'Version' to set 'FileVersion' by 'GetAssemblyVersion' target in 'Microsoft.NET.GenerateAssemblyInfo.targets'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<InformationalVersion>$(PSCoreFormattedVersion)</InformationalVersion>

<!--VersionPrefix>$(PSCorePrefixVersion)</VersionPrefix-->
<!--VersionSuffix>$(PSCoreLabelVersion)</VersionSuffix-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they can be removed now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

build.psm1 Outdated
if ($VersionSuffix) {
dotnet pack "src/$_" --output $OutputPath --version-suffix $VersionSuffix /p:IncludeSymbols=true
if ($ReleaseTag) {
dotnet pack "src/$_" --output $OutputPath "/property:ReleaseTag=$ReleaseTag1" /p:IncludeSymbols=true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. (I can not test this.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ProjectVersion = '!$(ProjectVersion)!'
InformationalVersion = $(InformationalVersion);
"
Overwrite="true"/-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#Closed

build.psm1 Outdated
$ReleaseTag1 = $ReleaseTag -Replace '^v'
dotnet restore $TopProject "/property:ReleaseTag=$ReleaseTag1"
} else {
dotnet restore $TopProject
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -451,24 +451,26 @@ function Invoke-AppveyorFinish

if ($env:APPVEYOR_REPO_TAG_NAME)
{
# ignore the first part of semver, use the preview part
$preReleaseVersion = ($env:APPVEYOR_REPO_TAG_NAME).Split('-')[1]
$preReleaseVersion = "v$($env:APPVEYOR_REPO_TAG_NAME)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

@iSazonov iSazonov Aug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Feature] - done.

Copy link
Member

@TravisEz13 TravisEz13 Sep 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

}

# only publish to nuget feed if it is a daily build and tests passed
if((Test-DailyBuild) -and $env:TestPassed -eq 'True')
{
Publish-NuGetFeed -OutputPath .\nuget-artifacts -VersionSuffix $preReleaseVersion
Publish-NuGetFeed -OutputPath .\nuget-artifacts -ReleaseTag "/property:ReleaseTag=$preReleaseVersion"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-ReleaseTag "/property:ReleaseTag=$preReleaseVersion"

This seems wrong ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry. Fixed.

@iSazonov iSazonov force-pushed the cproj-psversion-from-git branch from 92b1ec4 to 76a7e6f Compare August 31, 2017 10:41
@daxian-dbw
Copy link
Member

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
Copy link
Member

TravisEz13 commented Sep 1, 2017

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
Copy link
Member

@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
Copy link
Member

No concerns with that change

@TravisEz13
Copy link
Member

Windows artifacts look good.

@daxian-dbw
Copy link
Member

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.

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).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@daxian-dbw daxian-dbw dismissed their stale review September 6, 2017 05:35

Comment fixed

@daxian-dbw
Copy link
Member

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

@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 6, 2017

Yes, thanks! Look very good.

@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 6, 2017

Get-ComputerInfo test failed in AppVeyor CI.

@daxian-dbw
Copy link
Member

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.

@daxian-dbw daxian-dbw merged commit e588a18 into PowerShell:master Sep 6, 2017
@iSazonov
Copy link
Collaborator Author

iSazonov commented Sep 7, 2017

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

@iSazonov iSazonov deleted the cproj-psversion-from-git branch September 7, 2017 05:02
@daxian-dbw
Copy link
Member

@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
Copy link
Collaborator Author

iSazonov commented Sep 7, 2017

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

@iSazonov
Copy link
Collaborator Author

@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
Copy link
Member

I vote for getting the needed info from the attributes 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants