search

librespot-org / librespot

Open Source Spotify client library
MIT License
4.52k stars 544 forks source link

Remove and update dependencies #1140

Closed yubiuser closed 1 year ago

yubiuser commented 1 year ago

I used cargo-udeps from here to scan for unused dependencies. Two package were removed, num and glob. With cargo upgrades from here I scanned for upgradable dependencies in the Cagro.toml files. I updated them (and regenerated the Cargo.lock) execept of

librespot-core: /core/Cargo.toml
    base64 matches 0.13.1;  latest is 0.21.0
    quick-xml matches 0.23.1;   latest is 0.28.1
    rsa matches 0.6.1;  latest is 0.8.2
    vergen matches 7.5.1;   latest is 8.1.0

librespot-discovery: /discovery/Cargo.toml
    base64 matches 0.13.1;  latest is 0.21.0

because they require changes to the code beyond my rust knowledge. I tracked them down as far as I could - hopefully someone else can continue here:

yubiuser commented 1 year ago

Needed to increase MSRV to 1.63

yubiuser commented 1 year ago

pbkdf2 uses a wrapper now which returns a result which need to be handled.

yubiuser commented 1 year ago

Needed to increase MSRV to 1.64

yubiuser commented 1 year ago

Thanks for your review. __

I'm not sure, why you're trying to change that into single u8 values.

Because this is trial and error. As I said above:

changes to the code beyond my rust knowledge

_

I'll try to get it working but I might need more help on that.

yubiuser commented 1 year ago

This PR consists of 9 commits - do you want me to squash them down?

eladyn commented 1 year ago

Regarding the gstreamer update, I rememberer that there was #1067 before, which seems to improve the code by avoiding all the expects, so maybe that could be considered.

yubiuser commented 1 year ago

I have no problem with including this change even if it does not have the same aim as this PR. However, I don't know which "PR-rule" this project prefers: I'm used to "One PR does one thing". But happy to here from reviewers/maintainers.

roderickvd commented 1 year ago

Thanks a lot for your work on this!

Generally we prefer "isolated" PRs. To me this PR fits just fine: it's all about a cargo update (OK, so a little more 😄) Please don't squash now, but keep the commit log so it's easier to review deltas. Then at the end we can squash easily.

Let me know when you are "done" for a final review or need some help anywhere.

yubiuser commented 1 year ago

I think I'm done :-) As I said, there are some not updated dependencies but resolving the resulting compile error is out of my capabilities.

__

OK, so a little more

Only what's necessary to resolve the errors resulting from the changes made in the dependencies.

roderickvd commented 1 year ago

What errors are those? The CI passes?

Sorry if I sound redundant -- I am not super active here anymore so am not following all discussions, but want to keep this project breathing anyway.

yubiuser commented 1 year ago

The CI passes with all changes I made. But as I wrote in my PR description, some dependencies are not updated, because they will cause compilation errors. (They do not occur in this PR as I did not updated those).

I updated them (and regenerated the Cargo.lock) execept of

librespot-core: /core/Cargo.toml base64 matches 0.13.1; latest is 0.21.0 quick-xml matches 0.23.1; latest is 0.28.1 rsa matches 0.6.1; latest is 0.8.2 vergen matches 7.5.1; latest is 8.1.0

librespot-discovery: /discovery/Cargo.toml base64 matches 0.13.1; latest is 0.21.0

roderickvd commented 1 year ago

Ah OK! So I'll merge this then :+1:

If you want any help on the others, feel free to continue on Gitter, or maybe even open a draft PR here with the compiling errors in so others can help you out.

Thanks for your effort!