feat(rustup): Add rustup as a backend

InnocentZero commented 7 months ago

Adds rustup as a backend for pacdef. Only works on components listed for now. install_packages needs a rewrite to accomodate the new syntax. Fixes #54 .

steven-omaha commented 7 months ago

Let me know when you'd like a review.

InnocentZero commented 7 months ago

@steven-omaha I'd like for you to check commit a3b6319 before anything else. I've slightly modified the logic to split repo and name in Package and make repo field public. This doesn't break any compatibility though but makes it way easier to have rustup deal with toolchains and components at once.

steven-omaha commented 7 months ago

@InnocentZero Thank you, I'll get back to you latest in a week.

steven-omaha commented 7 months ago

@InnocentZero FYI, I haven't forgotten about this, but my time is a bit scarce right now. Will get to it when I can.

InnocentZero commented 7 months ago

Of course, by all means take your time.

InnocentZero commented 6 months ago

@steven-omaha Hi, any updates? Hope you haven't forgotten about this :sweat_smile:

steven-omaha commented 6 months ago

Hey, I definitely haven't! It's on my agenda for this week. RL is just a bit unpredictable atm.

Looking forward to merging this!

InnocentZero commented 6 months ago

This is good work so far, but requires a couple of bigger changes. I probably didn't catch all things that could be improved. Please try and include these changes. Let me know if you need any help or guidance.

Hi. Really sorry for the sub optimal code this time. Turned out I hadn't configured helix with clippy (implicitly assumed it did). I also usually rename variables and refactor before pushing, but this time I missed for some reason :sweat:

I have an exam tomorrow, but the changes are ready and committed on my local machine. I'll go through all of them once more and push them by tomorrow afternoon at best. Thanks for the reviews.

EDIT: This comment was supposed to be posted yesterday. Quite a few things I'm forgetting wrt this PR :sweat_smile:

steven-omaha commented 6 months ago

FYI: anyhow::bail!() is so common in Rust that it deserves its own import.

steven-omaha commented 6 months ago

I'm on my phone right now so can't really review, but lines 127 to 131 is a match statement in disguise, which is what you would want to use when dealing with Options.

Edit: that pattern appears multiple times in your code

steven-omaha commented 6 months ago

The code is ready to merge! That's excellent news! I'll probably clean it up a bit more later, but that's fine.

I wanted to test the code. Using pacdef package review I generated the following section.


[rustup]
toolchain/nightly
component/nightly/cargo
component/nightly/clippy
component/nightly/rust-docs
component/nightly/rust-src
component/nightly/rust-std
component/nightly/rustc
component/nightly/rustfmt
toolchain/stable
component/stable/cargo
component/stable/clippy
component/stable/rls  #  removing this here
component/stable/rust-analysis  # and this here
component/stable/rust-docs
component/stable/rust-src
component/stable/rust-std
component/stable/rustc
component/stable/rustfmt

I had rls and rust-analysis installed which I didn't know. I removed the two lines and ran the following

$ cargo run --release -- package clean
Would remove the following packages:

[rustup]
component/stable/rls
component/stable/rust-analysis

Continue? [Y/n]
error: toolchain 'stable-x86_64-unknown-linux-gnu' does not contain component 'stable' for target 'x86_64-unknown-linux-gnu'
Error: running action

Caused by:
    0: removing packages
    1: command returned with exit code 1

What's going on here?

InnocentZero commented 6 months ago

What's going on here?

My bad. While cleaning up I forgot to actually increment the iterator while checking if that toolchain is still lying around or not. I believe it should be fixed now.

steven-omaha commented 6 months ago

That fixed it. Merging!

steven-omaha commented 6 months ago

I'm gonna test this for a week or so until I release it for everyone.

InnocentZero commented 6 months ago

@steven-omaha I tested the following scenarios for installation

toolchain installed and components installed for that toolchain: Works perfectly
toolchain installed and some components installed for that toolchain/some removed: Works but package sync takes more time to remove every component in a separate command. I did not time it, just observed.
Component installed without the toolchain being installed: Doesn't work

Out of all the scenarios point 3 is the only one that ends with an error.

In ideal circumstances point 3 makes no sense. But I would have preferred to handle it more gracefully than it ending with an error but I do not want to complicate the logic a lot.

An extreme case of point 2, only toolchain and no components will take too long for package clean because every component will have a separate command called for it but tbf it makes sense considering no one will ideally want a toolchain with nothing installed for that toolchain.

For package clean:

Removing a toolchain with installed components removes the components, which is expected.
Removing a component with the toolchain installed lets the rest of the toolchain/components sit in peace.
Removing a toolchain with no installed components also works without any warnings/errors.
Removing a component with no toolchain is not possible because you can't have this configuration to begin with.

steven-omaha commented 6 months ago

Okay, that makes sense. I would say we leave this as it is for now. We can consider this if bug reports start coming in.

Thanks again for your great work!

steven-omaha / pacdef

feat(rustup): Add rustup as a backend #55