Cleanup

[marioortizmanero]

Description

This cleans up quite a few things:

• Removes unnecessary dependencies
• Uses debug_assert instead of log to ensure endpoint predicates are met; some people don't enable logging at all
• Clean up unnecessary derive macros.
• Move all Serialize/Deserialize items from src to rspotify-model, like Token, where they fit better. This forced me to create a ModelError, but I don't really see the problem.
• Move custom [de]serialization modules into custom_serde and fix pub(in crate) occurences
• Token::from_cache now returns a Result instead of an Option, as the TODO above indicated.
• Bumps all the dependencies

Motivation and Context

I'm working on the last things needed to finally release v0.11. This is one of them :)

I got some of these suggestions from this very nice article, give it a read yourself: https://matklad.github.io/2021/09/04/fast-rust-builds.html

Dependencies

None, if anything I've removed some.

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

The CI should also pass after this

[marioortizmanero]

I've ran some compilation profiling with the following command:

cargo +nightly build -Z timings --release

And got these results:

cargo-timing.zip

---

I also ran cargo llvm-lines --release | head -n 12 and got this:

  Lines          Copies       Function name
  -----          ------       -------------
  120081 (100%)  4257 (100%)  (TOTAL)
    2441 (2.0%)     7 (0.2%)  <serde_json::de::SeqAccess<R> as serde::de::SeqAccess>::next_element_seed
    1920 (1.6%)     3 (0.1%)  <&mut serde_json::de::Deserializer<R> as serde::de::Deserializer>::deserialize_struct
    1781 (1.5%)    29 (0.7%)  alloc::alloc::box_free
    1558 (1.3%)     2 (0.0%)  rspotify::clients::base::BaseClient::fetch_access_token::{{closure}}
    1524 (1.3%)     2 (0.0%)  serde_json::de::Deserializer<R>::ignore_value
    1399 (1.2%)    29 (0.7%)  <core::result::Result<T,E> as core::ops::try_trait::Try>::branch
    1363 (1.1%)     5 (0.1%)  <&mut serde_json::de::Deserializer<R> as serde::de::Deserializer>::deserialize_str
    1245 (1.0%)     1 (0.0%)  hyper::body::to_bytes::to_bytes::{{closure}}
    1200 (1.0%)     2 (0.0%)  <<rspotify_model::error::_::<impl serde::de::Deserialize for rspotify_model::error::ApiError>::deserialize::__Visitor as serde::de::Visitor>::visit_enum::__Visitor as serde::de::Visitor>::visit_map

---

Just in case you're curious :)

[marioortizmanero]

I've updated the dependencies with cargo upgrade. But I haven't seen dependabot in a long time. Is it still working, @ramsayleung ?

[marioortizmanero]

Here are the results of this PR, run them yourself as well if you want to make sure:

cargo test --release (with dev-dependencies)

• Before:
  • reqwest: 175 compilation units, 2m 24s
  • ureq: 142 compilation units, 1m 50s
• After:
  • reqwest: 169 compilation units, 2m 27s
  • ureq: 133 compilation units, 1m 27s

cargo build --release

• Before:
  • reqwest: 159 compilation units, 1m 2s
  • ureq: 120 compilation units, 55s
• After:
  • reqwest: 159 compilation units, 1m 21s
  • ureq: 102 compilation units, 52s

So in the end the cleanup does help a bit in terms of compilation time. The Cargo.toml and similars are much tidier, too. I'm done, this is ready for review as well.

[ramsayleung]

I got some of these suggestions from this very nice article, give it a read yourself: matklad.github.io/2021/09/04/fast-rust-builds.html

Yes, I have seen this post in the /r/rust, and took a rough glance to this article. I would like to read it later.

And I am not sure if it's necessary to introduce bors into rspotify to set up some workflows, as Aleksey suggested?

But I haven't seen dependabot in a long time. Is it still working, @ramsayleung ?

It should work, I haven't changed anything of this bot.

So in the end the cleanup does help a bit in terms of compilation time

It seems the compilation time of debug build and release build for reqwest both grow a bit.

[ramsayleung]

Since you have replaced log with debug_assert, is it possible to remove this crate?

[marioortizmanero]

I was going to remove it but I think it's still useful to have around. I wanted to add some log statements before releasing the new version because we don't even log that much right now. But I wanted to wait for the pending PRs to be merged first.

[marioortizmanero]

Also, nah I wouldn't introduce bors. It seems complicated and we're a small team so not really necessary.

Reqwest might be a bit slower but that's just the benchmark innaccuracy; I only did one run. There's no reason the build times should increase otherwise. As you can see there are less dependencies and its tree allows for more parallelization. Perhaps the benchmark wasn't the best idea.

[ramsayleung]

Is it possible to benchmark with CI, just treating every check PR as build benchmark. Thus, we could check the last CI build as the before benchmark and current build as after benchmark?

[marioortizmanero]

I mean you can always take a look at how long it takes the CI to finish, but it's just as inaccurate as doing it manually because there's only one run.

• Last commit in this PR:
  • https://github.com/ramsayleung/rspotify/runs/3524321817 ureq: 2m 49s
  • https://github.com/ramsayleung/rspotify/runs/3524321899 reqwest: 4m 4s
• Last commit in master:
  • https://github.com/ramsayleung/rspotify/runs/3531367936 ureq: 2m 49s
  • https://github.com/ramsayleung/rspotify/runs/3531367978 reqwest: 4m 35s

The thing is that even if the changes in this PR don't improve compile times, it's still an improvement because it tidies up the Cargo.toml files and similars.

[ramsayleung]

Would you like to remove the log crate, otherwise I think this PR is ready to merge?

[marioortizmanero]

I'll add some more logs to the main crate after the rest of the PRs are merged, so we'll need it (I've added it to the meta-issue). I'll merge then, thanks!