Refactor into rustic_core library, rustic cli library and rustic application part

[simonsan]

Fixes #461

Todo

Library

• replace removed anyhow result with suited ErrorKinds
  • mostly done, but rustic_core/backend/stdin.rs is missing
• go through ErrorKinds that contain ErrorKindMember(#[from] AnotherError) and check if we can handle them within code and use our own Error for it
• replace anyhow bail! macro at places with returning library errors
  • research: which errors does the library part expose? (search for usage of bail! macro)
• introduce feature flags for clap, merge -> combine them under cli feature flag
• refactor/reimplement logging/tracing

Public API

• Added a integration test case for the public API: https://github.com/Enselic/cargo-public-api#-as-a-ci-check
  • run with:
    • Linux: export UPDATE_EXPECT=1; cargo test public_api -p rustic_core
    • Windows (Powershell): $env:UPDATE_EXPECT=1; cargo test public_api -p rustic_core
• https://www.lurklurk.org/effective-rust/re-export.html
• https://web.archive.org/web/20230128000646/http://www.tglman.com/posts/rust_lib_error_management.html

CLI

• reimplement commands based on Runnable trait
  • check src/commands/start.rs for implementation details
• implement config file
• implement merging strategies
• check helpers.rs for issues to be tackled

CI

• Check workflows if and how they need adaptation
• create workflow for public api check
  • utilizing existing public_api integration test for various OS

Follow up

• progress bars: how to only in CLI not in library (trait ProgressExt)
  • maybe only implement them in CLI and have a progress trait in library, that certain structs can implement, so if something in that regards is needed, each struct can expose their own progress/status
• check for non-consumed parameters (activate clippy lints)
• move outputs of e.g. println! and eprintln! to structs
• check let _ bindings where we throw away the result. we should probably check the results and handle possible errors

Public API

• document public API well
• https://deterministic.space/elegant-apis-in-rust.html
• https://rust-lang.github.io/api-guidelines/

CI

• determine workflow for releasing packages
  • e.g. with https://github.com/marcoIeni/release-plz
  • or cargo-smart-release

Research

empty

Notes

• 2023-05-02: we discussed goals of this PR and limited the scope to:
  • splitting the library and cli part
  • implement the error handling needed
  • reimplement the commands on abscissa framework
  • check if logging is working
  • implement overriding/merging config internally
  • we have discussed as well, that the library won't be made public (as in published to crates.io for now), as it needs to be further refined after the split

[simonsan]

@aawsome So this would be a really early stage how my approach would be. It's not in a working state right now, though.

[aawsome]

Hi @simonsan
Just had a short glance at abscissa but I think I didn't yet fully get how the framework exactly works.
What I think I understood:

• we should use library error handling like you already started to implement (Thanks a lot!)
• logging should be already working as abscissa supports the log crate
• the CLI command structure is already pretty good, everything already lives under src/commands
• abscissa uses a config, pretty similar to what rust uses in src/commands/configfile.rsand mainly some renaming/moving must be done.
• It is right that rustic currently uses the same structs for defining config file sections and CLI options and therefore derives clap::Parser as well as Deserialize. I think we should keep this in the first run. We can later decide if we should change something. The good point is that the commands which do config overwrites have all structs which also derive merge::Merge and therefore config::Overwrite can be easily implemented by just calling merge from the Merge trait.

Some points that may need some thinking:

• There are progress bars within many functions which is currently implemented by generating and passing an indicatif::ProgressBar struct. I'd think that in order to use in a library we should make this generic over some Progress trait which is trivially implemented by indicatif::ProgressBar and use indicatif::ProgressBar only in the CLI command(s).
• There are few other topics in the commands/helper.rs which need to be tackled.
• Besides progress bars and logs, some commands do produce output, e.g. backup, snapshots, repoinfo, forget, prune, see the println! usage. This should be moved to suitable structs (many are obvious like backup producing a SnapshotFile, snapshots producing a Vec<SnapshotFile>)

Thanks again for tackling this issue and don't hesitate to call out for help or directions!

[simonsan]

• It is right that rustic currently uses the same structs for defining config file sections and CLI options and therefore derives clap::Parser as well as Deserialize. I think we should keep this in the first run.

Because I'm right now working on this one:
What I thought was that to have clap in a library is not really nice, not only for build times, but also because it's the CLI basically. So what I did was to copy and rename the structs keeping all annotations working and having the same struct in the library without annotations/macros. The library part should take precedence, so what we can do is to either do it the trait way and pass the *cli-Structs as is. Or we convert them on the CLI side to our library struct.

Any ideas what's better, because I think it's really important to decouple there. Keeping the clap part in a library does doesn't make my brain super happy gives me a bit of headache, tbf :D

EDIT: But also in general I wonder how the approach is for (De)-Serialization? Especially the serde_as seems important?

[simonsan]

@aawsome Are you reachable via a platform that allows faster communication than this one, e.g. Discord, Matrix, IRC, Telegram?

[aawsome]

Any ideas what's better, because I think it's really important to decouple there. Keeping the clap part in a library does doesn't make my brain super happy gives me a bit of headache, tbf :D

My suggestion:
The Parser/Args impl should be hidden behind a clap feature flag and Deserialize impl should be hidden behind a serde feature flag (and maybe the same for Merge/merge) for the config option structs which we expose in a library crate. The CLI crate could then use the library with this features and doesn't have to redefine them.
Another crate just using the the library could manually set the option structs. I would also make all fields pub and impl Default for these structs, so it can be easily used. Another possibility, of course, would be to implement the builder pattern.

EDIT: But also in general I wonder how the approach is for (De)-Serialization? Especially the serde_as seems important?

serde_as is currently mainly used when struct don't implement Deserialize - mostly when FromStr is already implemented - or for binary data that is in fact serialized as BASE64.

Are you reachable via a platform that allows faster communication than this one, e.g. Discord, Matrix, IRC, Telegram?

I'll look for a way to communicate. However, I may be reachable only at some awkward times as I have some other responsibilities like job and family/children... ;-)

[simonsan]

The Parser/Args impl should be hidden behind a clap feature flag and Deserialize impl should be hidden behind a serde feature flag (and maybe the same for Merge/merge) for the config option structs which we expose in a library crate. The CLI crate could then use the library with this features and doesn't have to redefine them.

That's a good idea, thank you for the hint! we could make a CLI flag as well, that bundles clap, serde and merge into one?

I'll look for a way to communicate. However, I may be reachable only at some awkward times as I have some other responsibilities like job and family/children... ;-)

Same here (: Maybe our awkward times match up 😅

[aawsome]

Ah, just saw one thing: The structs under repofiles should still always implement Serialize and Deserialize as the repository format is JSON and we need to read/write to the repository also when called as library.

[aawsome]

So maybe it doesn't make sense to add the serde feature, as we will anyway depend on serde.

[simonsan]

So maybe it doesn't make sense to add the serde feature, as we will anyway depend on serde.

Yeah, thought the same after seeing how much it is everywhere. will remove the feature again 👍🏽

[simonsan]

Status update: Library builds now for the first time (OS: Win11) with 2 warnings for dead code. 🚀

[simonsan]

@aawsome I added a snapshot of the current public api of the library part (separated by OS) here: https://github.com/simonsan/rustic/tree/refactor/crates/rustic_core/tests/public_api_data

This is the full output though, over 5k lines. Used for CI basically.

Could you please take a look at it and give an opinion what needs to be changed there?

Better for the beginning is probably to run it on your own system with cargo public-api -ss -p rustic_core which utilizes https://crates.io/crates/cargo-public-api

[simonsan]

Status update: Library builds now on openSuse Tumbleweed. 🚀

[aawsome]

Status update: Library builds now on openSuse Tumbleweed. rocket

Wow! Great!

About interface tests:

This is the full output though, over 5k lines. Used for CI basically.
Could you please take a look at it and give an opinion what needs to be changed there?

Uhmm - actually didn't read through the 5k lines :-) I also don't know if we should expose really all this as public interface. Most likely not..

Generally about the interface:
In the commands there is currently loads of logic. I actually would start with only exposing this functionality to a public interface, i.e. something like rustic_core::snapshots(..), rustic_core::backup(..) and so on...
This would then be a very tight library interface which could be extended later step-by-step with more lower-level APIs (like manually opening a repository or later defining a user-defined backend and use this to open a repository, ...)
In the first run a CLI would then only do very simple things like parsing CLI options, setting up the Progressbars and just call the exposed library functionality.
If you like I can sketch this in a separate PR.

Also, I'm wondering that now the error types have been overworked if it would make sense to just use this in a first-start PR which we could merge pretty fast. Thinking about getting to the split library/CLI step-by-step...

The next days I unfortunately have only few time - weekend is family-time and in Germany the 1st of May is a holiday...

[simonsan]

In the commands there is currently loads of logic. I actually would start with only exposing this functionality to a public interface, i.e. something like rustic_core::snapshots(..), rustic_core::backup(..) and so on...

Exactly, same thought here. Will take a look and trying to expose a minimal interface as an API that we can extend, so we can keep the rest stable. There are some things, that I would need to change. Some things are wrapped in a result that clippy says is not needed. Some other things are passed by value but aren't consumed in the body, so we can change that as well to borrow, etc.

If you like I can sketch this in a separate PR.

No, come on, let's do it here together. You can also push to this PR, I did that intentionally, so we can work together on it. 👍🏽

Also, I'm wondering that now the error types have been overworked if it would make sense to just use this in a first-start PR which we could merge pretty fast. Thinking about getting to the split library/CLI step-by-step...

Would prefer it like doing it completely. think it's better to have it running directly and adapt everything so it is working like before, but split to cli/library. Maybe development on the main branch can be paused, so it's not many merge conflicts, and we focus on the split. Will make everything going forward much easier - and probably less time consuming.

The next days I unfortunately have only few time - weekend is family-time and in Germany the 1st of May is a holiday...

German here as well, same stuff. I have my daughter though in the "Wechselmodell", so whole next week, with probably less time to work on it. The week after more.

[simonsan]

I think I will take the following approach:

• make everything (reasonably) private in the library
  • make everything the library uses internally pub(crate)/pub
• implement the commands in the CLI and expose the needed parts from the library as public
• this could be the minimal needed interface
• then we can take a look for other things, that we would like to expose
  • with the cargo public-api command we will have a good overview there

Anything to add/change? Better approach?

[simonsan]

I wonder, does it make sense to have the errors as associated types in the traits?

e.g.?

pub trait ReadBackend: Clone + Send + Sync + 'static {
    type ReadBackendError: Send + Sync + 'static + std::error::Error;

    fn location(&self) -> String;

    fn set_option(&mut self, option: &str, value: &str) -> Result<(), Self::ReadBackendError>;
...
}

or would it be better to go with something like this?

type BackendResult<T> = Result<T, BackendErrorKind>

pub trait ReadBackend: Clone + Send + Sync + 'static {
    fn set_option(&mut self, option: &str, value: &str) -> BackendResult<()>;
}

I'm struggling a bit with the implications here 🤔 not often working with generics, I have to say.

I think I need some guidance here, to implement the error handling in the library part the right way.

EDIT: Variant 1 is probably the way to go, problem is I think we need to remove implemented default methods of this trait and copy and paste them, as they would need their own errors?

[simonsan]

@aawsome Would it be possible for you to have a 20 minutes Discord call (if you have, or another platform) in our mother tongue German to figure out how to go forward on this, also with regards to the trait impl and error handling. Probably much faster, than writing here. So less time invasive overall, I figure. 🤔

[aawsome]

I created a Discord channel: https://discord.gg/zGZsfkZue6
This evening I'll try to be there at around 21:15 MET (depends on when kids are in bed).

I never used Discord before, so hope everything works out ;-)

[aawsome]

Thanks a lot @simonsan for all your work!

I checked the changes today and didn't find problems - however as this is a big redesign, there might be still some bugs lurking around. We'll keep this beta for a while to find those bugs - if anyone finds something suspicious, please don't hesitate to report an issue!

LGTM!