-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
fix(cli): Windows: set exe file icon and PE metadata #6693
Conversation
cli/Cargo.toml
Outdated
[package.metadata.winres] | ||
# On Windows, this section defines the metadata that appears in the Deno.exe Portable Executable header | ||
OriginalFilename = "Deno.exe" | ||
LegalCopyright = "Copyright © 2018-2020 the Deno authors. All rights reserved. MIT license." |
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.
Not sure what's correct here so this is just a side note.
Was poking around on my Windows VM and for Microsoft products (notepad, explorer) etc this doesn't contain a year, it's just "© Microsoft Corporation. All Rights Reserved."
"Copyright: Copyright © 2018-2020 The Deno Authors" has a bit of an echo to 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.
Depends on what the deno project prefers really.
PuTTY has: Copyright © 1997 - 2017 Simon Tatham
RealVNC has: Copyright © 2002-2020 RealVNC Ltd.
nodejs has: Copyright Node.js contributors. MIT license.
Wireshark has: Copyright © 2000 Gerald Combs [email protected]
Firefox has: ©Firefox and Mozilla Developers: available under the MPL 2 license.
Chrome has: Copyright 2019 Google LLC. All rights reserved.
I guess I'll remove the "Copyright" and the years and the ©
Co-authored-by: Mark Tiedemann <[email protected]>
Co-authored-by: Mark Tiedemann <[email protected]>
Fixed - kept it transparent on the outside and white on the inside |
Yeah, that is as far as my graphic skills go... maybe we can get one of the deno artists to provide an icon file that works well in light and dark themes? |
@Spoonbender The default language seems to be "Language Neutral". Since Deno is only available in English so far, do you think it makes sense to set the language as well? |
Yes, because I can't resist the detailitis! |
According to https://deno.land/artwork, @kevinkassimo created this logo. Perhaps he can help? |
@MarkTiedemann Image with infill, without the white border: |
@lucacasonato Can you export the image as an |
… by Luca Casonato
Thanks @lucacasonato |
Huh that is weird. I exported it at the largest possible .ico size of 255x255. I'll take a look |
I would lean towards just landing this. We can improve it in the future. Is there more work to be done here? |
The white border is a conversion artefact I suspect - some tool was used to make the ICO and it dropped the alpha (transparency) channel. But let's land it as-is. We can always improve it further, PR numbers are cheap :) |
Can you try this .ico as a final attempt before landing? https://drive.google.com/file/d/1-10fAAErCQWQ-yVTCrcIXZEbeGMjDM-w/view?usp=sharing |
That one looks perfect to me, even with "Extra large icons". :) |
Looks great! I committed and pushed this new icon, I guess we can move forward now @piscisaureus |
|
||
#[cfg(target_os = "windows")] | ||
fn set_binary_metadata() { | ||
let mut res = winres::WindowsResource::new(); |
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 really dig that we can pull this in as a cargo dep, rather than figuring out how to get rc.exe
installed on every user's machine. I wish we could do that with our ninja/gn/python build dependencies too.
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.
Read through the implementation a couple of days ago, it doesn't bring it's rc.exe. It has to be in PATH which is the case if you have the Windows SDK.
Definitively agree that it is is clean and convenient how this works out tho!
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 is why my original implementation did not treat winres compilation errors as fatal - so users without rc.exe can keep building Deno on their machines. I guess we can always improve this in the future, perhaps by fetching rc.exe on-demand, if the need arises.
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
Fix: #6593
Windows: set executable icon and PE metadata
Result:
![image](https://user-images.githubusercontent.com/6010433/87108645-734df700-c26b-11ea-8202-1fb100e5d0a0.png)
![image](https://user-images.githubusercontent.com/6010433/87108700-92e51f80-c26b-11ea-9866-afc0f010d425.png)