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

utils_net:remove -c=never for ip cmdline #4019

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxujun
Copy link
Contributor

@maxujun maxujun commented Oct 29, 2024

ID:2979
Low version ip command has no this parameter "-c=never"
Remove -c=never,because default output without -c has no color code.

@maxujun
Copy link
Contributor Author

maxujun commented Oct 29, 2024

(1/2) Host_RHEL.m8.u10.product_rhel.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.9.ppc64le.io-github-autotest-qemu.bigfile_transfer_over_ipv6.remote_addr: STARTED
(1/2) Host_RHEL.m8.u10.product_rhel.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.9.ppc64le.io-github-autotest-qemu.bigfile_transfer_over_ipv6.remote_addr: PASS (346.58 s)
(2/2) Host_RHEL.m8.u10.product_rhel.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.9.ppc64le.io-github-autotest-qemu.bigfile_transfer_over_ipv6.link_local_addr: STARTED
(2/2) Host_RHEL.m8.u10.product_rhel.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.9.ppc64le.io-github-autotest-qemu.bigfile_transfer_over_ipv6.link_local_addr: PASS (346.30 s)

@maxujun
Copy link
Contributor Author

maxujun commented Nov 4, 2024

(1/1) Host_RHEL.m8.u10.product_rhel.nographic.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.9.s390x.io-github-autotest-qemu.qemu_guest_agent.virtio_serial.check_get_interfaces.s390-virtio: STARTED
(1/1) Host_RHEL.m8.u10.product_rhel.nographic.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.7.9.s390x.io-github-autotest-qemu.qemu_guest_agent.virtio_serial.check_get_interfaces.s390-virtio: PASS (50.32 s)

@maxujun
Copy link
Contributor Author

maxujun commented Nov 4, 2024

#ip -c=never link > a
#ip link > b
#md5sum a b
f35a2eab0f42ce2114a808e4ec526f38 a
f35a2eab0f42ce2114a808e4ec526f38 b
Have the same output without color code. So I don't think need parameter "-c=never" here.

@PaulYuuu @fbq815 Could you help review?

@PaulYuuu
Copy link
Contributor

This is a regression from #3931, IMO, not all distros need to set this, the color option is not always set during compiling the iproute.

%build
%configure --color auto
echo -e "\nSBINDIR=%{_sbindir}" >> config.mk
%make_build

We cannot make sure all distributions built the ip command with --color support, so I prefer to revert that commit and the easy way is to set an alias in the distro ks file like alias ip="ip --color=never"

@luckyh @chloerh, please leave your comment if you have.

@fbq815
Copy link
Contributor

fbq815 commented Nov 12, 2024

I've added the comment in #3931 maybe @maxujun could help contact with the developer for revert or distribute the differences between the original requirement and our needs

@chloerh
Copy link
Contributor

chloerh commented Nov 12, 2024

We cannot make sure all distributions built the ip command with --color support, so I prefer to revert that commit and the easy way is to set an alias in the distro ks file like alias ip="ip --color=never"

I think this could work on our side. I'll ask teammates to make sure. Please hold.
#3931 was aimed to fix errors that ip command on rhel10 returns with color characters messing with the output .

@chloerh
Copy link
Contributor

chloerh commented Nov 12, 2024

@PaulYuuu my teammates rejected the alias plan. I'm not sure whether this pr will affect our test or not, you could merge it and we'll see what happens.

@brucepatricklee
Copy link
Contributor

brucepatricklee commented Nov 12, 2024

instead of using the alias command here, a better way is to execute a non-color command such as "ip addr | xargs". Otherwise, you will have to add more and more alias for each colourful command :)

@PaulYuuu
Copy link
Contributor

instead of using the alias command here, a better way is to execute a non-color command such as "ip addr | xargs". Otherwise, you will have to add more and more alias for each colourful command :)

They are not better than each other. Redirecting output to PIPE requires modifying everywhere, and newly added functions must ensure that they respect this agreement(including avocado if it has). when ip command default to --color=always, then redirect may not work. Each distro has its style, color is human readable but useless for automation, we aim to fix this, and both solutions are okay for me.

@PaulYuuu
Copy link
Contributor

@PaulYuuu my teammates rejected the alias plan. I'm not sure whether this pr will affect our test or not, you could merge it and we'll see what happens.

Could you request them to provide a better solution if they have? We need to consider compatibility but this PR is not enough. Upstream first and then internal, #3931 is to adapt the RHEL10 change, but it introduced regressions for RHEL7 even though RHEL7 is EOL, maybe other distros don't need this but we haven't seen it yet.

@chloerh
Copy link
Contributor

chloerh commented Nov 13, 2024

@PaulYuuu I've discussed with teammates, we agreed that you could delete all --color=never , we'll fix the errors in tp-libvirt.

@maxujun
Copy link
Contributor Author

maxujun commented Nov 19, 2024

Have removed all -c=never.

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.

5 participants