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

Remove and update dependencies #1140

Merged
merged 10 commits into from
Apr 11, 2023

Conversation

yubiuser
Copy link
Contributor

@yubiuser yubiuser commented Apr 1, 2023

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:

  • vergen does not have thevergen::Config structure since 8.0 anymore, but uses vergen::EmitBuilder now.
  • rsa does not have the hash module anymore since 0.7.0, but uses OIDs
  • base64 changed encode* and decode* top level functions to methods of engine since 0.20
  • quick-xml failed with a few compiling errors

Signed-off-by: Christian König <[email protected]>
Signed-off-by: Christian König <[email protected]>
Signed-off-by: Christian König <[email protected]>
@yubiuser
Copy link
Contributor Author

yubiuser commented Apr 1, 2023

Needed to increase MSRV to 1.63

@yubiuser
Copy link
Contributor Author

yubiuser commented Apr 1, 2023

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

@yubiuser yubiuser force-pushed the remove_dependencies branch from b21788c to 79ecf51 Compare April 1, 2023 14:26
Signed-off-by: Christian König <[email protected]>
@yubiuser yubiuser force-pushed the remove_dependencies branch from 79ecf51 to cd1f16e Compare April 1, 2023 14:39
yubiuser added 2 commits April 1, 2023 20:33
Signed-off-by: Christian König <[email protected]>
Signed-off-by: Christian König <[email protected]>
@yubiuser
Copy link
Contributor Author

yubiuser commented Apr 1, 2023

Needed to increase MSRV to 1.64

@yubiuser yubiuser force-pushed the remove_dependencies branch 8 times, most recently from 8746206 to 8ad5d56 Compare April 2, 2023 14:03
Copy link
Contributor

@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.

Only some things that I noticed. :)

core/src/authentication.rs Outdated Show resolved Hide resolved
playback/src/decoder/passthrough_decoder.rs Outdated Show resolved Hide resolved
playback/src/decoder/passthrough_decoder.rs Outdated Show resolved Hide resolved
@yubiuser
Copy link
Contributor Author

yubiuser commented Apr 2, 2023

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 yubiuser force-pushed the remove_dependencies branch 2 times, most recently from 1be0ac9 to e3771e5 Compare April 2, 2023 18:57
@yubiuser yubiuser force-pushed the remove_dependencies branch from e3771e5 to 1e4c324 Compare April 2, 2023 19:09
Signed-off-by: Christian König <[email protected]>
@yubiuser yubiuser force-pushed the remove_dependencies branch from db359a9 to f4ec772 Compare April 2, 2023 20:30
Signed-off-by: Christian König <[email protected]>
@yubiuser yubiuser force-pushed the remove_dependencies branch from 8fe0eb2 to 379c127 Compare April 2, 2023 20:59
@yubiuser
Copy link
Contributor Author

yubiuser commented Apr 2, 2023

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

Signed-off-by: Christian König <[email protected]>
@eladyn
Copy link
Contributor

eladyn commented Apr 3, 2023

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
Copy link
Contributor Author

yubiuser commented Apr 4, 2023

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
Copy link
Member

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
Copy link
Contributor Author

yubiuser commented Apr 4, 2023

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Ah OK! So I'll merge this then 👍

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!

@roderickvd roderickvd merged commit e14dac3 into librespot-org:dev Apr 11, 2023
@yubiuser yubiuser deleted the remove_dependencies branch April 11, 2023 18:35
This was referenced Apr 11, 2023
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.

3 participants