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

spotify-tui: add collection variant patch #170915

Merged

Conversation

MoritzBoehme
Copy link
Contributor

@MoritzBoehme MoritzBoehme commented Apr 29, 2022

Description of changes

Add patch adding the collection variant to rspotify used by spotify-tui.
Rigellute/spotify-tui#960 (comment)

This fixes the issue of getting an error when playing liked songs. Rigellute/spotify-tui#960

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@jwijenbergh
Copy link
Contributor

It fails to build for me:

In this branch:

nix-build -A spotify-tui .

gives:

error: failed to sync

Caused by:
  failed to load pkg lockfile

Caused by:
  failed to resolve patches for `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to load source for dependency `rspotify`

Caused by:
  Unable to update /build/source/rspotify-0.10.0

Caused by:
  failed to read `/build/source/rspotify-0.10.0/Cargo.toml`

Caused by:
  No such file or directory (os error 2)

Traceback (most recent call last):
  File "/nix/store/r3nwnppm7n0fbnynfrx2q0dzjz5dgfar-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 42, in <module>
    main()
  File "/nix/store/r3nwnppm7n0fbnynfrx2q0dzjz5dgfar-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 17, in main
    assert list(data.keys()) == ["source"]
AssertionError
error: builder for '/nix/store/a721mj4bc6jzdiw319api0y74fm069mk-spotify-tui-0.25.0-vendor.tar.gz.drv' failed with exit code 1;
       last 10 log lines:
       >   failed to read `/build/source/rspotify-0.10.0/Cargo.toml`
       >
       > Caused by:
       >   No such file or directory (os error 2)
       > Traceback (most recent call last):
       >   File "/nix/store/r3nwnppm7n0fbnynfrx2q0dzjz5dgfar-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 42, in <module>
       >     main()
       >   File "/nix/store/r3nwnppm7n0fbnynfrx2q0dzjz5dgfar-cargo-vendor-normalise/bin/.cargo-vendor-normalise-wrapped", line 17, in main
       >     assert list(data.keys()) == ["source"]
       > AssertionError
       For full logs, run 'nix log /nix/store/a721mj4bc6jzdiw319api0y74fm069mk-spotify-tui-0.25.0-vendor.tar.gz.drv'.
error: 1 dependencies of derivation '/nix/store/3750ndflrd18gxkfrvwa024sxphza6a2-spotify-tui-0.25.0.drv' failed to build

Any idea why?

@MoritzBoehme
Copy link
Contributor Author

MoritzBoehme commented May 1, 2022

Oh I also had this error previously. I think because the rspotify-0.10.0 directory does not get copied correctly. But it is weird, that it builds on my machine. I'll clear my cached nix store builds and have another look...

@MoritzBoehme
Copy link
Contributor Author

Maybe you have a better idea how to patch rspotify and use it in the Cargo.lock. I haven't really done much with rust yet so I have limited experience.

@jwijenbergh
Copy link
Contributor

Maybe you have a better idea how to patch rspotify and use it in the Cargo.lock. I haven't really done much with rust yet so I have limited experience.

I am also honestly not sure how to do this, I don't have too much rust/nix packaging experience. I tried to find similar packages in nixpkgs that do this, but I couldn't find any. Simply replacing a rust crate with a patched crate doesn't seem like an odd thing to do, so I'm not sure why I can't figure it out

@MoritzBoehme MoritzBoehme force-pushed the spotify-tui-collection-patch branch from d96d752 to 0631c95 Compare May 2, 2022 08:09
@MoritzBoehme
Copy link
Contributor Author

Could you try again now?
Apparently a combination of cargoPatches and patches did the trick...

@MoritzBoehme
Copy link
Contributor Author

Result of nixpkgs-review pr 170915 run on x86_64-linux 1

1 package built:
  • spotify-tui

@jwijenbergh
Copy link
Contributor

Could you try again now? Apparently a combination of cargoPatches and patches did the trick...

works now using nixpkgs-review pr 170915. I guess cargoPatches is only for the lock file? https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.section.md#building-a-crate-with-an-absent-or-out-of-date-cargolock-file-building-a-crate-with-an-absent-or-out-of-date-cargolock-file

Anyways, I can't seem to reproduce the upstream issue of the inability to play liked song, they play fine for me. But I guess it depends on your spotify account?

If this patch fixes things for you then LGTM, it's better if upstream would move to a newer rspotify version though... But for now this will do.

@MoritzBoehme
Copy link
Contributor Author

MoritzBoehme commented May 2, 2022

works now using nixpkgs-review pr 170915. I guess cargoPatches is only for the lock file?

Yeah I thougt about that as well, but apparently they are just joined together if I read the source correctly. But hey if it works ¯\_(ツ)_/¯

Anyways, I can't seem to reproduce the upstream issue of the inability to play liked song, they play fine for me. But I guess it depends on your spotify account?

For me, it only occurred when I previously played my liked songs on my mobile phone or via the web client, and never through spotify-tui.

If this patch fixes things for you then LGTM, it's better if upstream would move to a newer rspotify version though... But for now this will do.

And I agree that an upstream update would be best, but as far as I understood rspotify had a lot of breaking changes, so it would take a considerable effort.

@jwijenbergh
Copy link
Contributor

Yeah I thougt about that as well, but apparently they are just joined together if I read the source correctly. But hey if it works ¯\_(ツ)_/¯

If it works it works!

For me, it only occurred when I previously played my liked songs on my mobile phone or via the web client, and never through spotify-tui.

Ah yes, can confirm that this fixes the issue. I could reproduce it by playing something on my phone in liked songs and then opening spotify-tui

And I agree that an upstream update would be best, but as far as I understood rspotify had a lot of breaking changes, so it would take a considerable effort.

Yeah exactly, rspotify 0.11.0 apparently has a lot of breaking changes/api changes.

Anyways thanks for the patch!

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/975

Comment on lines +15 to +20
cargoPatches = [
./Cargo.lock.patch
];
patches = [
./Cargo.toml.patch
];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cargoPatches = [
./Cargo.lock.patch
];
patches = [
./Cargo.toml.patch
];
cargoPatches = [
./Cargo.lock.patch
];

and I would suggest to combine the patch into one file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As described above, the derivation strangely won't build if the patches are applied through one patch file.

Copy link
Contributor

Choose a reason for hiding this comment

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

As @MoritzBoehme said, we have tried this already

pkgs/applications/audio/spotify-tui/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/spotify-tui/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from jwijenbergh July 8, 2022 09:45
Copy link
Contributor

@jwijenbergh jwijenbergh left a comment

Choose a reason for hiding this comment

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

Still looks good!

@SuperSandro2000 SuperSandro2000 merged commit fd00fef into NixOS:master Jul 14, 2022
@MoritzBoehme MoritzBoehme deleted the spotify-tui-collection-patch branch July 17, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants