-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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
add ability to package all of powershell core as a NuGet Package #4363
Conversation
tools/packaging/packaging.psm1
Outdated
|
||
# Setup staging directory so we don't change the original source directory | ||
$stagingRoot = New-SubFolder -Path $PSScriptRoot -ChildPath 'nugetStaging' -Clean | ||
$toolsFolder = Join-Path -path $stagingRoot -ChildPath 'tools' |
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 change the name to 'content'
<metadata> | ||
<id>powershell-$runtime$</id> | ||
<version>$version$</version> | ||
<title>PowerShell CLI for $runtime$</title> |
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 never use "CLI". Maybe <title>PowerShell Core $runtime$</title>
.
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.
Or just PowerShell for $runtime$
:)
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.
Update to PowerShell Core
<version>$version$</version> | ||
<title>PowerShell CLI for $runtime$</title> | ||
<authors>Microsoft PowerShell</authors> | ||
<owners>Microsoft PowerShell</owners> |
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 have #4352 under review - should we use "Microsoft" in community project?
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.
remove microsoft
build.psm1
Outdated
@@ -4,6 +4,84 @@ $script:TestModulePathSeparator = [System.IO.Path]::PathSeparator | |||
|
|||
$dotnetCLIChannel = "preview" | |||
$dotnetCLIRequiredVersion = "2.0.0-preview2-006502" | |||
$tagsUpToDate = $false | |||
function Get-PSTags |
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.
Better use the verb Sync
, as Get-
usually indicates result(s) will be returned while Sync
is an action.
build.psm1
Outdated
} | ||
|
||
$lastestTagCommitId = git rev-list --tags --max-count=1 | ||
return (git --git-dir="$PSScriptRoot/.git" describe $lastestTagCommitId) |
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.
How about return (git tag --list --sort=-v:refname | Select-Object -First 1)
?
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.
Your proposed implementation depends on version sorting.
The code I wrote gets the latest commit which is a tag, then gets the name.
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.
discussed offline and git describe --abbrev=0
does what you intended.
build.psm1
Outdated
$latestTag = Get-PSLatestTag | ||
# Get the last commit id in the current branch | ||
$commitId = git log --pretty="%H" -n1 | ||
# check in the branch is dirty |
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: in -> if
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
build.psm1
Outdated
# Get the last commit id in the current branch | ||
$commitId = git log --pretty="%H" -n1 | ||
# check in the branch is dirty | ||
$isDirty = (git describe --dirty) -match '-dirty$' |
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.
git describe --dirty
causes git to crash on my machine. No idea why is that.
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.
perhaps it could not find the git-dir. I'll update all the commands to specify it.
<metadata> | ||
<id>powershell-$runtime$</id> | ||
<version>$version$</version> | ||
<title>PowerShell CLI for $runtime$</title> |
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.
Or just PowerShell for $runtime$
:)
tools/packaging/packaging.psm1
Outdated
if($pscmdlet.ShouldProcess("Create NuPkg Package")) | ||
{ | ||
New-NugetPackage @Arguments | ||
} |
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.
nit: there are trailing spaces in this line.
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.
#resolved
[string]$Iteration = "1", | ||
|
||
[Switch] | ||
$Force |
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.
nit: same here.
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.
#resolved
--> | ||
<PropertyGroup> | ||
<AssemblyName>NotUsed</AssemblyName> | ||
<Description>PowerShell top-level project with .NET CLI host</Description> |
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 description needs to be updated
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.
#resloved
<licenseUrl>https://github.com/PowerShell/PowerShell/blob/master/LICENSE.txt</licenseUrl> | ||
<projectUrl>https://github.com/powershell/powershell</projectUrl> | ||
<iconUrl>https://github.com/PowerShell/PowerShell/blob/master/assets/Powershell_64.png</iconUrl> | ||
<description>This package contains the PowerShell CLI for $runtime$.</description> |
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.
PowerShell CLI
Maybe just PowerShell
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.
Changed to PowerShell Core
build.psm1
Outdated
$commitId+='-dirty' | ||
} | ||
|
||
return "$latestTag-x-g$commitId" |
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 we continue to use git describe
to get the commit id. It should work as long as we merge the release branch.
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.
updated
3ab8281
to
4bf60fd
Compare
4bf60fd
to
ac6c62a
Compare
} | ||
elseif(!$upstreamRemote) | ||
{ | ||
Write-Error "Please add a remote to PowerShell\PowerShell. Example: git remote add $upstreamRemoteDefaultName $PowerShellRemoteUrl" -ErrorAction Stop |
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.
Can we continue to execute the script after the error?
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.
You should not continue executing the script unless you have a remote to PowerShell as the error indicates. The script has always assumed you can fetch the tags. This is just adding a verification.
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 should only occur when someone manually runs the function and it cannot complete the action. So, returning an error and stopping is the correct thing to do.
--> | ||
<PropertyGroup> | ||
<AssemblyName>NotUsed</AssemblyName> | ||
<Description>PowerShell Core nuget packag with .NET CLI host including everything needed to run it.</Description> |
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 - "packag".
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.
LGTM except for a minor comment.
build.psm1
Outdated
@@ -4,6 +4,79 @@ $script:TestModulePathSeparator = [System.IO.Path]::PathSeparator | |||
|
|||
$dotnetCLIChannel = "preview" | |||
$dotnetCLIRequiredVersion = "2.0.0-preview2-006502" | |||
$tagsUpToDate = $false | |||
|
|||
function Sync-PSTags |
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.
Can you please add a comment to explain why we need to run Sync-PSTags
before getting the version (tag) and commit ID? It will help people to understand.
Example usage