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

Spotifyd rust packages #114309

Merged
merged 1 commit into from
Feb 25, 2021
Merged

Spotifyd rust packages #114309

merged 1 commit into from
Feb 25, 2021

Conversation

jshcmpbll
Copy link
Member

@jshcmpbll jshcmpbll commented Feb 25, 2021

Motivation for this change

Spotifyd v0.3.0 does not work - Caught panic with message: attempted to zero-initialize type `librespot_tremor::tremor_sys::ov_callbacks`, which is invalid Caught panic with message: called `Result::unwrap()` on an `Err` value: "SendError(..)" Player thread panicked!

According to this issue its due to the latest rust version. Pining the version of rust back to rustPackages_1_45 fixes this error.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Feb 25, 2021

Result of nixpkgs-review pr 114309 at dcccf49c run on aarch64-linux 1

1 package built:
1 suggestion:
  • warning: unclear-gpl

    gpl3 is a deprecated license, check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/applications/audio/spotifyd/default.nix:42:5:

       |
    42 |     license = with licenses; [ gpl3 ];
       |     ^
    

Result of nixpkgs-review pr 114309 at dcccf49c run on x86_64-linux 1

1 package built:
1 suggestion:
  • warning: unclear-gpl

    gpl3 is a deprecated license, check if project uses gpl3Plus or gpl3Only and change meta.license accordingly.

    Near pkgs/applications/audio/spotifyd/default.nix:42:5:

       |
    42 |     license = with licenses; [ gpl3 ];
       |     ^
    

@jshcmpbll jshcmpbll force-pushed the spotifyd-rustPackages branch from dcccf49 to 1ef5249 Compare February 25, 2021 03:59
@Mic92 Mic92 merged commit 770b6d8 into NixOS:master Feb 25, 2021
@sarcasticadmin sarcasticadmin mentioned this pull request Feb 26, 2021
10 tasks
@andir
Copy link
Member

andir commented Feb 27, 2021

Just to add some historical context here (who knows how long this will continue to happen):
I first applied the fix that was applied here (again) in #106050 which got removed in #109681 for no reason that I can see. We now also lack any kind of documentation in the source code why that older rustc is desired. History tells me someone will try to compile with the latest and greatest, sees no compiler errors and will open a PR that will bump the version again.

@jshcmpbll
Copy link
Member Author

@andir Very good point and I apologize for not adding a comment in the code for future contributors to validate the package not just compiles but works.

I was talking with @sarcasticadmin yesterday and he mentioned there should be some unit testing around this to prevent these issues in the future. I don't have much experience in that but its definitely something I'd like to learn more about and am willing to give it a shot.

@andir
Copy link
Member

andir commented Feb 27, 2021

As this requires a closed source backend I am not sure how to validate it outside of the sandbox.

@bnjmnt4n
Copy link
Contributor

bnjmnt4n commented Mar 4, 2021

Just to update: it seems like a version which fixes the original issue has been merged and released as v0.3.1 (see Spotifyd/spotifyd#900), although we might want to wait for Spotifyd/spotifyd#912 before updating.

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