| Message ID | 20231109162304.119506-2-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | update tests/shell for 5.4 kernels | expand |
On Thu, 2023-11-09 at 17:22 +0100, Pablo Neira Ayuso wrote: > export DIFF so it can be used from feature scripts to probe the > kernel. > > +DIFF="$(which diff)" > +if [ ! -x "$DIFF" ] ; then > + DIFF=true > +fi > +export DIFF what is the purpose of having $DIFF variable at all? Why not require to have `diff` installed? Maybe that justification is somewhere in the history of the project. If so, could you drop one line in the commit message what the point is? Thomas
On Thu, Nov 09, 2023 at 06:49:21PM +0100, Thomas Haller wrote: > On Thu, 2023-11-09 at 17:22 +0100, Pablo Neira Ayuso wrote: > > export DIFF so it can be used from feature scripts to probe the > > kernel. > > > > +DIFF="$(which diff)" > > +if [ ! -x "$DIFF" ] ; then > > + DIFF=true > > +fi > > +export DIFF > > > what is the purpose of having $DIFF variable at all? > Why not require to have `diff` installed? > > Maybe that justification is somewhere in the history of the project. If > so, could you drop one line in the commit message what the point is? It is all available in git annotate: 68310ba0f9c2 ("tests: shell: Search diff tool once and for all") 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification") I just need to move it around so I can use it from feature scripts. If you prefer I can just use 'diff' instead from the feature scripts.
On Thu, 2023-11-09 at 20:14 +0100, Pablo Neira Ayuso wrote: > On Thu, Nov 09, 2023 at 06:49:21PM +0100, Thomas Haller wrote: > > On Thu, 2023-11-09 at 17:22 +0100, Pablo Neira Ayuso wrote: > > > export DIFF so it can be used from feature scripts to probe the > > > kernel. > > > > > > +DIFF="$(which diff)" > > > +if [ ! -x "$DIFF" ] ; then > > > + DIFF=true > > > +fi > > > +export DIFF > > > > > > what is the purpose of having $DIFF variable at all? > > Why not require to have `diff` installed? > > > > Maybe that justification is somewhere in the history of the > > project. If > > so, could you drop one line in the commit message what the point > > is? > > It is all available in git annotate: > > 68310ba0f9c2 ("tests: shell: Search diff tool once and for all") > 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification") First use of $DIFF comes from 3fb3bb603374 ('tests/listing: add some listing tests') which says: In order to ease debug in case of failure, if the diff tool is in the system, then a textual diff is printed. With the `true` fallback, checks are skipped. It would be possible, to use a fallback that still checks for equality (albeit without fancy diff output). But really. Just require everybody to install a diff program. > > I just need to move it around so I can use it from feature scripts. > If you prefer I can just use 'diff' instead from the feature scripts. Sure. The patch is fine. I think one day, sed 's/\$DIFF\>/diff/g' -i $(git grep -l DIFF tests/shell/) should be done. Thomas
Thomas Haller <thaller@redhat.com> wrote: > I think one day, > > sed 's/\$DIFF\>/diff/g' -i $(git grep -l DIFF tests/shell/) > > should be done. FWIW I agree with this.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > I just need to move it around so I can use it from feature scripts. > If you prefer I can just use 'diff' instead from the feature scripts. Seems better to just use 'diff'.
diff --git a/tests/shell/run-tests.sh b/tests/shell/run-tests.sh index 27a0ec43042a..e51d51c9539b 100755 --- a/tests/shell/run-tests.sh +++ b/tests/shell/run-tests.sh @@ -572,6 +572,12 @@ feature_probe() return 1 } +DIFF="$(which diff)" +if [ ! -x "$DIFF" ] ; then + DIFF=true +fi +export DIFF + for feat in "${_HAVE_OPTS[@]}" ; do var="NFT_TEST_HAVE_$feat" if [ -z "${!var+x}" ] ; then @@ -590,11 +596,6 @@ if [ "$NFT_TEST_JOBS" -eq 0 ] ; then fi fi -DIFF="$(which diff)" -if [ ! -x "$DIFF" ] ; then - DIFF=true -fi - declare -A JOBS_PIDLIST _NFT_TEST_VALGRIND_VGDB_PREFIX=
export DIFF so it can be used from feature scripts to probe the kernel. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- tests/shell/run-tests.sh | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)