[PATCH iproute2-next 0/2] configure: add support for color

[PATCH iproute2-next 0/2] configure: add support for color

[Andrea Claudi]

This series add support for the color parameter in iproute2 configure
script. The idea is to make it possible for iproute2 users and packagers
to set a default value for the color option different from the current
one, COLOR_OPT_NEVER, while maintaining the current default behaviour.

Patch 1 add the color option to the configure script. Users can set
three different values, never, auto and always, with the same meanings
they have for the -c / -color ip option. Default value is 'never', which
results in ip, tc and bridge to maintain their current output behaviour
(i.e. colorless output).

Patch 2 makes it possible for ip, tc and bridge to use the configured
value for color as their default color output.

Andrea Claudi (2):
  configure: add the --color option
  treewide: use configured value as the default color output

 Makefile        |  3 ++-
 bridge/bridge.c |  3 ++-
 configure       | 37 +++++++++++++++++++++++++++++++++++++
 ip/ip.c         |  2 +-
 tc/tc.c         |  2 +-
 5 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.41.0

[PATCH iproute2-next 1/2] configure: add the --color option

[Andrea Claudi]

This commit allows users/packagers to choose a default for the color
output feature provided by some iproute2 tools.

The configure script option is documented in the script itself and it is
pretty much self-explanatory. The default value is set to "never" to
avoid changes to the current ip, tc, and bridge behaviour.

Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 Makefile  |  3 ++-
 configure | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7d1819ce..a24844cf 100644
--- a/Makefile
+++ b/Makefile
@@ -41,7 +41,8 @@ endif
 DEFINES+=-DCONF_USR_DIR=\"$(CONF_USR_DIR)\" \
          -DCONF_ETC_DIR=\"$(CONF_ETC_DIR)\" \
          -DNETNS_RUN_DIR=\"$(NETNS_RUN_DIR)\" \
-         -DNETNS_ETC_DIR=\"$(NETNS_ETC_DIR)\"
+         -DNETNS_ETC_DIR=\"$(NETNS_ETC_DIR)\" \
+         -DCONF_COLOR=$(CONF_COLOR)
 
 #options for AX.25
 ADDLIB+=ax25_ntop.o
diff --git a/configure b/configure
index 18be5a03..eb689341 100755
--- a/configure
+++ b/configure
@@ -5,6 +5,7 @@
 INCLUDE="$PWD/include"
 PREFIX="/usr"
 LIBDIR="\${prefix}/lib"
+COLOR="never"
 
 # Output file which is input to Makefile
 CONFIG=config.mk
@@ -479,6 +480,24 @@ check_cap()
 	fi
 }
 
+check_color()
+{
+	case "$COLOR" in
+		never)
+			echo 'CONF_COLOR:=COLOR_OPT_NEVER' >> $CONFIG
+			echo 'never'
+			;;
+		auto)
+			echo 'CONF_COLOR:=COLOR_OPT_AUTO' >> $CONFIG
+			echo 'auto'
+			;;
+		always)
+			echo 'CONF_COLOR:=COLOR_OPT_ALWAYS' >> $CONFIG
+			echo 'always'
+			;;
+	esac
+}
+
 quiet_config()
 {
 	cat <<EOF
@@ -509,6 +528,10 @@ usage()
 {
 	cat <<EOF
 Usage: $0 [OPTIONS]
+	--color <never|auto|always>	Default color output configuration. Available options:
+					  never: color output is disabled (default)
+					  auto: color output is enabled if stdout is a terminal
+					  always: color output is enabled regardless of stdout state
 	--include_dir <dir>		Path to iproute2 include dir
 	--libdir <dir>			Path to iproute2 lib dir
 	--libbpf_dir <dir>		Path to libbpf DESTDIR
@@ -527,6 +550,11 @@ if [ $# -eq 1 ] && [ "$(echo $1 | cut -c 1)" != '-' ]; then
 else
 	while [ "$#" -gt 0 ]; do
 		case "$1" in
+			--color)
+				shift
+				COLOR="$1" ;;
+			--color=*)
+				COLOR="${1#*=}" ;;
 			--include_dir)
 				shift
 				INCLUDE="$1" ;;
@@ -563,6 +591,12 @@ else
 	done
 fi
 
+case "$COLOR" in
+	never) ;;
+	auto) ;;
+	always) ;;
+	*) usage 1 ;;
+esac
 [ -d "$INCLUDE" ] || usage 1
 if [ "${LIBBPF_DIR-unused}" != "unused" ]; then
 	[ -d "$LIBBPF_DIR" ] || usage 1
@@ -634,6 +668,9 @@ check_strlcpy
 echo -n "libcap support: "
 check_cap
 
+echo -n "color output: "
+check_color
+
 echo >> $CONFIG
 echo "%.o: %.c" >> $CONFIG
 echo '	$(QUIET_CC)$(CC) $(CFLAGS) $(EXTRA_CFLAGS) $(CPPFLAGS) -c -o $@ $<' >> $CONFIG
-- 
2.41.0

Re: [PATCH iproute2-next 1/2] configure: add the --color option

[Stephen Hemminger]

On Wed, 13 Sep 2023 19:58:25 +0200
Andrea Claudi <aclaudi@redhat.com> wrote:

> This commit allows users/packagers to choose a default for the color
> output feature provided by some iproute2 tools.
> 
> The configure script option is documented in the script itself and it is
> pretty much self-explanatory. The default value is set to "never" to
> avoid changes to the current ip, tc, and bridge behaviour.
> 
> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> ---

More build time config is not the answer either.
Don't want complaints from distribution users about the change.
Needs to be an environment variable or config file.

Re: [PATCH iproute2-next 1/2] configure: add the --color option

[David Ahern]

On 9/15/23 9:59 AM, Stephen Hemminger wrote:
> On Wed, 13 Sep 2023 19:58:25 +0200
> Andrea Claudi <aclaudi@redhat.com> wrote:
> 
>> This commit allows users/packagers to choose a default for the color
>> output feature provided by some iproute2 tools.
>>
>> The configure script option is documented in the script itself and it is
>> pretty much self-explanatory. The default value is set to "never" to
>> avoid changes to the current ip, tc, and bridge behaviour.
>>
>> Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
>> ---
> 
> More build time config is not the answer either.
> Don't want complaints from distribution users about the change.
> Needs to be an environment variable or config file.

I liked the intent, and it defaults to off. Allowing an on by default
brings awareness of the usefulness of the color option.

I have applied the patches, so we need either a revert or followup based
on Stephen's objections.

Re: [PATCH iproute2-next 1/2] configure: add the --color option

[Andrea Claudi]

On Fri, Sep 15, 2023 at 08:59:12AM -0700, Stephen Hemminger wrote:
> On Wed, 13 Sep 2023 19:58:25 +0200
> Andrea Claudi <aclaudi@redhat.com> wrote:
> 
> > This commit allows users/packagers to choose a default for the color
> > output feature provided by some iproute2 tools.
> > 
> > The configure script option is documented in the script itself and it is
> > pretty much self-explanatory. The default value is set to "never" to
> > avoid changes to the current ip, tc, and bridge behaviour.
> > 
> > Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
> > ---
> 
> More build time config is not the answer either.
> Don't want complaints from distribution users about the change.
> Needs to be an environment variable or config file.
>

Hi Stephen,
This is not modifying the default behaviour; as David noted color output
will be off as it is right now. If packagers want to make use of this,
it's up to them to choose a sane default for their environment. After
all, we are providing options such as '--prefix' and '--libdir', and
there are endless possibilities to choose obviously wrong values for
these vars. Packagers are gonna deal with their own choices.

I think I can improve this in two ways:

1. Exclude 'always' from the allowed color choices
This is the setting with the highest chance to produce complaints, since
it is enabling color output regardless of stdout state. 'auto' instead
produces color output only on stdout that are terminals. Of course
'always' will remain as a param choice for the command line.

2. Add packaging guidelines to README (or README.packaging)
iproute packaging is a bit tricky, since some packaging systems simply
assume that configure comes from autotools. We even leverage this to our
advantage, providing configure options that packaging systems use
flawlessly as the autotools ones. I can provide some info about this,
and add some recommendations about sane configure defaults, especially
about the --color option.

What do you think? Is this approach fine for you?

[PATCH iproute2-next 2/2] treewide: use configured value as the default color output

[Andrea Claudi]

With Makefile providing -DCONF_COLOR, we can use its value as the
default color output.

This effectively allow users and packagers to define a default for the
color output feature without using shell aliases, and with minimum code
impact.

Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
 bridge/bridge.c | 3 ++-
 ip/ip.c         | 2 +-
 tc/tc.c         | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/bridge/bridge.c b/bridge/bridge.c
index 704be50c..339101a8 100644
--- a/bridge/bridge.c
+++ b/bridge/bridge.c
@@ -23,7 +23,6 @@ int preferred_family = AF_UNSPEC;
 int oneline;
 int show_stats;
 int show_details;
-static int color;
 int compress_vlans;
 int json;
 int timestamp;
@@ -103,6 +102,8 @@ static int batch(const char *name)
 int
 main(int argc, char **argv)
 {
+	int color = CONF_COLOR;
+
 	while (argc > 1) {
 		const char *opt = argv[1];
 
diff --git a/ip/ip.c b/ip/ip.c
index 8c046ef1..860ff957 100644
--- a/ip/ip.c
+++ b/ip/ip.c
@@ -168,7 +168,7 @@ int main(int argc, char **argv)
 	const char *libbpf_version;
 	char *batch_file = NULL;
 	char *basename;
-	int color = 0;
+	int color = CONF_COLOR;
 
 	/* to run vrf exec without root, capabilities might be set, drop them
 	 * if not needed as the first thing.
diff --git a/tc/tc.c b/tc/tc.c
index 25820500..082c6677 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -35,7 +35,6 @@ int use_iec;
 int force;
 bool use_names;
 int json;
-int color;
 int oneline;
 int brief;
 
@@ -254,6 +253,7 @@ int main(int argc, char **argv)
 {
 	const char *libbpf_version;
 	char *batch_file = NULL;
+	int color = CONF_COLOR;
 	int ret;
 
 	while (argc > 1) {
-- 
2.41.0

Re: [PATCH iproute2-next 0/2] configure: add support for color

[patchwork-bot+netdevbpf]

Hello:

This series was applied to iproute2/iproute2-next.git (main)
by David Ahern <dsahern@kernel.org>:

On Wed, 13 Sep 2023 19:58:24 +0200 you wrote:
> This series add support for the color parameter in iproute2 configure
> script. The idea is to make it possible for iproute2 users and packagers
> to set a default value for the color option different from the current
> one, COLOR_OPT_NEVER, while maintaining the current default behaviour.
> 
> Patch 1 add the color option to the configure script. Users can set
> three different values, never, auto and always, with the same meanings
> they have for the -c / -color ip option. Default value is 'never', which
> results in ip, tc and bridge to maintain their current output behaviour
> (i.e. colorless output).
> 
> [...]

Here is the summary with links:
  - [iproute2-next,1/2] configure: add the --color option
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=5e704f4b5ba2
  - [iproute2-next,2/2] treewide: use configured value as the default color output
    https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=b5d0273fdbab

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html