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

ext: Replace terminal_size with comfy-table #940

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

cgwalters
Copy link
Collaborator

I was looking at our vendoring set and while it's not actually relevant I found myself wondering why we had three versions of windows-sys. Having that many crate versions is often a signal that there's an unmaintained dependency.

And indeed, terminal_size is no longer cool. The "in" crowd has moved on to newer, hipper things. Life moves fast, we need to keep up.

(OK but yes also this drops some manual column printing code
we had which is also a win)

@cgwalters cgwalters requested a review from omertuc December 6, 2024 14:34
@omertuc
Copy link
Contributor

omertuc commented Dec 6, 2024

Trying to test this PR, I built it into a bootc image, bootc switch'd into it, then I was wondering if I was getting the ostree-rs-ext version from this repo or some other one:

[omer@hal9000 ~]$ ls -lah /usr/libexec/libostree/ext/ostree-container
lrwxrwxrwx. 1 root root 23 Jan  1  1970 /usr/libexec/libostree/ext/ostree-container -> ../../../bin/rpm-ostree

Why is this pointing at /usr/bin/rpm-ostree? I would've expected it to point at a binary related to ostree-ext?

@cgwalters
Copy link
Collaborator Author

Why is this pointing at /usr/bin/rpm-ostree? I would've expected it to point at a binary related to ostree-ext?

That's exactly it - right now rpm-ostree is vendoring the ostree-ext code in Fedora. The recent merge here will let us switch that over to bootc (and we'll make rpm-ostree hard Requires: bootc).

If you want to test this you'll need to explicitly take over the symlink, via make install-ostree-hooks at build time or just on a live system bootc usroverlay && ln -sfr /usr/bin/bootc /usr/libexec/libostree/ext/ostree-container

@cgwalters
Copy link
Collaborator Author

Hmm though actually in our own CI jobs we should start taking over the hooks now. I'll do a PR for that

@omertuc
Copy link
Contributor

omertuc commented Dec 6, 2024

image

Not sure which is worse 😆

But seriously, without | head the older version doesn't have this weird newline issue

@cgwalters
Copy link
Collaborator Author

The | head thing is an old, old problem... rust-lang/rust#46016

@cgwalters
Copy link
Collaborator Author

I'm not getting the extra newlines here...I did sanity check this too before pushing

@omertuc
Copy link
Contributor

omertuc commented Dec 6, 2024

Seems like it only happens if the terminal is smaller than the largest line

size.mp4

EDIT: jinx

@cgwalters
Copy link
Collaborator Author

Ahh wait I do get the newlines if one of the output lines is long enough to wrap past the terminal size. I think this may be fixable with https://docs.rs/comfy-table/latest/comfy_table/enum.ContentArrangement.html ?

@omertuc
Copy link
Contributor

omertuc commented Dec 6, 2024

Ahh wait I do get the newlines if one of the output lines is long enough to wrap past the terminal size. I think this may be fixable with docs.rs/comfy-table/latest/comfy_table/enum.ContentArrangement.html ?

Yeah

use comfy_table::{presets::NOTHING, Table};

fn main() {
    let mut table = Table::new();

    table
        .load_preset(NOTHING)
        .set_header(vec!["REPOSITORY", "TYPE"])
        .set_content_arrangement(comfy_table::ContentArrangement::Dynamic);

    table.add_row(vec!["foo", "bar"]);
    table.add_row(vec![
        "fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo",
        "baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar",
    ]);

    println!("{table}");
}

Seems well behaved

@omertuc
Copy link
Contributor

omertuc commented Dec 6, 2024

use std::env::args;

use comfy_table::{presets::NOTHING, Table};

fn main() {
    let mut table = Table::new();

    table
        .load_preset(NOTHING)
        .set_header(vec!["REPOSITORY", "TYPE"])
        .set_content_arrangement(match args().nth(1).as_deref() {
            Some("d") => comfy_table::ContentArrangement::Disabled,
            Some("dyn") => comfy_table::ContentArrangement::Dynamic,
            _ => comfy_table::ContentArrangement::DynamicFullWidth,
        });

    table.add_row(vec!["foo", "bar"]);
    table.add_row(vec![
        "fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo",
        "baaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaar",
    ]);

    println!("{table}");
}

Not sure I understand the difference between Dynamic and DynamicFull, didn't make a diff in my toy demo

image

I was looking at our vendoring set and while it's not actually
relevant I found myself wondering why we had *three* versions
of `windows-sys`. Having that many crate versions is often a signal
that there's an unmaintained dependency.

And indeed, `terminal_size` is no longer cool. The "in" crowd
has moved on to newer, hipper things. Life moves fast, we need
to keep up.

(OK but yes also this drops some manual column printing code
 we had which is also a win)

Signed-off-by: Colin Walters <walters@verbum.org>
@cgwalters cgwalters merged commit 3ead145 into containers:main Dec 6, 2024
17 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants