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

Librespot upgrade to 0.4 #1182

Merged
merged 1 commit into from
Mar 22, 2023
Merged

Librespot upgrade to 0.4 #1182

merged 1 commit into from
Mar 22, 2023

Conversation

Icelk
Copy link
Contributor

@Icelk Icelk commented Mar 8, 2023

Upgrades librespot to 0.4

Considerations

  • Audio normalization: I'm unsure of the changes, but I think we could improve clipping with normilazation when using softvol
  • Credentials: librespot now seems to want to handle credentials storage by itself. Should we hand over this job?

@Icelk
Copy link
Contributor Author

Icelk commented Mar 8, 2023

This seems to be blocking on rust 1.63. Tomorrow, a new rust version will release, so maybe we could up our required version? (if you have a MSRV policy of 5 stable releases back)

@Icelk Icelk mentioned this pull request Mar 8, 2023
Merged
@eladyn
Copy link
Member

eladyn commented Mar 8, 2023

Oh, cool! I only had a quick look and am surprised that the amount of changes is not as big as I feared (given the major version jump 0.2 → 0.4).

This seems to be blocking on rust 1.63. Tomorrow, a new rust version will release, so maybe we could up our required version? (if you have a MSRV policy of 5 stable releases back)

I don't think, we have a strict MSRV policy. The closest to such a thing is probably #1123 (comment). I think, bumping to 1.63 or 1.64 should be totally fine. You'll just need to change that in Cargo.toml as well as in CI.

@Icelk
Copy link
Contributor Author

Icelk commented Mar 8, 2023

Oh, cool! I only had a quick look and am surprised that the amount of changes is not as big as I feared (given the major version jump 0.2 → 0.4).

I know! Hopefully, once v0.5 lands, the upgrade will be easier.

I don't think, we have a strict MSRV policy. The closest to such a thing is probably #1123 (comment). I think, bumping to 1.63 or 1.64 should be totally fine. You'll just need to change that in Cargo.toml as well as in CI.

Nice, I'll just do that. The 1.62 recommendation came from a long time ago, so I think it's OK to bump two versions (12 weeks).

@slondr
Copy link
Member

slondr commented Mar 9, 2023 •
edited
Loading

Just to weigh in on the MSRV discussion: my suggestion of using Gentoo Stable as a heuristic would currently put us at Rust 1.66.1; of course, if we don't actively have a reason to go beyond a specific version, keeping to an older version is fine.

So, I'm ok with us bumping to 1.64 for this PR.

slondr
slondr requested changes Mar 9, 2023
View reviewed changes
Copy link
Member

@slondr slondr left a comment

Choose a reason for hiding this comment

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

The changes made look good to me, and the checks are passing.

@Icelk, could you address the merge conflicts? They're from the "none" audio controller commit, I believe.

W/r/t the normalization or credentials features, I'd be interested to see what those would look like in practice. For credentials specifically it's interesting to imagine some of our codebase downsizing, but naturally we'd want to ensure we aren't losing functionality.

@slondr slondr linked an issue Mar 9, 2023 that may be closed by this pull request
spotifyd does not build reproducibly #1167
Closed
@slondr slondr added this to the v0.3.5 milestone Mar 9, 2023
@Icelk
Copy link
Contributor Author

Icelk commented Mar 9, 2023

@Icelk, could you address the merge conflicts? They're from the "none" audio controller commit, I believe.

Yes, I just did :)

W/r/t the normalization or credentials features, I'd be interested to see what those would look like in practice. For credentials specifically it's interesting to imagine some of our codebase downsizing, but naturally we'd want to ensure we aren't losing functionality.

Once this is merged, I can try removing our credentials logic and share the process. The cache format / persistence doesn't need to be defined? Because we get the password and username on startup, so the cache is only for speed? Why don't we just keep it in memory?

slondr
slondr previously approved these changes Mar 9, 2023
View reviewed changes
eladyn
eladyn requested changes Mar 9, 2023
View reviewed changes
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Thank you again for working on this! I added some comments, but overall this looks good.

src/config.rs Outdated Show resolved Hide resolved
src/dbus_mpris.rs Outdated Show resolved Hide resolved
src/main_loop.rs Show resolved Hide resolved
src/setup.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/main_loop.rs Outdated Show resolved Hide resolved
src/main_loop.rs Outdated Show resolved Hide resolved
src/no_mixer.rs Outdated Show resolved Hide resolved
@eladyn
Copy link
Member

eladyn commented Mar 9, 2023

I noticed that librespot apparently also added an AlsaMixer, which seems to handle a lot of ALSA weirdness and many edge cases. Maybe we could switch to that at some point, to reduce our code size. (Doesn't need to be this PR, though.)

@Icelk Icelk mentioned this pull request Mar 11, 2023
Merged
@slondr slondr added the dependencies Pull requests that update a dependency file label Mar 11, 2023
@Icelk
Copy link
Contributor Author

Icelk commented Mar 15, 2023

I noticed that librespot apparently also added an AlsaMixer, which seems to handle a lot of ALSA weirdness and many edge cases. Maybe we could switch to that at some point, to reduce our code size. (Doesn't need to be this PR, though.)

Another thing I could look into in the future :)

@Icelk
Copy link
Contributor Author

Icelk commented Mar 15, 2023

Thanks for the feedback! Everything you mentioned is fixed, and I rebased.

@Icelk Icelk requested review from eladyn and slondr and removed request for eladyn and slondr March 15, 2023 17:55
@eladyn eladyn mentioned this pull request Mar 16, 2023
Closed
@slondr slondr linked an issue Mar 22, 2023 that may be closed by this pull request
error when compiling #1190
Closed
slondr
slondr requested changes Mar 22, 2023
View reviewed changes
Copy link
Member

@slondr slondr left a comment

Choose a reason for hiding this comment

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

Now that #1183 and #1145 are landed—the latter containing the MSRV bump—could you rebase this on master one more time @Icelk? I think we should be good to go thereafter

@Icelk
Copy link
Contributor Author

Icelk commented Mar 22, 2023

@slondr Just rebased. Thanks for merging #1183 & #1145!

slondr
slondr approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member

@slondr slondr left a comment

Choose a reason for hiding this comment

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

Awesome!

eladyn
eladyn approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member

@eladyn eladyn left a comment

Choose a reason for hiding this comment

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

Thank you, merging! 🚀

@eladyn eladyn merged commit 36b7f33 into Spotifyd:master Mar 22, 2023
@Icelk Icelk deleted the librespot-upgrade branch March 22, 2023 14:33
@Icelk
Copy link
Contributor Author

Icelk commented Mar 22, 2023

Awesome, thanks!

@ulbi ulbi mentioned this pull request Mar 26, 2023
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladyn eladyn eladyn approved these changes

@slondr slondr slondr approved these changes

Assignees

@Icelk Icelk

Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
v0.3.5
Development

Successfully merging this pull request may close these issues.

error when compiling spotifyd does not build reproducibly
3 participants
@Icelk @eladyn @slondr