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

feat: add XBPS support (for Void Linux) #330

Merged
merged 9 commits into from
Mar 10, 2022
Merged

Conversation

tranzystorekk
Copy link
Contributor

Adds support for the XBPS package manager from Void Linux.

The implementation is complete, but I'm not sure how to add tests, I'd appreciate some hints 😄

@rami3l
Copy link
Owner

rami3l commented Feb 22, 2022

@tranzystorek-io
Thanks a lot for your contribution!

Sorry, I might be slow to respond now, so the code review might have to take place a bit later.

As for the tests, they are mostly smoke tests defined in ./tests.
For those, we use (almost) the same test DSL syntax as the original pacapt, so icy/pacapt@1bbf2a3 should be a good starting point!
If you are curious, the Rust version of the DSL is defined here.

To run these tests, you can see the definition of test environments here. In short, you have to set up a Void Linux environment in Docker.

Finally, thanks for your feedback and I'll make contributing instructions more visible!
If you have any questions, please feel free to contact me :)

@rami3l rami3l changed the title WIP: Add XBPS support WIP: Add XBPS support | Read CONTRIB.md indicating where to look at Feb 22, 2022
@rami3l rami3l changed the title WIP: Add XBPS support | Read CONTRIB.md indicating where to look at WIP: Add XBPS support | Re-add CONTRIBUTING.md indicating where to look at when add pm support Feb 22, 2022
@rami3l rami3l changed the title WIP: Add XBPS support | Re-add CONTRIBUTING.md indicating where to look at when add pm support Re-add CONTRIBUTING.md on how to add PM support Feb 22, 2022
@rami3l rami3l self-assigned this Feb 22, 2022
@rami3l rami3l added enhancement New feature or request good first issue Good for newcomers labels Feb 22, 2022
@rami3l rami3l changed the title Re-add CONTRIBUTING.md on how to add PM support WIP: Add XBPS support Feb 22, 2022
@rami3l rami3l changed the title WIP: Add XBPS support feat: add XBPS support (for Void Linux) Feb 22, 2022
@rami3l
Copy link
Owner

rami3l commented Feb 22, 2022

@tranzystorek-io
Sorry, I was trying out GitHub Projects Beta and I accidentally changed the title of this PR 😓
It's normal right now anyway.

@tranzystorekk
Copy link
Contributor Author

Thanks, it's all clear now, I should have a test suite ready in no time! 😄

@tranzystorekk
Copy link
Contributor Author

I noticed that xbps-query doesn't support listing selected packages like pacman -Qe package_a package_b does, do you think there should be some warning?

@rami3l
Copy link
Owner

rami3l commented Feb 23, 2022

@tranzystorek-io
Good question! In fact, you can implement this command by:

  • Either sending a warning saying that it's not supported;
  • Or, better yet, implement the correct behavior if it's trivial to do so.

Actually, I'm no expert in XBPS, but the question is rather "how you want it to behave", so the decision is still yours, while in general I'll personally favor the second approach. This is why I'm doing this project in Rust rather than sticking with the original sh implementation: with PmMode and Strategy you can make subcommands launch in the way you like (with or without output, prompts, etc.).

For example, if it's just a for loop over the keywords launching a subcommand for each, please do us all a favor by implementing it for the maximal convenience :)

@tranzystorekk
Copy link
Contributor Author

For example, if it's just a for loop over the keywords launching a subcommand for each, please do us all a favor by implementing it for the maximal convenience :)

Tried to implement that, but there doesn't seem to be a PmMode that would allow me to collect the outputs of the command launches separately, and otherwise I get a version where each run is announced:

obraz

@rami3l
Copy link
Owner

rami3l commented Feb 26, 2022

@tranzystorek-io

there doesn't seem to be a PmMode that would allow me to collect the outputs of the command launches separately

I see what you mean. It shouldn't harm though if you first suppress all stdout, collect output in a Rust Vec for example, and then print them all out, similar to what I did here: I picked a part of the original stdout to be printed.

pacaptr/src/pm/brew.rs

Lines 47 to 50 in 10689a4

let out_bytes = self
.check_output(cmd, PmMode::Mute, &Strategy::default())
.await?;
exec::grep_print(&String::from_utf8(out_bytes)?, kws)


I admit though the current API might not be the best in terms of UX improvements: for example, it cannot easily show the progress in a series of subcommand launches, and controlling output is also a bit awkward.

Thanks again for your advice! In the long run, I'll indeed consider adding some API for this pattern especially if it's general enough in package managers.

src/pm/xbps.rs Outdated Show resolved Hide resolved
src/pm/xbps.rs Outdated Show resolved Hide resolved
@rami3l
Copy link
Owner

rami3l commented Feb 26, 2022

@tranzystorek-io
Thanks a lot! 🎉

Now that you're definitely more comfortable with xbps than I am, I think you can consider to add some smoke tests now in tests/xbps.rs.

PS: Don't mind that dependabot action, when you forked it wasn't fixed yet :/

@tranzystorekk
Copy link
Contributor Author

@rami3l whew, took me a bit but the code and an initial test suite is ready!

src/pm/xbps.rs Outdated Show resolved Hide resolved
src/pm/xbps.rs Outdated Show resolved Hide resolved
src/pm/xbps.rs Outdated Show resolved Hide resolved
src/pm/xbps.rs Outdated Show resolved Hide resolved
}

#[test]
fn xbps_q() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you feel like doing it, you can also add tests on the special error handling of -Q and -Qe here :)

src/pm/xbps.rs Show resolved Hide resolved
@tranzystorekk
Copy link
Contributor Author

tranzystorekk commented Mar 9, 2022

Hmm, seems like mixing print_err and tokio stdout causes races when writing:

image

Seems like we would need an async variant of print_err

@rami3l
Copy link
Owner

rami3l commented Mar 9, 2022

@tranzystorek-io
Oops... Don't worry, I will definitely investigate this issue once I have time to do so.

src/pm/xbps.rs Outdated Show resolved Hide resolved
@rami3l
Copy link
Owner

rami3l commented Mar 10, 2022

@tranzystorek-io
Again, many thanks! Do you think now it's the right time to merge this PR, or you still have things to add? :)

@tranzystorekk tranzystorekk marked this pull request as ready for review March 10, 2022 15:32
@tranzystorekk
Copy link
Contributor Author

@rami3l I think it's good to go 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants