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

Nikolai Kondrashov Nikolai.Kondrashov at redhat.com
Sun Sep 7 12:00:34 UTC 2014


Hi Lukas,

I thought we agreed on yesterday's meeting we shouldn't do this?

Anyway, I admit that the ability to run from arbitrary directory might be
convenient.

Please see my comments below.

On 09/05/2014 05:18 PM, Lukas Slebodnik wrote:
>  From a466251fb4d653300ad1d78c4c8755f6cb9a3045 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] CI: Improve usability of ci script

Could you please make the subject more specific?
It's a bit too general now.

> CI scrip can be now executed from different directory than sssd git root.
>
> [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

The paths in the output above don't seem possible with the patch code.
Unless I'm missing something, could you please fix them?

>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2429
>
> ---
>   contrib/ci/deps.sh |  2 +-
>   contrib/ci/run     | 16 +++++++++++++---
>   2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/ci/deps.sh b/contrib/ci/deps.sh
> index 41acbc27bacefff80c18feaae1f6d56352eecca5..791761af50237909476d7d37ba4114c212de299d 100644
> --- a/contrib/ci/deps.sh
> +++ b/contrib/ci/deps.sh
> @@ -39,7 +39,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' $BASE_DIR/contrib/sssd.spec.in |

This makes the module depend on the executable that uses it and will make the
program harder to understand and debug.

Could you please make a module, say named "src.sh", put "BASE_DIR" variable
there, but under "SRC_DIR" name, and source that module from here and
everywhere else it is needed? If you do this, please follow the conventions of
other modules.

Alternatively, just copy the variable declaration to deps.sh, but give it a
"DEPS_" prefix.

Thank you.

>               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 c62b63799cb7142e8e243dbb76fd714512865a8c..fe84ad26ce21292a4abef999f32775d5d95cd65c 100755
> --- a/contrib/ci/run
> +++ b/contrib/ci/run
> @@ -21,6 +21,10 @@ set -o nounset -o pipefail -o errexit
>   export PATH=`dirname "\`readlink -f \"\$0\"\`"`:$PATH
>   export LC_ALL=C
>
> +declare -r BASE_DIR="$(cd -P "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)"

I would say, that "readlink -f" would be simpler than cd && pwd. I.e.:

     declare -r BASE_DIR="$(readlink -f "$(dirname "${BASH_SOURCE[0]}")/../..")"

> +declare -r CWD_DIR="$(pwd)"
> +declare LOG_DIR="$(pwd)"

Can you name these "WORK_ROOT" and "WORK_DIR" instead? "CWD_DIR" is wrong,
because current working directory will change during the execution. "LOG_DIR"
is wrong, because not only logs are put there, but also builds and reports.

Also, if you make deps.sh independent of the executable's BASE_DIR
declaration, you can move these three declarations down to the other globals.

A minor improvement could also be to use "$PWD" instead of "$(pwd)" as the
latter would avoid spawning a subshell.

Thanks, Lukas.

Sincerely,
Nick



More information about the sssd-devel mailing list