On 11.12.2015 09:22, Lukas Slebodnik wrote:
> On (10/12/15 18:04), Petr Spacek wrote:
>> On 9.12.2015 15:30, Petr Spacek wrote:
>>> Hello,
>>>
>>> this patch automates some of sanity checks proposed by Petr Vobornik.
>>>
>>> 'make review' should be used in root of clean Git tree which has patches 
>>> under
>>> review applied.
>>>
> +1 for automatisation
> -1 for name.
> 
> Your script does not review[1] anything. Code review(peer review) is a job
> for humans. What do you think about alternative
> names: "review-helper", "sanity-check" ...

I like short name but I respect your point, so I've updated the script to print:
"No problems detected using lint, pep8, ./make{api,aci}, and VERSION" at the 
end.
Now it should be clear what it did, so reviewer knows what steps were already
done and what can be skipped during further manual review.

Attached patch also adds 'assert-clean-tree' target which is run before lint
target, so no time is wasted if the tree is not clean. Parallel build will
execute and terminate both targets at the same time, but it saves time in both
cases, the output is just not so nice.

And IPA cannot be built in parallel anyway :-)

>>> Magic in review.sh attempts to detect nearest remote branch which can be 
>>> used
>>> as diff base for review. Please see review.sh for further details.
>>
>> And here is the patch! :-)
>>
>> -- 
>> Petr^2 Spacek
> 
> [1] https://en.wikipedia.org/wiki/Code_review
> 
>>From 52393bf2bb0ad74dbe37e496b1fd41a6ab22bd90 Mon Sep 17 00:00:00 2001
>> From: Petr Spacek <pspa...@redhat.com>
>> Date: Tue, 8 Dec 2015 12:06:33 +0100
>> Subject: [PATCH] Add 'review' target for make. It automates following tasks:
>>
>> - check if ACI.txt and API.txt are up-to-date
>> - check if VERSION was changed if API was changed
>> - pep8 --diff does not produce new errors when ran on diff from origin/branch
>> - make lint does not produce errors
>> ---
>> Makefile  |  4 ++++
>> review.sh | 64 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 68 insertions(+)
>> create mode 100755 review.sh
>>
>> diff --git a/Makefile b/Makefile
>> index 
>> d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..b85b3a5d6362150dcebf16745414c3d416c9547f
>>  100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -79,6 +79,10 @@ check: bootstrap-autogen server tests
>>              (cd $$subdir && $(MAKE) check) || exit 1; \
>>      done
>>
>> +# works only in Git tree
>> +review: version-update
>> +    ./review.sh
>> +
>> bootstrap-autogen: version-update client-autogen
>>      @echo "Building IPA $(IPA_VERSION)"
>>      cd asn1; if [ ! -e Makefile ]; then ../autogen.sh --prefix=/usr 
>> --sysconfdir=/etc --localstatedir=/var --libdir=$(LIBDIR); fi
>> diff --git a/review.sh b/review.sh
>> new file mode 100755
>> index 
>> 0000000000000000000000000000000000000000..3a5114fbe88d3aba8e926e049500f5ac5f41a83e
>> --- /dev/null
>> +++ b/review.sh
>> @@ -0,0 +1,64 @@
>> +#!/bin/bash
>> +is_tree_clean() {
>> +    test "$(git status --porcelain "$@")" == ""
>> +    return $?
>> +}
>> +
>> +log_error() {
>> +    echo "ERROR: ${1}, continuing ..."
>> +    errs=(${errs[@]} "ERROR: ${1}\n")
>> +}
>> +
>> +set -o errexit -o nounset #-o xtrace
>> +
>> +LOGFILE="$(mktemp --suff=.log)"
>> +exec > >(tee -i ${LOGFILE})
>> +exec 2>&1
>> +
>> +echo -n "make lint is running ... "
>> +make --silent lint || log_error "make lint failed"
>> +
>> +# Go backwards in history until you find a remote branch from which current
>> +# branch was created. This will be used as base for git diff.
>> +PATCHCNT=0
>> +BASEBRANCH=""
> If base branch means the remote branch which was used for cloning the cerrent
> git HEAD than you can simplify it a littl bit.
> git rev-parse --symbolic-full-name @{upstream}

Nice! I did not know this trick. Unfortunately it does not work for my
use-case: I clone upstream repo, pick a branch to base review on, create my
own branch from the upstream one, and then apply patches on top of that.

Imagine that I'm going to run the tool on branch 'review-tool'. Git history
tree looks like this:

* d036f3e (HEAD -> review-tool) fixup! Add 'review' target for make.
* c6d75b0 Makefile: disable parallel build
| * 667d50b (dkupka-review) dns: Add --auto-reverse option.
| * c85f176 dns: Check if domain already exists.
| * a55cd76 dns: do not add (forward)zone if it is already resolvable.
| * 0b0c529 (dns-no-forwarders) DNS: Make --no-forwarders option default.
|/
* d2e8470 Add 'review' target for make. It automates following tasks:
| * 101ffae (mbasti-review) Fix DNS tests: dns-resolve returns warning
| * 52393bf Add 'review' target for make. It automates following tasks:
|/
* 848912a (origin/master, master) add missing /ipaplatform/constants.py to
.gitignore

$ git rev-parse --symbolic-full-name @{upstream}
errors out like this:
fatal: no upstream configured for branch 'review-tool'

I definitely agree that the code above is just an ugly hack, but I would like
to keep current behavior so I do not need to change my workflow.

>> +CURRBRANCH="$(git branch --remote --contains)"
>> +while [ "${BASEBRANCH}" == "" ]
>> +do
>> +    BASEBRANCH="$(git branch --remote --contains "HEAD~${PATCHCNT}" | grep 
>> -v "^. ${CURRBRANCH}$" || :)"
>> +    PATCHCNT="$(expr "${PATCHCNT}" + 1)"
> Then for patch count you can use
> PATCHCNT=$(git log --oneline $BASEBRANCH..HEAD | wc -l)

-- 
Petr^2 Spacek
From 277d2d252d7193a8e44579029af37278eaf8ec30 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Thu, 10 Dec 2015 18:35:20 +0100
Subject: [PATCH] Makefile: disable parallel build

IPA build system cannot cope with parallel build anyway, so this patch
disables parallel build explicitly so it does not blow up when user
has -j specified in default MAKEOPTS.
---
 Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Makefile b/Makefile
index d2c37f1597a011af2bd9ef1a4f7ce87ac59620a3..715844bdacb737ee423b1fe512bacc803159e4d5 100644
--- a/Makefile
+++ b/Makefile
@@ -1,3 +1,6 @@
+# IPA build system cannot cope with parallel build; disable parallel build
+.NOTPARALLEL:
+
 include VERSION
 
 SUBDIRS=asn1 daemons install ipapython ipa-client
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to