Add icon to Windows executable

[NewtonChutney]

This PR changes the build to include an icon for the Windows executable

fixes #5940

Icon contributed by @goyalyashpal

[pascalkuthe]

I am personally not a fan of pulling in an extra dependency for something that IMO has very little practical use. Nobody start a TUI application from clicking on it, especially because you would need to run it inside a terminal emultor (so the shortcut would have the icon of the terminal emulator).

[NewtonChutney]

I am personally not a fan of pulling in an extra dependency for something that IMO has very little practical use. Nobody start a TUI application from clicking on it, especially because you would need to run it inside a terminal emultor (so the shortcut would have the icon of the terminal emulator).

Creating a shortcut that launches another terminal emulator (eg wezterm), might be the only way to launch a tui app in linux.. (the Linux desktop shortcut icon strategy)

But in Windows, we can launch an tui app by double clicking it.. and it would launch in either conhost/WT (as per the user's preferences). When launched in conhost, it retains its own icon..

Windows: build artifact - its shortcut - thumbnail in taskbar - launched in conhost (build with icon), conhost (stable release), windows terminal

Anyhow, I do respect it, if the PR is closed because of the additional deps.. But it just doesn't scream a polished experience

[NewtonChutney]

There's another approach that doesn't use external crates.. have to check: https://stackoverflow.com/a/76500197
Update: didn't work, but found another post that does!
In this, the resource file is compiled manually and linked with rustc later.. (no extra dependencies but one artifact is added)
Will add this commit here..

[NewtonChutney]

With winres, I see the binary's details are being injected too from the cargo config..

[goyalyashpal]

With winres, I see the binary's details are being injected too from the cargo config..

that's awesome @NewtonChutney - and if you urself hid that comment as OT, then i think u should unhide it.
(that's why i mention on my comments if it is hidden by me)

[goyalyashpal]

The logo.svg was a one liner while the logo-dark/light were multi line files.. I had run a formatter on all of them.

2. lol, i tried same here: Pretty print the minified logo.svg #5941 . apparently, 96bytes (CLR\t * 32 times) is a bit too much.

and actually, i didn't bother with calculations back then... but today:

• the pretty printed one (with tabs) takes less "space" than the original "minified" one lol...
• the original minified doesn't even contain the final file-end newline - characteristic of a linux file

$ eza -lB logo.svg # Original (master branch)
-a--- 2,848 18 Dec ... logo.svg

$ eza -lB logo.svg # Add Newlines, Remove trailing whitespace
-a--- 2,709 18 Dec ... logo.svg

$ eza -lB logo.svg # Add 2nd level tab indentation
-a--- 2,751 18 Dec ... logo.svg

$ eza -lB logo.svg # FInal: Pretty root svg tag - newlines to attributes
-a--- 2,755 18 Dec ... logo.svg

$ # -----------------------------------------------------------------
$ eza -lB logo.svg # JUST FOR RECORDS - "convert indentation to spaces"
-a--- 2,886 18 Dec ... logo.svg

$ eza -lB logo.svg # JUST FOR RECORDS - pr #5941
-a--- 2,989 18 Dec ... logo.svg

$ eza -lB logo.svg #  JUST FOR RECORDS - pr #5941, but now with "convert indentation to tabs"
-a--- 2,785 18 Dec ... logo.svg

[NewtonChutney]

By the logs I see the CI pipeline couldn't take the file..
Lemme try another take..
Or could we keep that file in the helix-term directory beside the build.rs script? This was what was suggested in the post

[NewtonChutney]

The relative upper directory ../ doesn't work locally:

println!("cargo:rustc-link-lib=../contrib/helix-icon-windows");

but ./ does work:

println!("cargo:rustc-link-lib=./contrib/helix-icon-windows");

The attempt with subpath from the root:

println!("cargo:rustc-link-lib=contrib/helix-icon-windows");

[NewtonChutney]

No luck.. I'd need help fixing this!
The cwd/pwd in the CI pipeline seems different

[pascalkuthe]

This is just how rust buildscripts work. Depending on how cargo is I.voked the cwd is different you cant make assumptions about that.

You will need to use the environment variables set by cargo

[NewtonChutney]

You will need to use the environment variables set by cargo

Thanks for the tip @pascalkuthe
You inspired me to dig inside the winres crate to look for cargo vars

I hope this works.. one more test..? :-)
ᓚᘏᗢ

[NewtonChutney]

I have pushed again after formatting, could you run the workflow again and do a final review? @the-mikedavis @pascalkuthe

[pascalkuthe]

I am not quite comfortable committing an opaque binary to the repo. Could we build the .lib file by invoking rc from build.rs?

[NewtonChutney]

I am not quite comfortable committing an opaque binary to the repo. Could we build the .lib file by invoking rc from build.rs?

Yes, but we'll have to do similarly to the winres crate.. Parse regedit for entries, and check if the binary exists..
Working on it

[pascalkuthe]

I am not quite comfortable committing an opaque binary to the repo. Could we build the .lib file by invoking rc from build.rs?

Yes, but we'll have to do similarly to the winres crate.. Parse regedit for entries, and check if the binary exists.. Working on it

I think you can just use cc for that. The cc crat3 provides a function that finds the toolchain in the registry for you and we depend on that already

[NewtonChutney]

So far, with the cc crate I've been able to get link.exe but not rc.exe
I'll have to check if there's another function..

Edit: there's a fun that is implemented on the MscvTool struct..


[NewtonChutney]

I'm having difficulty utilizing this function: get_sdk10_dir() which is in the source code of cc
https://github.com/rust-lang/cc-rs/blob/f17047de579adbe1c3a562b87cf9c0376a8e66cc/src/windows_registry.rs#L735

It appears to be part of this module.. But there's no way to instantiate/utilize it?

[pascalkuthe]

yeah that is private API I assume that you could just use windows_registry::find directly. But that doesn't seem to be the case, unfortunately. In that case just copying the get_sdk function from winres is probably the best solution as cc doesn't expose the relevant internals.

[NewtonChutney]

Yepp, I'll copy the minimum required from winres
To clarify, windows_registry::find isn't private, but it only returns VS installations and related tools..
The other get_10sdk is private

[NewtonChutney]

@pascalkuthe, please do take a look now.. 😅

[pascalkuthe]

this looks mostly good to me the amount of code would be ok as long as you feature gate it so other platforms are not slowed down (see comments)

[NewtonChutney]

It appears the Cachix workflow *was failing..
https://github.com/NewtonChutney/helix/actions/runs/7469536530/job/20326793896#step:5:1
That appears to be a different commit