Powerlevel9k / powerlevel9k

Powerlevel9k was a tool for building a beautiful and highly functional CLI, customized for you. P9k had a substantial impact on CLI UX, and its legacy is now continued by P10k.
https://github.com/romkatv/powerlevel10k
MIT License
13.46k stars 949 forks source link

[Bugfix] More consistent results from 'ip' on Non-OSX systems #1356

Open Jekotia opened 4 years ago

Jekotia commented 4 years ago

Description

Due to issues matching with some versions of iproute's 'ip' command, I've rewritten and thoroughly commented the non-OSX portion of p9k::parseIp. See https://github.com/Powerlevel9k/powerlevel9k/issues/823

If desired, a lot of this can be rewritten to be shorter (and theoretically a smidge faster to execute); I chose to write the initial code in this manner to make it easier to read what I've done when reviewing the pull request.

Questions

None.

Additional notes against the bulletpoints from your PR guidelines:

romkatv commented 4 years ago

p9k::parseIp used to skip interfaces that aren't UP. Is this still the case with this PR?

Jekotia commented 4 years ago

Yes it is. Two checks are preformed; the first is dependent on POWERLEVEL9K_IP_INTERFACE being unset. If the value is unset, the check is made and it gets the name for the first interface which is UP.

The second check is preformed always; using either the interface name acquired in the first check or the interface name provided by the user configuration (POWERLEVEL9K_IP_INTERFACE), it acquires the IP address for that interface.

romkatv commented 4 years ago

I'll rephrase my question.

With the current implementation, if I set POWERLEVEL9K_IP_INTERFACE=foo, ip prompt segment won't be displayed unless network interface foo is UP (as can be seen in the output of ip link show foo).

Is this behavior preserved by the PR?

Jekotia commented 4 years ago

Yes it is.

romkatv commented 4 years ago

Yes it is.

Have you tried it? Once you get ip prompt segment working with POWERLEVEL9K_IP_INTERFACE=foo, type sudo ip link set foo down. This should hide ip segment.

Jekotia commented 4 years ago

Unfortunately, I do not have any systems to test this on (I use WSL on my computer, which it seems cannot toggle interfaces on the Windows host; the other *nix systems are servers and turning off an interface on any one of them would be followed by much yelling), but I see the problem you seem to be hinting at.

The way I've written this, if you manually specify an interface the IP address, if found, will ALWAYS be returned for that interface. The only instances where it will return nothing is if a) no interface is specified and no interface is up with an address b) a non-existent interface is specified.

I'll update this when I can.

romkatv commented 4 years ago

The way I've written this, if you manually specify an interface the IP address, if found, will ALWAYS be returned for that interface. The only instances where it will return nothing is if a) no interface is specified and no interface is up with an address b) a non-existent interface is specified.

That's right. The output of ip -o -f inet addr show foo is the same whether foo is UP or DOWN. Since it's the only command that your code calls when POWERLEVEL9K_IP_INTERFACE=foo, it's not possible to differentiate between UP and DOWN links.

Also note that the detection of link state is broken in the current code but in a different direction from your PR. While your PR will sometimes display an IP from an interface that is down, the current code will sometimes fail to display an IP from an interface that is up.

P.S.

It's a good idea to test your code on Linux and FreeBSD in addition to WSL. These systems have different network stacks and tools.

Jekotia commented 4 years ago

Regarding the issue in the current code, that's exactly what I'm trying (and less successfully than I had hoped) to address. It's failing to show the IP for an interface which is up, as explained in the referenced issue.

I've tested it on several CentOS and Debian systems, I just don't have the ability to flip their interfaces on and off since they're production systems at work.

romkatv commented 4 years ago

Regarding the issue in the current code, that's exactly what I'm trying (and less successfully than I had hoped) to address.

Yes, I've read your description of the issue affecting WSL. I'm referring to a different issue that affects Linux. I don't remember the details but the essential part is that the current code is mistakenly looking at the operational link state instead of its administrative state. Operational state is unreliable with many drivers and is best ignored.

I've tested it on several CentOS and Debian systems, I just don't have the ability to flip their interfaces on and off since they're production systems at work.

If you cannot get access to a physical Linux or FreeBSD box, use a VM. WSL is a poor substitute for Linux when it comes to testing your code because it has no Linux kernel. Linux in a VM on Windows will work just like a real Linux in all but the most obscure of corner cases. You could also try WSL2 or Docker, both of which run a real Linux kernel in a VM, but you'll still need access to FreeBSD or some other non-Apple BSD. Either way, it's a good investment of your time to learn how to run an OS in a VM.

P.S.

Please note that I'm neither a developer nor a user of Powerlevel9k. My opinion on what you should do comes with no authority whatsoever. Should you decide to heed or ignore my comments will have no bearing on your PR's acceptance chances.