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

fix build with sixel feature on windows #638

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

juliamertz
Copy link
Contributor

There's an open pull request for viuer adding support for the native icy_sixel implementation: atanunq/viuer#63. However, I currently can't get that branch to work without pulling in the broken sixel-sys dependency.

For now, I've forked that PR's branch and removed the old sixel dependencies, which allows the Windows build to succeed again. I'll leave this here as a draft until that PR is merged.

This should also fix #628

@ollbx
Copy link

ollbx commented Dec 30, 2024

Hi. I created the PR for icy_sixel in viuer. Thanks for making the PR here, spotify-player was actually one of the things I had in mind for this.

I've made some changes to my PR branch, so that the auto-traits on the error type are no longer affected. The changeset required to get things to compile is now relatively simple. Based on current master it should look something like this:

diff --git a/spotify_player/Cargo.toml b/spotify_player/Cargo.toml
index 5b0c33e..e4b9e7f 100644
--- a/spotify_player/Cargo.toml
+++ b/spotify_player/Cargo.toml
@@ -41,7 +41,7 @@ tracing = "0.1.41"
 tracing-subscriber = { version = "0.3.19", features = ["env-filter"] }
 backtrace = "0.3.74"
 souvlaki = { version = "0.7.3", optional = true }
-viuer = { version = "0.9.1", optional = true }
+viuer = { version = "0.9.1", optional = true, git = "https://github.com/ollbx/viuer/", branch = "icy_sixel" }
 image = { version = "0.25.5", optional = true }
 notify-rust = { version = "4.11.3", optional = true, default-features = false, features = [
        "d",
@@ -87,7 +87,7 @@ gstreamer-backend = ["streaming", "librespot-playback/gstreamer-backend"]
 streaming = ["librespot-playback", "librespot-connect"]
 media-control = ["souvlaki", "winit", "windows"]
 image = ["viuer", "dep:image"]
-sixel = ["image", "viuer/sixel"]
+sixel = ["image", "viuer/icy_sixel"]
 notify = ["notify-rust"]
 daemon = ["daemonize", "streaming"]
 fzf = ["fuzzy-matcher"]

That being said, given that icy_sixel in Linux etc. is probably a worse choice than sixel (given the currently experimental nature). I would suggest a separate feature flag, similar to what I've done in the vuier project:

diff --git a/spotify_player/Cargo.toml b/spotify_player/Cargo.toml
index 5b0c33e..c93e5dd 100644
--- a/spotify_player/Cargo.toml
+++ b/spotify_player/Cargo.toml
@@ -41,7 +41,7 @@ tracing = "0.1.41"
 tracing-subscriber = { version = "0.3.19", features = ["env-filter"] }
 backtrace = "0.3.74"
 souvlaki = { version = "0.7.3", optional = true }
-viuer = { version = "0.9.1", optional = true }
+viuer = { version = "0.9.1", optional = true, git = "https://github.com/ollbx/viuer/", branch = "icy_sixel" }
 image = { version = "0.25.5", optional = true }
 notify-rust = { version = "4.11.3", optional = true, default-features = false, features = [
        "d",
@@ -88,6 +88,7 @@ streaming = ["librespot-playback", "librespot-connect"]
 media-control = ["souvlaki", "winit", "windows"]
 image = ["viuer", "dep:image"]
 sixel = ["image", "viuer/sixel"]
+icy_sixel = ["image", "viuer/icy_sixel"]
 notify = ["notify-rust"]
 daemon = ["daemonize", "streaming"]
 fzf = ["fuzzy-matcher"]
diff --git a/spotify_player/src/main.rs b/spotify_player/src/main.rs
index 3f65c5e..84b44ba 100644
--- a/spotify_player/src/main.rs
+++ b/spotify_player/src/main.rs
@@ -80,7 +80,7 @@ async fn start_app(state: &state::SharedState) -> Result<()> {
             // initialize `viuer` supports for kitty, iterm2, and sixel
             viuer::get_kitty_support();
             viuer::is_iterm_supported();
-            #[cfg(feature = "sixel")]
+            #[cfg(any(feature = "sixel", feature = "icy_sixel"))]
             viuer::is_sixel_supported();
         }
     }

@ollbx
Copy link

ollbx commented Dec 30, 2024

Another alternative could be to specify different dependencies for Windows and the rest, so that viuer is compiled with feature icy_sixel in Windows and sixel otherwise.

@juliamertz
Copy link
Contributor Author

@ollbx Thanks! I've applied your changes

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.

Cannot install with sixel lib
2 participants