[SSSD] [PATCH] CI: Improve usability of ci script

Lukas Slebodnik lslebodn at redhat.com
Fri Sep 12 07:22:42 UTC 2014


On (11/09/14 23:09), Nikolai Kondrashov wrote:
>On 09/11/2014 09:16 PM, Lukas Slebodnik wrote:
>>On (11/09/14 13:42), Roland Mainz wrote:
>>BTW: i would log all files to the one directory, but I didn't want break
>>backword compatibility. It could break other Nikolai's scrips.
>
>Thank you for considering that. Indeed, the Jenkins job archival configuration
>depends on file location somewhat, but it's easy enough to change.
>
>The current file layout was a compromise between making it logical and making
>code simple. We can discuss it separately.
>
>>0001-CI-Add-module-for-shared-variables.patch
>>
>> From 5dd91ab12a6514654cb234192a9401665b1e8065 Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik<lslebodn at redhat.com>
>>Date: Thu, 11 Sep 2014 09:59:22 +0200
>>Subject: [PATCH 1/2] CI: Add module for shared variables
>>
>>---
>>  Makefile.am                    |  4 +++-
>>  contrib/ci/run                 | 16 ++++++++--------
>>  contrib/ci/shared_variables.sh | 27 +++++++++++++++++++++++++++
>>  3 files changed, 38 insertions(+), 9 deletions(-)
>>  create mode 100644 contrib/ci/shared_variables.sh
>>
>>diff --git a/Makefile.am b/Makefile.am
>>index b19d6980383cb241fee52dfadc56733bd768ced2..bd54fe36d36a6df5a4aed70256972cfc9601a6e2 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/shared_variables.sh \
>>+    $(NULL)
>>
>>  ###############################
>>  # Global compilation settings #
>>diff --git a/contrib/ci/run b/contrib/ci/run
>>index c62b63799cb7142e8e243dbb76fd714512865a8c..ba4a446dec2082206a56e4c51831d7881cc36371 100755
>>--- a/contrib/ci/run
>>+++ b/contrib/ci/run
>>@@ -25,6 +25,7 @@ export LC_ALL=C
>>  . distro.sh
>>  . configure.sh
>>  . misc.sh
>>+. shared_variables.sh
>
>Um, "shared_variables.sh" module name is too long, too generic and doesn't
>really make much sense having, akin to having "code.sh".
>
You proposed name of module "src.sh". This name does not say anything.
We are no more in 80's in DOS world. There is no more limitation for name 8+3.
I prefer to have long self descriptive name.

>There is no need to put WORK_DIR in a module, as only contrib/ci/run uses it,
>but if you really want to have it in a module, together with SRC_DIR, please
>put both of them into "misc.sh", which is just the place for mixed things
>which don't have a module of their own (yet).
>
It was your request in mail [1] to move this variable into shared module.
There isn't reason to put them into module "misc.sh". The name of module
"misc.sh" evokes name "garbage" or "I don't know find proper name for module".
It's better to have self descriptive name
otherwise you need to look into the module itself  to find out what
it actualy does.

>Besides, I would prefer to keep the convention of having module name a prefix
>to the identifiers in it, with the exception of a single module like "misc.sh"
>(or "util.sh"), and "shared_variables.sh" would break that.
>
>>0002-CI-Improve-usability-of-ci-script.patch
>>
>> From 268e6bf82c902be146a0554a9873825a0b6a7281 Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik<lslebodn at redhat.com>
>>Date: Thu, 4 Sep 2014 22:09:23 +0200
>>Subject: [PATCH 2/2] CI: Improve usability of ci script
>
>Could you please make the subject more specific? It's a bit too general now.
>Something like "CI: Allow running outside source root" would be better, IMHO.
>
The title exactly says what is a purpose of this patch. The curret version of
CI script is inflexible and very limiting from user point of view. You have to
execute script from one particular directory. This is a very big limitation.
Attached patch remove this limitation and add ability to execute script from
any directory. For example, you can execute scipt from ramdisk and save
creating files in SSD disk. (I don't have one), but it is still faster to build
in ramdisk than in HDD. On the other hand, I can rephrase commit message to be
more descriptive.

>>CI scrip can be now executed from different directory than sssd git root.
>
>A nitpick: "scrip".
>Also a nitpick: it doesn't have to be a git root, just source root.
>
will fix

>>[build at host ci_dir]$ pwd
>>/usr/src/sssd/ci_dir
>>[build at host ci_dir]$ ../contrib/ci/run -n
>>autoreconf:             success  00:00:12 ci_dir/ci-autoreconf.log
>>DEBUG BUILD:                              ci_dir/ci-build-debug
>>configure:              success  00:00:14 ci_dir/ci-build-debug/ci-configure.log
>>make-tests:             success  00:01:03 ci_dir/ci-build-debug/ci-make-tests.log
>>make-check-valgrind:    failure  00:00:48 ci_dir/ci-build-debug/ci-make-check-valgrind.log
>>SUCCESS
>
>It seems that "ci_dir/" cannot be present in CI output with the proposed code.
>Could you please remove it? Otherwise it is confusing.
>
I was testing patch with argument "--prefix=ci_prefix" and than just replaced
this string with wrong one. It should have been removed.
Fixed in new version.

>>+declare LOG_DIR="$PWD"
>>+declare -r WORK_DIR="$PWD"
>
>I would really prefer "WORK_DIR" and "LOG_DIR" being called "WORK_ROOT" and
>"WORK_DIR" respectively, or something similar instead. Because LOG_DIR is
>always at or below WORK_DIR and they're used to point to directories not only
>containing log files, but also builds and reports.
>
In mail [1], you proposed either WORK_DIR or WORK_ROOT.
My preference is to use WORK_DIR. It is more suitable.

At the moment LOG_DIR is always at or below WORK_DIR, but there is no reason
for such limitation. The main problem is that you put log files and build
files into the same directory. With current version it will be easier
to separate them and have log files and build files in different directories,
but I did not want to break backward compatibility with your scripts.
On the other hand, patch would be simpler if I did it.

>Yes, right now LOG_DIR is only used in log output, but essentially it is
>pointing to a directory where current CI output goes and once we'll need to
>point to that directory while being in another one and output something else
>than log files there, we'll need that variable renamed.
I didn't get your point. "CI output" is not the same thing as log files.
In current version, "CI output" is supper set of ci log files.
It is not problem of attached patch, which slightly improve it.
It is a bad design decision of 1st version, which I didn't want to completely
break.

Patch with rephrased commit message is attached.

LS
-------------- next part --------------
>From f3d761d594fee392c6438f1de57cb85e78e99820 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 11 Sep 2014 09:59:22 +0200
Subject: [PATCH 1/2] CI: Add module for shared variables

---
 Makefile.am                    |  4 +++-
 contrib/ci/run                 | 16 ++++++++--------
 contrib/ci/shared_variables.sh | 27 +++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 9 deletions(-)
 create mode 100644 contrib/ci/shared_variables.sh

diff --git a/Makefile.am b/Makefile.am
index b19d6980383cb241fee52dfadc56733bd768ced2..bd54fe36d36a6df5a4aed70256972cfc9601a6e2 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/shared_variables.sh \
+    $(NULL)
 
 ###############################
 # Global compilation settings #
diff --git a/contrib/ci/run b/contrib/ci/run
index c62b63799cb7142e8e243dbb76fd714512865a8c..ba4a446dec2082206a56e4c51831d7881cc36371 100755
--- a/contrib/ci/run
+++ b/contrib/ci/run
@@ -25,6 +25,7 @@ export LC_ALL=C
 . distro.sh
 . configure.sh
 . misc.sh
+. shared_variables.sh
 
 declare -r DEBUG_CFLAGS="-g3 -O2"
 declare -r COVERAGE_CFLAGS="-g3 -O0 --coverage"
@@ -40,7 +41,6 @@ declare -r COVERAGE_MIN_FUNCS=0
 
 declare BASE_PFX=""
 declare DEPS=true
-declare BASE_DIR=`pwd`
 declare MODERATE=false
 declare RIGOROUS=false
 
@@ -65,13 +65,13 @@ Default options: --essential
 EOF
 }
 
-# Output a file display path: a path relocated from base directory (BASE_DIR)
+# Output a file display path: a path relocated from base directory (SRC_DIR)
 # to base prefix (BASE_PFX).
 # Args: path
 function disppath()
 {
     declare -r path=`readlink -f "$1"`
-    printf "%s" "$BASE_PFX${path:${#BASE_DIR}+1}"
+    printf "%s" "$BASE_PFX${path:${#SRC_DIR}+1}"
 }
 
 # Run a stage.
@@ -187,7 +187,7 @@ function build_debug()
     declare status
 
     test_dir=`mktemp --directory /dev/shm/ci-test-dir.XXXXXXXX`
-    stage configure         "$BASE_DIR/configure" \
+    stage configure         "$SRC_DIR/configure" \
                                 "${CONFIGURE_ARG_LIST[@]}" \
                                 --with-test-dir="$test_dir"
 
@@ -246,7 +246,7 @@ function build_coverage()
     export CFLAGS="$COVERAGE_CFLAGS"
 
     test_dir=`mktemp --directory /dev/shm/ci-test-dir.XXXXXXXX`
-    stage configure         scan-build "$BASE_DIR/configure" \
+    stage configure         scan-build "$SRC_DIR/configure" \
                                            "${CONFIGURE_ARG_LIST[@]}" \
                                            --with-test-dir="$test_dir"
 
@@ -265,14 +265,14 @@ function build_coverage()
                             true
 
     stage lcov-pre          lcov --capture --initial --directory . \
-                                 --base-directory "$BASE_DIR" \
+                                 --base-directory "$SRC_DIR" \
                                  --output-file ci-base.info
     # Run tests
     stage make-check        scan-build make -j $CPU_NUM check || true
     mv "$test_dir" ci-test-dir
 
     stage lcov-post         lcov --capture --directory . \
-                                 --base-directory "$BASE_DIR" \
+                                 --base-directory "$SRC_DIR" \
                                  --output-file ci-check.info
     stage lcov-merge        lcov --add-tracefile ci-base.info \
                                  --add-tracefile ci-check.info \
@@ -283,7 +283,7 @@ function build_coverage()
     stage genhtml           eval 'genhtml --output-directory \
                                           "$coverage_report_dir" \
                                           --title "sssd" --show-details \
-                                          --legend --prefix "$BASE_DIR" \
+                                          --legend --prefix "$SRC_DIR" \
                                           ci.info |& tee ci-genhtml.out'
     printf "%-$((TITLE_WIDTH + RESULT_WIDTH))s%s\n" \
             "coverage report:" \
diff --git a/contrib/ci/shared_variables.sh b/contrib/ci/shared_variables.sh
new file mode 100644
index 0000000000000000000000000000000000000000..59d31efd5b6c052fe2d08d7398d19720198c2c29
--- /dev/null
+++ b/contrib/ci/shared_variables.sh
@@ -0,0 +1,27 @@
+#
+# Shared variables.
+#
+#    Authors:
+#        Lukas Slebodnik <lslebodn at redhat.com>
+#
+#    Copyright (C) 2014 Red Hat
+#
+#    This program is free software; you can redistribute it and/or modify
+#    it under the terms of the GNU General Public License as published by
+#    the Free Software Foundation; either version 3 of the License, or
+#    (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU General Public License for more details.
+#
+#    You should have received a copy of the GNU General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if [ -z ${_SHARED_VARIABLES_SH+set} ]; then
+declare -r _SHARED_VARIABLES_SH=
+
+declare -r SRC_DIR="$(readlink -f "$(dirname "${BASH_SOURCE[0]}")/../..")"
+
+fi # _SHARED_VARIABLES_SH
-- 
2.1.0

-------------- next part --------------
>From 9b515ab2137b116afaa7d053bda5ccd20f45d004 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Thu, 4 Sep 2014 22:09:23 +0200
Subject: [PATCH 2/2] CI: Improve usability of ci script

The CI script is very limiting from user point of view. You have to execute
script from one particular directory. This is a big limitation.
Attached patch remove this it and add ability to execute script from
any directory. For example, you can execute script from ramdisk and save
creating files in SSD disk.

[build at host ci_dir]$ pwd
/usr/src/sssd/ci_dir
[build at host ci_dir]$ ../contrib/ci/run -n
autoreconf:             success  00:00:12 ci-autoreconf.log
DEBUG BUILD:                              ci-build-debug
configure:              success  00:00:14 ci-build-debug/ci-configure.log
make-tests:             success  00:01:03 ci-build-debug/ci-make-tests.log
make-check-valgrind:    failure  00:00:48 ci-build-debug/ci-make-check-valgrind.log
SUCCESS

Resolves:
https://fedorahosted.org/sssd/ticket/2429
---
 contrib/ci/deps.sh             |  3 ++-
 contrib/ci/run                 | 16 ++++++++++++----
 contrib/ci/shared_variables.sh |  1 +
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh
index 41acbc27bacefff80c18feaae1f6d56352eecca5..7fbb60a473e50c4d33d492598dc41816f4e7309b 100644
--- a/contrib/ci/deps.sh
+++ b/contrib/ci/deps.sh
@@ -20,6 +20,7 @@ if [ -z ${_DEPS_SH+set} ]; then
 declare -r _DEPS_SH=
 
 . distro.sh
+. shared_variables.sh
 
 # Dependency list
 declare -a DEPS_LIST=(
@@ -39,7 +40,7 @@ if [[ "$DISTRO_BRANCH" == -redhat-* ]]; then
     _DEPS_LIST_SPEC=`
         sed -e 's/@PACKAGE_VERSION@/0/g' \
             -e 's/@PACKAGE_NAME@/package-name/g' \
-            -e 's/@PRERELEASE_VERSION@//g' contrib/sssd.spec.in |
+            -e 's/@PRERELEASE_VERSION@//g' "$SRC_DIR/contrib/sssd.spec.in" |
             rpm-spec-builddeps /dev/stdin`
     readarray -t -O "${#DEPS_LIST[@]}" DEPS_LIST <<<"$_DEPS_LIST_SPEC"
 fi
diff --git a/contrib/ci/run b/contrib/ci/run
index ba4a446dec2082206a56e4c51831d7881cc36371..e9566df610d6a4f2939c51aa694839ca8ff86331 100755
--- a/contrib/ci/run
+++ b/contrib/ci/run
@@ -39,6 +39,7 @@ declare -r COVERAGE_MIN_LINES=15
 # Minimum percentage of code functions covered by tests
 declare -r COVERAGE_MIN_FUNCS=0
 
+declare LOG_DIR="$PWD"
 declare BASE_PFX=""
 declare DEPS=true
 declare MODERATE=false
@@ -65,13 +66,13 @@ Default options: --essential
 EOF
 }
 
-# Output a file display path: a path relocated from base directory (SRC_DIR)
-# to base prefix (BASE_PFX).
+# Output a file display path: a path relocated from working directory
+# (WORK_DIR) to base prefix (BASE_PFX).
 # Args: path
 function disppath()
 {
     declare -r path=`readlink -f "$1"`
-    printf "%s" "$BASE_PFX${path:${#SRC_DIR}+1}"
+    printf "%s" "$BASE_PFX${path:${#WORK_DIR}+1}"
 }
 
 # Run a stage.
@@ -79,7 +80,7 @@ function disppath()
 function stage()
 {
     declare -r id="$1";     shift
-    declare -r log="ci-$id.log"
+    declare -r log="$LOG_DIR/ci-$id.log"
     declare status
     declare start
     declare end
@@ -302,14 +303,17 @@ function run_build()
 {
     declare -r id="$1"; shift
     declare -r dir="ci-build-$id"
+    declare -r LOG_DIR_BACKUP="$LOG_DIR"
 
     mkdir "$dir"
     printf "%-$((TITLE_WIDTH + RESULT_WIDTH))s%s\n" \
            "${id^^} BUILD:" "`disppath \"\$dir\"`"
 
+    LOG_DIR="$LOG_DIR/$dir"
     cd "$dir"
     "$@"
     cd ..
+    LOG_DIR="$LOG_DIR_BACKUP"
 }
 
 #
@@ -357,7 +361,11 @@ export V=1
 if "$DEPS"; then
     stage install-deps  deps_install
 fi
+
+pushd "$SRC_DIR" >/dev/null
 stage autoreconf    autoreconf --install --force
+popd >/dev/null
+
 run_build debug     build_debug
 if "$RIGOROUS"; then
     run_build coverage  build_coverage
diff --git a/contrib/ci/shared_variables.sh b/contrib/ci/shared_variables.sh
index 59d31efd5b6c052fe2d08d7398d19720198c2c29..e29b17dd0fefb3d3d0565d14e148b900672c38df 100644
--- a/contrib/ci/shared_variables.sh
+++ b/contrib/ci/shared_variables.sh
@@ -23,5 +23,6 @@ if [ -z ${_SHARED_VARIABLES_SH+set} ]; then
 declare -r _SHARED_VARIABLES_SH=
 
 declare -r SRC_DIR="$(readlink -f "$(dirname "${BASH_SOURCE[0]}")/../..")"
+declare -r WORK_DIR="$PWD"
 
 fi # _SHARED_VARIABLES_SH
-- 
2.1.0



More information about the sssd-devel mailing list