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

Windows support #602

Closed
wants to merge 4 commits into from
Closed

Windows support #602

wants to merge 4 commits into from

Conversation

JCapucho
Copy link
Contributor

This PR allows spotifyd to run on windows as non-daemon and daemon-like, only the rodio-backend is supported and the config file is stored on:

%AppData%/Roaming/spotifyd/spotifyd.conf

All output is logged into a .spotifyd.log file on windows when running in both non-daemon and daemon mode although in daemon mode the startup is logged to the console before a detached subprocess with the same binary and arguments (with the exception that the argument --no-daemon is set) is started

@mainrs mainrs added this to the v1.0.0 milestone Jun 11, 2020
@mainrs
Copy link
Member

mainrs commented Jun 11, 2020

As this is (compared to other PRs) larger change I'll add it to the v1 milestone for now. This doesn't mean however that it won't get merged beforehand.

@MartB
Copy link

MartB commented Jun 15, 2020

This seems to work perfectly fine on windows, just needs a rebase.

@JCapucho
Copy link
Contributor Author

I rebased the branch and improved the logging now --no-daemon will use the terminal and the daemon will use the file but the file will be stored next to the config file instead of the current working directory this makes it easier to use windows services

@mainrs
Copy link
Member

mainrs commented Jun 17, 2020

Thanks for that! I will merge this after I can update the librespot dependency. I will take a closer look at it then and do a proper code review. I just skimmed through it for now.

@mainrs mainrs self-assigned this Jun 17, 2020
@mainrs mainrs mentioned this pull request Jul 11, 2020
6 tasks
@mainrs mainrs mentioned this pull request Aug 21, 2020
@Plecra Plecra mentioned this pull request Sep 25, 2020
@Plecra
Copy link
Contributor

Plecra commented Sep 25, 2020

Just want to add that there's currently a runtime dependency on libogg (from with-tremor), and I wouldn't expect to find it on most windows systems. Users will have to do something about this if they want to use spotifyd.

Edit: Skimmed through the implementation, and it looks like tremor and libogg are alternatives to lewton. Is there a reason we couldn't switch to the Rust implementation?

@Plecra
Copy link
Contributor

Plecra commented Sep 27, 2020

Also, why aren't we using the windows services API for this? It seems to fit the role best.

@JCapucho
Copy link
Contributor Author

JCapucho commented Sep 27, 2020

I'm longer working on this and don't plan on working on it since I finally got rid of windows on the last machine I used that had it, if my memory is right I didn't use services because they are hard to get right and require special permissions while this method doesn't

@ekx
Copy link

ekx commented Sep 30, 2020

Tried this out today. Seems to work well so far... Found one Issue: When I switch the audio output devices while spotifyd is running it keeps playing on the old device. Tested on latest Windows 10

@CptPotato
Copy link

A small issue I ran into is that I can't get the password_cmd option to work on windows. Other than that it is working fine for me 👍

@Runeii
Copy link

Runeii commented Oct 5, 2020

Seems that on Windows (10) the following commands work:

cargo build --release --no-default-features --features rodio_backend
cargo run --no-default-features --features rodio_backend

And this PR will build/run completely fine. However, it's not currently possible to install it using:

cargo install --path . --locked
cargo install --path . --locked --no-default-features --features rodio_backend

As it complains about:

error: failed to run custom build command for 'alsa-sys v0.1.2'

Which suggests to me it's ignoring the rodio_backend build flag.

@stale
Copy link

stale bot commented Jan 13, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix Issues that will not be fixed under any circumstances label Jan 13, 2021
@slondr slondr added the pinned Apply to make stale bot ignore issue/PR. label Jan 17, 2021
@stale stale bot removed the wontfix Issues that will not be fixed under any circumstances label Jan 17, 2021
@zoeleu
Copy link

zoeleu commented Mar 6, 2021

Why is this dead? This is a great feature. I want to have spotify running HQ, without the desktop client.

@Brin-o
Copy link

Brin-o commented Mar 8, 2021

Hi, just want to express my support for getting this pr in. It would be really cool to get a lightweight spotify client for windows.

@StaticPH
Copy link

StaticPH commented Mar 8, 2021

I can confirm that with a bit of finagling, the code from the PR branch runs on Windows 8.1
To be clear, this means that the working build I have is based off the version 0.2.24 codebase, and not a more recent version with JCapucho's changes patched in; I don't know if simply applying the changes to the current codebase would yield a working build, and I didn't particularly need anything from the 3.0.x release, so I never tried.

I found that I had to specify limited version ranges for librespot, protobuf-codegen, and protobuf-codegen-pure in Cargo.toml

[dependencies.librespot]
version = ">= 0.1.1, < 0.1.2"
default-features = false
features = ["with-tremor"]

[build-dependencies.protobuf-codegen]
version = "~2.8.1, < 2.9.0"

[build-dependencies.protobuf-codegen-pure]
version = "~2.8.1, < 2.9.0"

I think I might have also needed to manually update the manifest and lockfiles for librespot accordingly, and build it with --features="rodio-backend,with-tremor", but its been several weeks and its not clear from my command history if I did or not.

I definitely had to regenerate/update the lockfile for spotifyd itself, making sure that it listed acceptable versions for protobuf-codegen, protobuf-codegen-pure, and the various librespot crates. I probably did that with cargo generate-lockfile --offline, but again, my command history doesn't indicate whether or not I did it by hand.

To build spotifyd, I had to use the MSVC toolchain rather than GNU toolchain, as I found that no matter what I did, the latter either failed to build, built an executable that simply didn't work, or built an executable I couldn't run; your results may differ.
The actual command I used is rustup run 1.47.0-x86_64-pc-windows-msvc cargo build --release --no-default-features --features="rodio_backend" --manifest ~/.cargo/git/checkouts/spotifyd-36f4565cca355e0b/d119fc3/Cargo.toml.
It may or may not also be possible with the right setup to build and run with the dbus_keyring and/or dbus_mpris features, but I didn't attempt that because just getting spotifyd to run at all was enough for me.

@ghost ghost mentioned this pull request Mar 18, 2021
@vincenttermaat
Copy link

What needs to be done to get this merged?

@robinvd
Copy link
Contributor

robinvd commented Jun 21, 2021

It should atleast be updated to the current master.

@lamaland
Copy link

lamaland commented Sep 6, 2021

Hi there, any help needed here? Would love to run this daemon on Windows.
Thanks for working on this.

@vincenttermaat
Copy link

I am using librespot instead

@CypherpunkSamurai
Copy link

Is this PR still active?

@DomingoMG
Copy link

Is something known?

@Katzenwerfer
Copy link

I believe it is currently preferred to use librespot (which was linked three comments above)

@brian6932
Copy link

I built this, and after playing with it for a bit, managed to get it to work.
For anyone on Windows that is getting:

Caught panic with message: attempted to zero-initialize type `librespot_tremor::tremor_sys::ov_callbacks`, which is invalid

Remove librespot's with-tremor feature flag in Cargo.toml
Specifically the 2nd thing here: #719 (comment)
The other methods of getting it to work did not work for me
Can someone please rebase this already, so that we don't have to do these annoying workarounds?

@slondr
Copy link
Member

slondr commented Mar 11, 2022

@Katzenwerfer @DomingoMG @CypherpunkSamurai
This PR is fairly old, and has merge conflicts which aren't resolved, plus the author has already disclaimed it. Finally, I'm not sure any maintainer uses Windows to test this.

If someone could get this into a working, mergeable state, I may be able to throw together a Windows box to try it.

Copy link

@hackedXD hackedXD left a comment

Choose a reason for hiding this comment

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

test

@CypherpunkSamurai
Copy link

We need two reviews for this PR to pass. I'll try cloning the branch and testing it out. If anyone else got time please do.

Copy link

@RakeshChowdhury RakeshChowdhury left a comment

Choose a reason for hiding this comment

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

LGTM

@eladyn
Copy link
Member

eladyn commented Apr 29, 2023

@RakeshChowdhury As has been pointed out in this comment, this PR currently is not mergeable and - up until now - no one has stepped up to fix the conflicts. So unless someone with interest shows up to get it working, this PR is unfortunately not going to happen. :/

@catumin
Copy link
Contributor

catumin commented Aug 25, 2023

My branch here https://github.com/BKasin/spotifyd/tree/windows is rebased against the current commit of master, and seems to be working. Config file paths could definitely be handled better, and I'll have to research windows daemons/services if a better solution for that is desired. But as things stand it seems to work completely fine.


What is the preferred way for me to handle getting that branch ready? Should I open a PR or should this one be re-targeted?

image

@eladyn
Copy link
Member

eladyn commented Aug 30, 2023

@BKasin thank you for working on this! I haven't had a look yet, but opening a new PR seems like the easiest way forward.
I'm currently not at home (and won't be for some time), but I'll try to at least give feedback an answer any questions you have.

@catumin catumin mentioned this pull request Aug 30, 2023
@catumin
Copy link
Contributor

catumin commented Oct 20, 2023

@eladyn can probably close this now that #1219 is merged.

@eladyn eladyn closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pinned Apply to make stale bot ignore issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.