Implement -version parameter in console host (address part of https:/… #3115

Merged
merged 2 commits into from Feb 23, 2017

Conversation

Projects
None yet
8 participants
@JamesWTruher
Member

JamesWTruher commented Feb 8, 2017

#1084

This does not support providing a specific version to run, but
like most other *nix commands, -version will now return the version
of the PowerShell Engine. 'powershell' is prepended to the output to
match other *nix commands. We are using gitcommitid which includes more
info about the build.

@joeyaiello

This comment has been minimized.

Show comment
Hide comment
@joeyaiello

joeyaiello Feb 9, 2017

Member

@PowerShell/powershell-committee is good with all of this. 👍

Member

joeyaiello commented Feb 9, 2017

@PowerShell/powershell-committee is good with all of this. 👍

@@ -551,6 +571,7 @@ private void ParseHelper(string[] args)
{
_sshServerMode = true;
}
+

This comment has been minimized.

@vors

vors Feb 9, 2017

Collaborator

redundant extra-line before else #Resolved

@vors

vors Feb 9, 2017

Collaborator

redundant extra-line before else #Resolved

+ {
+ get
+ {
+ return _showVersion;

This comment has been minimized.

@vors

vors Feb 9, 2017

Collaborator

why not simple { get; private set; } #WontFix

@vors

vors Feb 9, 2017

Collaborator

why not simple { get; private set; } #WontFix

This comment has been minimized.

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

I think it's fine to follow the existing code pattern here. We can convert the style using a tool later. #ByDesign

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

I think it's fine to follow the existing code pattern here. We can convert the style using a tool later. #ByDesign

This comment has been minimized.

@JamesWTruher

JamesWTruher Feb 17, 2017

Member

to match the current code style


In reply to: 100213197 [](ancestors = 100213197)

@JamesWTruher

JamesWTruher Feb 17, 2017

Member

to match the current code style


In reply to: 100213197 [](ancestors = 100213197)

@@ -509,6 +516,19 @@ private void ParseHelper(string[] args)
switchKey = switchKey.Substring(1);
}
+ // If version is in the commandline, don't continue to look at any other parameters

This comment has been minimized.

@vors

vors Feb 9, 2017

Collaborator

I wonder how it's going to interact with existing in Windows PowerShell
powershell -version 2.0

Since we are stopping to build Windows powershell, and core edition doesn't have any -version 2.0 support, it's probably okey. #ByDesign

@vors

vors Feb 9, 2017

Collaborator

I wonder how it's going to interact with existing in Windows PowerShell
powershell -version 2.0

Since we are stopping to build Windows powershell, and core edition doesn't have any -version 2.0 support, it's probably okey. #ByDesign

This comment has been minimized.

@SteveL-MSFT

SteveL-MSFT Feb 9, 2017

Member

@vors, the @PowerShell/powershell-committee discussed this and that is pretty much the thinking: -version for Windows PowerShell really only supported 2.0 and we're moving away from that. Aligning with Linux convention makes sense. PSCore supports side-by-side intrinsically and using file paths to launch different versions is completely acceptable. #ByDesign

@SteveL-MSFT

SteveL-MSFT Feb 9, 2017

Member

@vors, the @PowerShell/powershell-committee discussed this and that is pretty much the thinking: -version for Windows PowerShell really only supported 2.0 and we're moving away from that. Aligning with Linux convention makes sense. PSCore supports side-by-side intrinsically and using file paths to launch different versions is completely acceptable. #ByDesign

This comment has been minimized.

@joeyaiello

joeyaiello Feb 9, 2017

Member

Furthermore we can always do something like powershell -version:2 if people really demand that functionality in the future (and actually @lzybkr and @BrucePay agreed that it would even be acceptable to do powershell -version 2 as it's highly unlikely that anyone wants to echo one int when they shell out to PowerShell).

@joeyaiello

joeyaiello Feb 9, 2017

Member

Furthermore we can always do something like powershell -version:2 if people really demand that functionality in the future (and actually @lzybkr and @BrucePay agreed that it would even be acceptable to do powershell -version 2 as it's highly unlikely that anyone wants to echo one int when they shell out to PowerShell).

+ _noInteractive = true;
+ _skipUserInit = true;
+ _noExit = false;
+ _commandLineCommand = "'powershell ' + $psversiontable.gitcommitid";

This comment has been minimized.

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

Why are you setting _commandLineCommand here? #Resolved

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

Why are you setting _commandLineCommand here? #Resolved

This comment has been minimized.

@JamesWTruher

JamesWTruher Feb 17, 2017

Member

vestige of earlier implementation - removed


In reply to: 100401276 [](ancestors = 100401276)

@JamesWTruher

JamesWTruher Feb 17, 2017

Member

vestige of earlier implementation - removed


In reply to: 100401276 [](ancestors = 100401276)

+ // Alternatively, we could call s_theConsoleHost.UI.WriteLine(s_theConsoleHost.Version.ToString());
+ // or start up the engine and retrieve the information via $psversiontable.GitCommitId
+ // but this returns the semantic version and avoids executing a script
+ s_theConsoleHost.UI.WriteLine("powershell " + PSVersionInfo.GetCommitInfo());

This comment has been minimized.

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

I recommend to use PSVersionInfo.GetPSVersionTable()["GitCommitId"], or even better, add an internal static property GitCommitId right after the PSVersion property at here, and then use PSVersionInfo.GitCommitId here.

Reason: PSVersionInfo has a type initializer, which calls GetCommitInfo() and store the value in a hashtable. the current implementation of GetCommitInfo() reads git commit information from a file, which is slow and this implementation is subject to change (this method may be gone in near future -- we want to generate code at build time to embed the git commit id info, so as to avoid reading a file at startup time). Since the type initializer must run the first time you access a type, it's a waste to call GetCommitInfo() here to read the file again. #Resolved

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

I recommend to use PSVersionInfo.GetPSVersionTable()["GitCommitId"], or even better, add an internal static property GitCommitId right after the PSVersion property at here, and then use PSVersionInfo.GitCommitId here.

Reason: PSVersionInfo has a type initializer, which calls GetCommitInfo() and store the value in a hashtable. the current implementation of GetCommitInfo() reads git commit information from a file, which is slow and this implementation is subject to change (this method may be gone in near future -- we want to generate code at build time to embed the git commit id info, so as to avoid reading a file at startup time). Since the type initializer must run the first time you access a type, it's a waste to call GetCommitInfo() here to read the file again. #Resolved

+ $observed = & $powershell -version -command get-date
+ # no extraneous output
+ $observed | should be $currentVersion
+

This comment has been minimized.

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

There is an extra blank line here. #Resolved

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

There is an extra blank line here. #Resolved

+
+ It "-Version should ignore other parameters" {
+ $currentVersion = "powershell " + $PSVersionTable.GitCommitId.ToString()
+ $observed = & $powershell -version -command get-date

This comment has been minimized.

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

what would happen if I have powershell -noprofile -noexit -version? #WontFix

@daxian-dbw

daxian-dbw Feb 9, 2017

Member

what would happen if I have powershell -noprofile -noexit -version? #WontFix

This comment has been minimized.

@JamesWTruher

JamesWTruher Feb 17, 2017

Member

it would report the version and exit - I can add a test for that explicitly if you like. Rather than testing all of the permutations for the parameters, I chose a representative single case


In reply to: 100405816 [](ancestors = 100405816)

@JamesWTruher

JamesWTruher Feb 17, 2017

Member

it would report the version and exit - I can add a test for that explicitly if you like. Rather than testing all of the permutations for the parameters, I chose a representative single case


In reply to: 100405816 [](ancestors = 100405816)

Implement -version parameter in console host (address part of #1084)
This does not support providing a specific version to run, but
like most other *nix commands, -version will now return the version
of the PowerShell Engine. 'powershell' is prepended to the output to
match other *nix commands. We are using gitcommitid which includes more
info about the build.
use a new static internal for GitCommitId rather than calling method …
…on PSVersionInfo

Remove extraneous blank lines in files
remove line which set the commandline when retrieving the version as it is unneeded
@daxian-dbw

LGTM

@daxian-dbw daxian-dbw merged commit 9e27f4a into PowerShell:master Feb 23, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

BrucePay added a commit to BrucePay/PowerShell that referenced this pull request Mar 17, 2017

Implement -version parameter in console host (address part of https:/… (
#3115)

* Implement -version parameter in console host (address part of PowerShell#1084)

This does not support providing a specific version to run, but
like most other *nix commands, -version will now return the version
of the PowerShell Engine. 'powershell' is prepended to the output to
match other *nix commands. We are using gitcommitid which includes more
info about the build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment