Re: [Bridge] [PATCH iproute2-next 1/2] configure: add the --color option
On Fri, Sep 15, 2023 at 08:59:12AM -0700, Stephen Hemminger wrote: > On Wed, 13 Sep 2023 19:58:25 +0200 > Andrea Claudi 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 > > --- > > 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?
[Bridge] [PATCH iproute2-next 1/2] configure: add the --color option
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
---
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 < 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 Path to iproute2 include dir
--libdir Path to iproute2 lib dir
--libbpf_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: [Bridge] [PATCH iproute2-next 1/2] configure: add the --color option
On 9/15/23 9:59 AM, Stephen Hemminger wrote: > On Wed, 13 Sep 2023 19:58:25 +0200 > Andrea Claudi 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 >> --- > > 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: [Bridge] [PATCH iproute2-next 1/2] configure: add the --color option
On Wed, 13 Sep 2023 19:58:25 +0200 Andrea Claudi 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 > --- 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.
