Skip to content
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

Give more specific errors when rcedit cannot set an icon #74

Merged
merged 1 commit into from
May 7, 2018

Conversation

malept
Copy link
Member

@malept malept commented May 2, 2018

In the Electron Packager and Electron Forge issue trackers, one of the common issues that pops up is people asking what's going on when the build process errors out with rcedit.exe failed with exit code 1. Fatal error: Unable to set icon. This pull request attempts to give more details as to why setting the icon failed.

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

LOG(ERROR) << ... would be the more idiomatic way to do this in Electron's source.

Other than that, LGTM. Thanks for the patch!

@MarshallOfSound
Copy link
Member

@ckerr I don't think rcedit has access to that macro 🤔

@malept
Copy link
Member Author

malept commented May 2, 2018

I'd be happy to use a more idiomatic way to deal with errors, but my knowledge of the C++/Windows ecosystem is minimal, so assistance would be much appreciated.

Copy link
Contributor

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

For this tiny project fprintf looks good enough, currently it does not have LOG(ERROR) which is a thing from Chromium.

@zcbenz zcbenz merged commit fc54d9f into electron:master May 7, 2018
@malept malept deleted the better-icon-errors branch May 7, 2018 05:49
@malept
Copy link
Member Author

malept commented May 7, 2018

Thanks @zcbenz. When can we expect a new release of rcedit and node-rcedit?

@zcbenz
Copy link
Contributor

zcbenz commented May 7, 2018

@malept Soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants