Skip to content

Disable Valgrind by default during testing #3269

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.

Sign up for GitHub

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

Merged
itchyny merged 1 commit into from
Mar 4, 2025
+19 −29
Merged

Disable Valgrind by default during testing #3269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ jobs:
./configure \
--host=${{ matrix.CC }} \
--disable-docs \
--disable-valgrind \
--with-oniguruma=builtin \
--enable-static \
--enable-all-static \
Expand Down Expand Up @@ -154,7 +153,6 @@ jobs:
./configure \
--host="${{ matrix.target }}$(uname -r)" \
--disable-docs \
--disable-valgrind \
--with-oniguruma=builtin \
--enable-static \
--enable-all-static \
Expand Down Expand Up @@ -230,7 +228,6 @@ jobs:
autoreconf -i
./configure \
--disable-docs \
--disable-valgrind \
--with-oniguruma=builtin \
--disable-shared \
--enable-static \
Expand Down Expand Up @@ -276,7 +273,6 @@ jobs:
autoreconf -i
./configure \
--disable-docs \
--disable-valgrind \
--with-oniguruma=builtin
make distcheck
make dist dist-zip
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/oniguruma.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ jobs:
autoreconf -i
./configure \
--disable-docs \
--enable-valgrind \
--with-oniguruma=yes
make -j"$(nproc)"
file ./jq
Expand Down Expand Up @@ -54,6 +55,7 @@ jobs:
autoreconf -i
./configure \
--disable-docs \
--enable-valgrind \
--with-oniguruma=no
make -j"$(nproc)"
file ./jq
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/scanbuild.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ jobs:
MAKEVARS: ${{ matrix.makevars }}
run: |
autoreconf -i
./configure --with-oniguruma=builtin $COVERAGE
./configure --with-oniguruma=builtin --enable-valgrind $COVERAGE
scan-build --keep-going make -j4
- name: Test
env:
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/valgrind.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ jobs:
autoreconf -i
./configure \
--disable-docs \
--enable-valgrind \
--with-oniguruma=builtin
make -j"$(nproc)"
file ./jq
Expand Down
1 change: 0 additions & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ COPY . /app
RUN autoreconf -i \
&& ./configure \
--disable-docs \
--disable-valgrind \
--with-oniguruma=builtin \
--enable-static \
--enable-all-static \
Expand Down
23 changes: 11 additions & 12 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,23 +65,18 @@ endif

include_HEADERS = src/jv.h src/jq.h

if ENABLE_UBSAN
AM_CFLAGS += -fsanitize=undefined
endif

AM_CPPFLAGS = -I$(srcdir)/src -I$(srcdir)/vendor

### Running tests under Valgrind
### Address sanitizer (ASan)

if ENABLE_ASAN
AM_CFLAGS += -fsanitize=address
NO_VALGRIND = 1
else
if ENABLE_VALGRIND
NO_VALGRIND =
else
NO_VALGRIND = 1
endif

### Undefined Behavior Sanitizer

if ENABLE_UBSAN
AM_CFLAGS += -fsanitize=undefined
endif

### Code coverage with gcov
Expand Down Expand Up @@ -127,7 +122,11 @@ TESTS = tests/mantest tests/jqtest tests/shtest tests/utf8test tests/base64test
if !WIN32
TESTS += tests/optionaltest
endif
AM_TESTS_ENVIRONMENT = JQ=$(abs_builddir)/jq NO_VALGRIND=$(NO_VALGRIND)

AM_TESTS_ENVIRONMENT = JQ=$(abs_builddir)/jq
if ENABLE_VALGRIND
AM_TESTS_ENVIRONMENT += ENABLE_VALGRIND=1
endif

# This is a magic make variable that causes it to treat tests/man.test as a
# DATA-type dependency for the check target. As a result, it will attempt to
Expand Down
13 changes: 3 additions & 10 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ if test "$USE_MAINTAINER_MODE" = yes; then
AC_CHECK_PROGS(LEX, flex lex)
fi

dnl Check for valgrind
AC_CHECK_PROGS(valgrind_cmd, valgrind)
if test "x$valgrind_cmd" = "x" ; then
AC_MSG_WARN([valgrind is required to test jq.])
fi
AC_CHECK_FUNCS(memmem)

AC_CHECK_HEADER("sys/cygwin.h", [have_cygwin=1;])
Expand All @@ -54,10 +49,9 @@ dnl Running tests with Valgrind is slow. It is faster to iterate on
dnl code without Valgrind until tests pass, then enable Valgrind and
dnl fix leaks.
AC_ARG_ENABLE([valgrind],
AS_HELP_STRING([--disable-valgrind],[do not run tests under Valgrind]))
AS_HELP_STRING([--enable-valgrind],[enable Valgrind during testing]))
Copy link
Member

@wader wader Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to understand: With this configure will not check if valgrind is actually available when using --enable-valgrind?

Copy link
Contributor Author

@itchyny itchyny Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but in the setup script the command availability is checked already.

Copy link
Member

@wader wader Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, the check in tests/setup? as i understand the will silently skip using valgrind if missing?

Copy link
Contributor Author

@itchyny itchyny Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. The behavior is not changed in this PR.

Copy link
Member

@wader wader Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i guess it could possible fail if valgrind was explicitly enabled but missing? but if so could be done in a separate PR i think


dnl Running tests with Valgrind is slow; address sanitizer (ASAN) is
dnl faster.
dnl Address sanitizer (ASan)
AC_ARG_ENABLE([asan],
AS_HELP_STRING([--enable-asan],[enable address sanitizer]))

Expand Down Expand Up @@ -114,7 +108,7 @@ AS_IF([test "x$enable_decnum" != "xno"],[
AC_DEFINE([USE_DECNUM], 1, [Define to enable decnum support.])
])

AM_CONDITIONAL([ENABLE_VALGRIND], [test "x$enable_valgrind" != xno])
AM_CONDITIONAL([ENABLE_VALGRIND], [test "x$enable_valgrind" = xyes])
AM_CONDITIONAL([ENABLE_ASAN], [test "x$enable_asan" = xyes])
AM_CONDITIONAL([ENABLE_UBSAN], [test "x$enable_ubsan" = xyes])
AM_CONDITIONAL([ENABLE_GCOV], [test "x$enable_gcov" = xyes])
Expand Down Expand Up @@ -291,7 +285,6 @@ AC_SUBST(onig_LDFLAGS)

AM_CONDITIONAL([BUILD_ONIGURUMA], [test "x$build_oniguruma" = xyes])
AM_CONDITIONAL([WITH_ONIGURUMA], [test "x$with_oniguruma" != xno])
AC_SUBST([BUNDLER], ["$bundle_cmd"])

AC_CONFIG_MACRO_DIRS([config/m4 m4])
AC_CONFIG_HEADERS([src/config.h])
Expand Down
2 changes: 1 addition & 1 deletion tests/setup
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ JQ=${JQ:-$JQBASEDIR/jq}
LC_ALL=C
export LC_ALL

if [ -z "${NO_VALGRIND-}" ] && which valgrind > /dev/null; then
if [ -n "${ENABLE_VALGRIND-}" ] && which valgrind > /dev/null; then
VALGRIND="valgrind --error-exitcode=1 --leak-check=full \
--suppressions=$JQTESTDIR/onig.supp \
--suppressions=$JQTESTDIR/local.supp"
Expand Down
Loading