Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Handle when the ICO file isn't present#13060

Closed
TylerLeonhardt wants to merge 11 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
TylerLeonhardt:patch-1TylerLeonhardt/PowerShell:patch-1Copy head branch name to clipboard
Closed

Handle when the ICO file isn't present#13060
TylerLeonhardt wants to merge 11 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
TylerLeonhardt:patch-1TylerLeonhardt/PowerShell:patch-1Copy head branch name to clipboard

Conversation

@TylerLeonhardt

@TylerLeonhardt TylerLeonhardt commented Jun 30, 2020

Copy link
Copy Markdown
Member

PR Summary

  • Bump rcedit version used
  • Check if ICO file exists before running rcedit

PR Context

rcedit is failing to set icons in CI scenarios because the ICO doesn't exist in the package.

PR Checklist

@iSazonov

Copy link
Copy Markdown
Collaborator

@TylerLeonhardt Does new version work? Can we merge?

@iSazonov iSazonov added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label Jun 30, 2020
@TylerLeonhardt TylerLeonhardt changed the title Bump rcedit version Handle when the ICO file isn't present Jul 1, 2020
@TylerLeonhardt

Copy link
Copy Markdown
Member Author

This needed more work because apparently the actual issue is that the ICO doesn't exist. I'm tracking down why, but this change is nice because setting the ICO isn't too important.

cc @adityapatwardhan

@adityapatwardhan

Copy link
Copy Markdown
Member

Setting of the icon should be done in packaging instead

@adityapatwardhan

Copy link
Copy Markdown
Member

Changes should be done here:

$ProductSemanticVersion = Get-PackageSemanticVersion -Version $PackageVersion

@TylerLeonhardt

Copy link
Copy Markdown
Member Author

@adityapatwardhan do we know if we're using the Daily at that point?

@TravisEz13

TravisEz13 commented Jul 2, 2020

Copy link
Copy Markdown
Member

If we take this type of fix, I think we should do something more temporary like: https://github.com/PowerShell/PowerShell/pull/13078/files

GitHub
PR Summary Fixes #13077. We now only try to set the icon for pwsh.exe when the ico file is in the payload. PR Context

PR Checklist

PR has a meaningful title

Use the present tense and imperative...

}
(Test-Path "~/.rcedit/rcedit-x64.exe") {
Write-Verbose "Install RCEdit for modifying exe resources" -Verbose
$rceditUrl = "https://github.com/electron/rcedit/releases/download/v1.1.1/rcedit-x64.exe"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this need to run on the client machine?

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 2, 2020
@TylerLeonhardt

Copy link
Copy Markdown
Member Author

Closing this in favor of #13123

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 7, 2020
@TylerLeonhardt TylerLeonhardt deleted the patch-1 branch July 7, 2020 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.