-
Notifications
You must be signed in to change notification settings - Fork 223
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
Log build info #772
Log build info #772
Conversation
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 like it! LGTM
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.
Just couple questions
PowerShellEditorServices.build.ps1
Outdated
$buildVersion = $env:APPVEYOR_BUILD_VERSION | ||
$buildOrigin = if ($env:CI) { "AppVeyor CI" } else { "AppVeyor" } | ||
} | ||
elseif ($env:VSTS_BUILD) |
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.
Last I checked the VSTS environment variable that's set is TF_BUILD
. Do they also set VSTS_BUILD
?
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.
But I just saw that TF_BUILD
is defined as well. So probably a good opportunity to remove VSTS_BUILD
.
{ | ||
$psd1Path = [System.IO.Path]::Combine($PSScriptRoot, "module", "PowerShellEditorServices", "PowerShellEditorServices.psd1") | ||
$buildVersion = (Import-PowerShellDataFile -LiteralPath $psd1Path).Version | ||
$buildOrigin = "VSTS" |
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.
Ehhem "Azure Pipelines" 😊
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 about it, but felt we should keep things consistent and change it over all at once.
@@ -20,6 +20,8 @@ | |||
<PackageReference Include="Microsoft.PowerShell.SDK"> | |||
<Version>6.0.0-alpha13</Version> | |||
</PackageReference> | |||
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0" /> | |||
<PackageReference Include="System.Runtime.InteropServices.RuntimeInformation" Version="4.3.0" /> |
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.
Now that we are shipping this, I think there were a couple instances where we did some ifdefs between Core and Framework (which didn't have this package). If it's easy, can you remove the ifdefs
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 don't forget to register the open source usage.
@@ -20,8 +20,6 @@ | |||
<PackageReference Include="Microsoft.PowerShell.SDK"> | |||
<Version>6.0.0-alpha13</Version> | |||
</PackageReference> | |||
<PackageReference Include="System.Runtime.Extensions" Version="4.3.0" /> |
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 come you removed this? Is it because PowerShellEditorServices project uses it?
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.
Hmmmm, this might have been my "rip out redundant dependencies" reflex kicking in from the v2 refactor. Perhaps I should put it back in 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.
LGTM 👍 just a single question
Partially helps PowerShell/vscode-powershell#1393.
This logs the information about where the extension was build in the log file at startup.
I also cleaned up the startup logging a little bit.