[SSSD] [PATCH v2] CI: Enforce Valgrind check
Lukas Slebodnik
lslebodn at redhat.com
Wed Sep 17 16:48:47 UTC 2014
On (17/09/14 18:48), Nikolai Kondrashov wrote:
>On 09/17/2014 06:11 PM, Lukas Slebodnik wrote:
>>On (17/09/14 17:06), Nikolai Kondrashov wrote:
>>>--- a/Makefile.am
>>>+++ b/Makefile.am
>>>@@ -342,7 +342,8 @@ dist_noinst_DATA = \
>>> contrib/ci/configure.sh \
>>> contrib/ci/deps.sh \
>>> contrib/ci/distro.sh \
>>>- contrib/ci/misc.sh
>>>+ contrib/ci/misc.sh \
>>>+ contrib/ci/sssd.supp
>>We agreed to put $(NULL) at the end of list in Makefile.am
>>The benefit is that with next change just one line is added instead of 2+ 1-
>
>Sure. Updated patches attached. Thanks, Lukas.
>
>>We don't want to do mass changes, but you touch the list, si it is the best
>>time to do it.
>
>Hmm, I noticed that we practice such approach. Is it because of the merges?
>
>It seems to me it's just prolonging the pain and delaying the improvements.
>I would personally cut it off (do the mass change) and forget about it, but I
>probably never did that many merges.
>
>Nick
>From bcc5552e2c7955483f728e5b0bce75bef3ff34af Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Thu, 4 Sep 2014 14:04:15 +0300
>Subject: [PATCH 1/4] CI: Add Valgrind suppression support
>
>Add an empty Valgrind suppressions file, use it when invoking Valgrind.
>This prepares for addition of Valgrind suppressions for current false
>positives and issues that cannot be fixed, preparing for enforcing
>Valgrind check.
>
>Make Valgrind output a suppression for every error and make it output
>used suppression names and counts at the end of each run. This
>simplifies discovery and addition of new suppressions and removal of
>unused ones.
>
>Related to https://fedorahosted.org/sssd/ticket/2428
>---
> Makefile.am | 4 +++-
> contrib/ci/run | 8 ++++++--
> contrib/ci/sssd.supp | 9 +++++++++
> 3 files changed, 18 insertions(+), 3 deletions(-)
> create mode 100644 contrib/ci/sssd.supp
>
>diff --git a/Makefile.am b/Makefile.am
>index b19d698..d3ecd38 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -342,7 +342,9 @@ dist_noinst_DATA = \
> contrib/ci/configure.sh \
> contrib/ci/deps.sh \
> contrib/ci/distro.sh \
>- contrib/ci/misc.sh
>+ contrib/ci/misc.sh \
>+ contrib/ci/sssd.supp \
>+ $(NULL)
^^^
'\t' instead of spaces
Please follow our conventions.
>
> ###############################
> # Global compilation settings #
>diff --git a/contrib/ci/run b/contrib/ci/run
>index 9864818..4619eb3 100755
>--- a/contrib/ci/run
>+++ b/contrib/ci/run
>@@ -18,7 +18,8 @@
> # along with this program. If not, see <http://www.gnu.org/licenses/>.
>
> set -o nounset -o pipefail -o errexit
>-export PATH=`dirname "\`readlink -f \"\$0\"\`"`:$PATH
>+declare -r CI_DIR=`dirname "\`readlink -f \"\$0\"\`"`
>+export PATH=$CI_DIR:$PATH
> export LC_ALL=C
>
> . deps.sh
>@@ -206,7 +207,10 @@ function build_debug()
> --vgdb=no \
> --trace-children=yes \
> --trace-children-skip='*/bin/*,*/sbin/*' \
>- --leak-check=full ||
>+ --leak-check=full \
>+ --gen-suppressions=all \
>+ --suppressions="$CI_DIR/sssd.supp" \
>+ --verbose ||
> status=$?
> mv "$test_dir" ci-test-dir
>
>diff --git a/contrib/ci/sssd.supp b/contrib/ci/sssd.supp
>new file mode 100644
>index 0000000..d2541e8
>--- /dev/null
>+++ b/contrib/ci/sssd.supp
>@@ -0,0 +1,9 @@
>+#
>+# Valgrind suppression patterns
>+#
>+# See an introduction to suppressions in Valgrind manual:
>+# http://valgrind.org/docs/manual/manual-core.html#manual-core.suppress
>+#
>+# Each suppression name here must start with "sssd-" to differentiate it from
>+# suppressions maintained elsewhere.
>+#
>--
>2.1.0
>
>From 87272b8909049b6eb09973ad6cb022de5ac1a362 Mon Sep 17 00:00:00 2001
>From: Nikolai Kondrashov <Nikolai.Kondrashov at redhat.com>
>Date: Thu, 4 Sep 2014 17:09:31 +0300
>Subject: [PATCH 2/4] CI: Don't run dlopen-tests under Valgrind
>
>Disable running dlopen-tests under Valgrind as their use of dlclose
>makes Valgrind drop symbols and produce meaningless backtraces, which
>cannot be matched with specific suppressions.
>---
> contrib/ci/run | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/contrib/ci/run b/contrib/ci/run
>index 4619eb3..68ec1aa 100755
>--- a/contrib/ci/run
>+++ b/contrib/ci/run
>@@ -203,7 +203,8 @@ function build_debug()
> stage make-check-valgrind \
> make-check-wrap -j $CPU_NUM check -- \
> libtool --mode=execute \
>- valgrind-condense 99 '!(*.py)' -- \
>+ valgrind-condense 99 \
>+ '!(*.py|*dlopen-tests)' -- \
> --vgdb=no \
^^^^^^^^^
Why do we need to have disabled vgdb?
It is true that vgdb is enabled by default, but default value of vgdb-error is
999999999.
> --trace-children=yes \
> --trace-children-skip='*/bin/*,*/sbin/*' \
Could you explain your style of indentation here?
I cannot find any rule for indentation. Somewhere are 4 spaces, elsewhere 8 ...
As a result of this unusual indentation almost everything is near the 80
column limit.
----------------
You are mixing pates related to ticket #2428 with others
1st) contains: "Related to https://fedorahosted.org/sssd/ticket/2428"
3rd) contains: "Fixes https://fedorahosted.org/sssd/ticket/2428"
2nd, 4th) does not contain anything
It seems that you don't use our git commit template
@see .git-commit-template
@see git show 3d9edb4c510028def2df41aa7b0ce705b197e6fc
LS
More information about the sssd-devel
mailing list