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.

Sign up for GitHub

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

Jump to bottom

Remove unsafe code #940

Merged
merged 4 commits into from
Jan 23, 2022
Merged

Remove unsafe code #940

merged 4 commits into from
Jan 23, 2022

Conversation

JasonLG1979
Copy link
Contributor

As mentioned in #917 (comment).

@roderickvd
Copy link
Member

Ready?

@roderickvd roderickvd self-assigned this Jan 22, 2022
@roderickvd roderickvd added breaking includes a breaking change enhancement labels Jan 22, 2022
roderickvd
roderickvd reviewed Jan 22, 2022
View reviewed changes
Copy link
Member

@roderickvd roderickvd left a comment

Choose a reason for hiding this comment

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

Sorry for the mix-up but let's make sure this targets new-api so we can use the new ubiquitous librespot_core::Error type.

CHANGELOG.md Outdated Show resolved Hide resolved
core/src/spotify_id.rs Show resolved Hide resolved
@JasonLG1979
Copy link
Contributor Author

Sorry for the mix-up but let's make sure this targets new-api so we can use the new ubiquitous librespot_core::Error type.

I can do both dev and new-api so users of dev (me and Raspotify) can benefit since we have no ETA on the new-api branch.

There's also the logic that states that since dev is the main branch and new-api is a branch off of that, that it would be up to whoever (me) to manage the conflicts with the new-api branch when it is rebased.

My proposal is to merge this into dev and then I will rebase new-api and fix any conflicts.

@JasonLG1979
Copy link
Contributor Author

The general idea being that if code can benefit dev without major refactoring it should be merged into dev 1st and then new-api should be rebased.

@JasonLG1979
Copy link
Contributor Author

I'm also having an issue with compiling the new-api branch for armv6 (required for Pi 1 and Pi Zero). I can't remember the exact error off the top of my head and I haven't really looked that far into it yet but as it sits you can't use the make script in the Raspotify repo to build a armv6 binary like you can the dev branch. That will certainly affect other upstream devs if it turns out that the new-api branch simply just isn't compatible with armv6.

@roderickvd
Copy link
Member

I can do both dev and new-api so users of dev (me and Raspotify) can benefit since we have no ETA on the new-api branch.

I am considering doing an intermediate merge of new-api to dev soon-ish. So we can get all of the HTTP metadata and audio retrieval out there, before we take the plunge to rip out spirc and implement the new dealer. Symphonia should be releasing 0.5 soon with proper gapless support and I want to wait for that.

Also I am porting the recent librespot-java work for that extra metadata over HTTP.

There's also the logic that states that since dev is the main branch and new-api is a branch off of that, that it would be up to whoever (me) to manage the conflicts with the new-api branch when it is rebased.

My proposal is to merge this into dev and then I will rebase new-api and fix any conflicts.

OK, sounds good to me.

I'm also having an issue with compiling the new-api branch for armv6 (required for Pi 1 and Pi Zero). I can't remember the exact error off the top of my head and I haven't really looked that far into it yet but as it sits you can't use the make script in the Raspotify repo to build a armv6 binary like you can the dev branch. That will certainly affect other upstream devs if it turns out that the new-api branch simply just isn't compatible with armv6.

OK, let me know. Assuming that's the case (I don't have any of such devices) I don't know what I did to cause that.

@JasonLG1979
Copy link
Contributor Author

I am considering doing an intermediate merge of new-api to dev soon-ish. So we can get all of the HTTP metadata and audio retrieval out there, before we take the plunge to rip out spirc and implement the new dealer. Symphonia should be releasing 0.5 soon with proper gapless support and I want to wait for that.

Also I am porting the recent librespot-java work for that extra metadata over HTTP.

Sounds good.

OK, let me know. Assuming that's the case (I don't have any of such devices) I don't know what I did to cause that.

It's nothing you did directly one of the dependencies won't compile. It's nothing in librespot's actual code base.

Like I said I haven't really looked that hard into it and I haven't tried to compile that branch on an actual Pi Zero because it takes like 4 hours. It could very well be just a matter of rejiggering some tooling.

@roderickvd roderickvd merged commit ceebb37 into librespot-org:dev Jan 23, 2022
@roderickvd
Copy link
Member

Merged. As a suggestion for new-api, you may want to again remove the unsafe blocks and then do the fix-until-it-compiles dance, instead of trying to merge this. The player event handler didn't change, but metadata did, and I don't think it would be pretty. But it's just a suggestion.

@JasonLG1979
Copy link
Contributor Author

Merged. As a suggestion for new-api, you may want to again remove the unsafe blocks and then do the fix-until-it-compiles dance, instead of trying to merge this. The player event handler didn't change, but metadata did, and I don't think it would be pretty. But it's just a suggestion.

My general mode of operation is basically what you describe.

@JasonLG1979 JasonLG1979 deleted the remove-unsafe branch February 15, 2022 01:24
michaelherger added a commit to michaelherger/librespot that referenced this pull request Feb 17, 2022
@michaelherger
Merge branch 'spotty-dev' into spotty
bb1f917 
* spotty-dev: (76 commits)
  Quantum-realm level normalisation optimization (librespot-org#965)
  Only log runtime argument if it starts with a dash "-"
  Improved error handling when fetching tokens or track data.
  update changelog
  Prevent shuffle crash
  Remove basic normalisation deprecation warning
  Fix Alsa softvol linear mapping (librespot-org#950)
  simplify get_factor (librespot-org#942)
  Update LMS integration to
  Remove unsafe code (librespot-org#940)
  Save some more CPU cycles in the limiter (librespot-org#939)
  New dynamic limiter for very wide dynamic ranges (librespot-org#935)
  Fix `--device` argument to various backends (librespot-org#938)
  examples/playlist_tracks: Use normal URI parser
  Fix clippy lint warning
  Clean up list_compatible_devices
  Sink: pass ownership of the packet on write()
  Restore giving feedback about auth success. The plugin is checking for this.
  Remove that last couple unwraps from main
  Fix auto fallback for --alsa-mixer-device and --alsa-mixer-index
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roderickvd roderickvd roderickvd left review comments

Assignees

@roderickvd roderickvd

Labels
breaking includes a breaking change enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JasonLG1979 @roderickvd