GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Refactor MSBuild project files to get PowerShell version from git tag by iSazonov · Pull Request #4182 · PowerShell/PowerShell

Conversation

@iSazonov

@iSazonov Ilya (iSazonov) commented Jul 4, 2017 •
edited by daxian-dbw
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Ilya (iSazonov) requested a review from Dongbo Wang (daxian-dbw) July 4, 2017 10:25
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Jul 4, 2017
@daxian-dbw Dongbo Wang (daxian-dbw) self-assigned this Jul 5, 2017
@iSazonov

Ilya (iSazonov) commented Jul 14, 2017

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@daxian-dbw) Could you please review?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jul 14, 2017

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Sure, next item on my list :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mi-hol

mi-hol commented Aug 18, 2017

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Dongbo Wang (daxian-dbw) force-pushed the cproj-psversion-from-git branch from cab3688 to f27883d Compare August 18, 2017 21:24
@iSazonov
Ilya (iSazonov) requested review from Travis Plunk (TravisEz13) and Aditya Patwardhan (adityapatwardhan) as code owners August 18, 2017 21:24
@daxian-dbw
Dongbo Wang (daxian-dbw) removed the request for review from Aditya Patwardhan (adityapatwardhan) August 18, 2017 21:31
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Aug 18, 2017

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Aug 19, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) force-pushed the cproj-psversion-from-git branch from f27883d to e4afa43 Compare August 21, 2017 11:12
@iSazonov Ilya (iSazonov) mentioned this pull request Aug 21, 2017
Minor formatting commits for Build.psm1 and csproj files #4630
Merged
TravisEz13
Travis Plunk (TravisEz13) reviewed Aug 21, 2017
View reviewed changes
Comment thread PowerShell.Common.props Outdated

@TravisEz13 Travis Plunk (TravisEz13) Aug 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Aug 21, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 21, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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 - Dongbo Wang (@daxian-dbw) ask the same and I'll check this and fix if need.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Aug 21, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 22, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Get PowerShell version from git tag
b74c493
@iSazonov
Ilya (iSazonov) force-pushed the cproj-psversion-from-git branch from e4afa43 to b74c493 Compare August 22, 2017 09:11
@iSazonov
Add support ReleaseTag in Start-PSBuild and Publish-NuGetFeed
840c18a
daxian-dbw
Dongbo Wang (daxian-dbw) previously requested changes Aug 22, 2017
View reviewed changes
Comment thread PowerShell.Common.props Outdated

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated
<GitExe>git</GitExe>
</PropertyGroup>

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Line removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Replaced with StandardOutputImportance="Low"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props
</PropertyGroup>

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good catch!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 25, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry. Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props
</PropertyGroup>

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

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 22, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Aug 23, 2017
View reviewed changes
Comment thread build.psm1
[Parameter(ParameterSetName = "ReleaseTag")]
[ValidatePattern("^v\d+\.\d+\.\d+(-\w+\.\d+)?$")]
[ValidateNotNullOrEmpty()]
[string]$ReleaseTag

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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 Travis Plunk (@TravisEz13) for input.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 24, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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.

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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) {

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Aug 23, 2017
View reviewed changes
Comment thread build.psm1 Outdated
[string]$OutputPath = "$PSScriptRoot/nuget-artifacts",
[Parameter(Mandatory=$true)]
[string]$VersionSuffix
[Parameter(ParameterSetName = "ReleaseTag")]

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 23, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Remove unneeded comments and codes.
890e435 
Fix  appveyor.psm1
Add WorkingDirectory=$(MSBuildProjectDirectory)
@daxian-dbw
Dongbo Wang (daxian-dbw) dismissed their stale review August 25, 2017 16:01

Dismiss my previous review because of new commits

daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Aug 28, 2017
View reviewed changes
Comment thread PowerShell.Common.props Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 25, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Yes. Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 31, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 31, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Moved.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 31, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread build.psm1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread PowerShell.Common.props Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 31, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Cleaned up.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

#Closed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread build.psm1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/appveyor.psm1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 31, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Travis Plunk (@TravisEz13) can you comment on this one? What would $env:APPVEYOR_REPO_TAG_NAME be like?
Ilya (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 31, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

[Feature] - done.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@TravisEz13 Travis Plunk (TravisEz13) Sep 1, 2017 •
edited by daxian-dbw
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 4, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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,

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tools/appveyor.psm1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Aug 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Aug 29, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry. Fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov
Minor fixes
39e04f1
@iSazonov
Ilya (iSazonov) force-pushed the cproj-psversion-from-git branch from 06e2998 to 39e04f1 Compare August 29, 2017 08:11
@daxian-dbw Dongbo Wang (daxian-dbw) added the Review - Waiting on Author label Aug 31, 2017
@iSazonov Ilya (iSazonov) mentioned this pull request Aug 31, 2017
'PackageVersion' is reset to '1.0.0' dotnet/sdk#1557
Closed
@iSazonov
Ilya (iSazonov) force-pushed the cproj-psversion-from-git branch from 0835d32 to 92b1ec4 Compare August 31, 2017 08:56
@iSazonov
[Feature] Minor cleanups
76a7e6f
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Aug 31, 2017

Copy link
Copy Markdown
Member

The changes to build.psm1 and PowerShell.Common.props are mostly good. I asked Travis Plunk (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13

Travis Plunk (TravisEz13) commented Sep 1, 2017 •
edited by daxian-dbw
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Ilya (iSazonov) and others added 2 commits September 4, 2017 08:19
@iSazonov
Fix previewLabel in AppVeyor.psm1
44101fc
@daxian-dbw
[Feature] Fix nuget package publish in daily build
953fb6b
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 4, 2017

Copy link
Copy Markdown
Member

Travis Plunk (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw Dongbo Wang (daxian-dbw) removed the Review - Waiting on Author label Sep 5, 2017
@TravisEz13

Travis Plunk (TravisEz13) commented Sep 5, 2017

Copy link
Copy Markdown
Member

No concerns with that change
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@TravisEz13

Travis Plunk (TravisEz13) commented Sep 5, 2017

Copy link
Copy Markdown
Member

Windows artifacts look good.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 5, 2017

Copy link
Copy Markdown
Member

Thanks Travis Plunk (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Update some comments in 'PowerShell.Common.props'
048ab75
daxian-dbw
Dongbo Wang (daxian-dbw) previously requested changes Sep 5, 2017
View reviewed changes
Comment thread PowerShell.Common.props Outdated
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).

@daxian-dbw Dongbo Wang (daxian-dbw) Sep 5, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 6, 2017

Copy link
Copy Markdown
Collaborator Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Ilya (iSazonov) and others added 2 commits September 6, 2017 06:59
@iSazonov
Fix comment
5bae6ba
@daxian-dbw
[Feature] Reword the comments
fb867bd
@daxian-dbw
Dongbo Wang (daxian-dbw) dismissed their stale review September 6, 2017 05:35

Comment fixed

@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 6, 2017

Copy link
Copy Markdown
Member

Ilya (@iSazonov) Can you please take a look and see if my re-write reflects what you think?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 6, 2017

Copy link
Copy Markdown
Collaborator Author

Yes, thanks! Look very good.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 6, 2017

Copy link
Copy Markdown
Collaborator Author

Get-ComputerInfo test failed in AppVeyor CI.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 6, 2017

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Sep 6, 2017
View reviewed changes
@daxian-dbw
Remove an unnecessary empty line
0a09b2d
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit e588a18 into PowerShell:master Sep 6, 2017
@iSazonov

Ilya (iSazonov) commented Sep 7, 2017

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@daxian-dbw) Travis Plunk (@TravisEz13) Thanks for review and approvement!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov
Ilya (iSazonov) deleted the cproj-psversion-from-git branch September 7, 2017 05:02
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 7, 2017

Copy link
Copy Markdown
Member

Ilya (@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#.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 7, 2017

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@daxian-dbw) My question was more about #3690 - I'll continue.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Sep 15, 2017

Copy link
Copy Markdown
Collaborator Author

Dongbo Wang (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Sep 15, 2017

Copy link
Copy Markdown
Member

I vote for getting the needed info from the attributes 😄
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@iSazonov @daxian-dbw
Refactor MSBuild project files to get PowerShell version from git tag (…
b5fcaef 
…PowerShell#4182)

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 Travis Plunk (TravisEz13) TravisEz13 left review comments
@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@iSazonov @daxian-dbw @mi-hol @TravisEz13 @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.