-
Notifications
You must be signed in to change notification settings - Fork 7.6k
use rcedit to embed icon and version information into pwsh.exe #5178
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
Conversation
@@ -1862,6 +1862,18 @@ function New-MSIPackage | |||
[Switch] $Force | |||
) | |||
|
|||
## need RCEdit to modify the binaries embedded resources | |||
if (-not (Test-Path "~/.rcedit/rcedit-x64.exe")) |
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.
What about Unix?
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'm not as familiar on non-Windows with regards to viewing resources (which Explorer does easily), is this something I can do easily with Ubuntu? If so, I can move this to the build step rather than packaging.
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 rcedit only works on Windows
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.
Get-Item <file> | fl *
show ProductVersion on WSL.
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.
@iSazonov not working for me with WSL:
PS /mnt/c/Windows/System32/WindowsPowerShell/v1.0> get-item ./powershell.exe | fl *
PSPath : Microsoft.PowerShell.Core\FileSystem::/mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe
PSParentPath : Microsoft.PowerShell.Core\FileSystem::/mnt/c/Windows/System32/WindowsPowerShell/v1.0
PSChildName : powershell.exe
PSDrive : /
PSProvider : Microsoft.PowerShell.Core\FileSystem
PSIsContainer : False
Mode : --r--l
VersionInfo : File: /mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe
InternalName:
OriginalFilename:
FileVersion:
FileDescription:
Product:
ProductVersion:
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 looks like PowerShell is simply using the FileVersionInfo class which doesn't return anything on Linux:
PS /mnt/c/Windows/System32/WindowsPowerShell/v1.0> [System.Diagnostics.FileVersionInfo]::GetVersionInfo("/mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe")
ProductVersion FileVersion FileName
-------------- ----------- --------
/mnt/c/Windows/System32/WindowsPowerShell/v1.0/powershell.exe
but on Windows:
PS C:\Windows\System32\WindowsPowerShell\v1.0> [System.Diagnostics.FileVersionInfo]::GetVersionInfo("C:\Windows\system32
\WindowsPowerShell\v1.0\powershell.exe")
ProductVersion FileVersion FileName
-------------- ----------- --------
10.0.17019.1000 10.0.17019.10... C:\Windows\system32\WindowsPowerShell\v1.0\powershell.exe
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.
Linux ELF binaries probably don't have a resource section to stuff this info into.
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.
@SteveL-MSFT Why you test the Windows FullCLR exe file? In my test I used SMA.dll from PowerShell Core.
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 used Windows PowerShell.exe as I knew that had that info populated. I see that SMA.dll from PSCore6 does have that info as does powershell.exe from PSCore6. So dotnet build is adding that, but the executable doesn't have 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.
This might be because the executable is an actual ELF binary while the dlls are using the same format as Windows making them portable.
This will fix #5034 as well. |
It seems CoreFX use System.Reflection.Metadata to read FileVersionInfo. Perhaps we can use the API to write meta data too. |
@iSazonov I think we can move to that later particularly if we expose cmdlets for it |
@SteveL-MSFT Do you want to exclude Unix from the PR? |
@iSazonov using the |
@SteveL-MSFT Thanks for clarify. I see Unix haven't a standard for this. |
build.psm1
Outdated
## need RCEdit to modify the binaries embedded resources | ||
if (-not (Test-Path "~/.rcedit/rcedit-x64.exe")) | ||
{ | ||
$rceditUrl = "https://github.com/electron/rcedit/releases/download/v1.0.0/rcedit-x64.exe" |
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 install this via chocolatey or nuget? Based on my understanding of our requirements for release builds, this will have to be rewritten sooner rather than later.
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 move it to Start-PSBootstrap?
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 found nuget package.
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 not on chocolatey or nuget currently. I could move the install to start-psbootstrap.
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 under MIT license. Nothing keeps us from packaging it our self... and chocolatey, doesn't even package it, it just encapsulates the installation code.
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 this can wait until we have to do it.
@@ -1442,6 +1442,15 @@ function Start-PSBootstrap { | |||
|
|||
# Install Windows dependencies if `-Package` or `-BuildWindowsNative` is specified | |||
if ($Environment.IsWindows) { | |||
## need RCEdit to modify the binaries embedded resources | |||
if (-not (Test-Path "~/.rcedit/rcedit-x64.exe")) |
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.
What about Windows 32-bit?
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.
Yeah, will add 32-bit support
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.
Actually, I don't think any change is needed here. I believe we build x86 running as x64, so we can still use the x64 rcedit to modify the x86 binary.
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.
Clear. Thanks.
Closed.
pwsh.exe today doesn't contain file version information and the icon is only associated with the shortcut file and not the exe
Fix is to use rcedit to embed:
Fix #2883
Fix #5166
Fix #5034