Change in vdsm[master]: build: Forbid bare exepct

[Nir Soffer]

Nir Soffer has uploaded a new change for review.

Change subject: build: Forbid bare exepct
......................................................................

build: Forbid bare exepct

Using bare except is bad practice and is never required. We should
disallow this in new code. This patch ensure that your build will break
if your patch added a bare except.

Change-Id: I5b7f33b6eb1c90610b3a5d053ba65ce3fc6b74fa
Signed-off-by: Nir Soffer 
---
M Makefile.am
1 file changed, 4 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/56/22456/1

diff --git a/Makefile.am b/Makefile.am
index 9b1a3c9..d25d395 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -79,6 +79,10 @@
    'lv' from line .*"

check-local:
+	if git show | grep 'except:'; then \
+		echo "error: empty except: is not allowed!"; \
+		exit 1; \
+	fi
    find . -path './.git' -prune -type f -o \
    	-name '*.py' -o -name '*.py.in'  | xargs $(PYFLAKES) | \
    	grep -w -v $(SKIP_PYFLAKES_ERR) | \

Antoni Segura Puimedon has posted comments on this change.

Change subject: build: Forbid bare exepct
......................................................................

Patch Set 1:

(1 comment)

....................................................
Commit Message
Line 3: AuthorDate: 2013-12-17 12:14:23 +0200
Line 4: Commit:     Nir Soffer 
Line 5: CommitDate: 2013-12-17 12:14:23 +0200
Line 6: 
Line 7: build: Forbid bare exepct
s/exepct/except/
Line 8: 
Line 9: Using bare except is bad practice and is never required. We should
Line 10: disallow this in new code. This patch ensure that your build will break
Line 11: if your patch added a bare except.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare exepct
......................................................................

Patch Set 1:

The implementation is little simplistic, which will cause this commit to be rejected by jenkins :-)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: build: Forbid bare exepct
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6119/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5332/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6222/ : FAILURE

Antoni Segura Puimedon has posted comments on this change.

Change subject: build: Forbid bare exepct
......................................................................

Patch Set 1:

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | grep 'except:'; then \
grep returns 0 when found and 1 otherwise. Shouldn't you exit and show error in the "0" case.
Line 83: 		echo "error: empty except: is not allowed!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare exepct
......................................................................

Patch Set 1: Verified+1

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | grep 'except:'; then \
Thats what I do - and it works.
Line 83: 		echo "error: empty except: is not allowed!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Change in vdsm[master]: build: Forbid bare except

Antoni Segura Puimedon has posted comments on this change.

Change subject: build: Forbid bare except
......................................................................

Patch Set 2: Code-Review+1

Change in vdsm[master]: build: Forbid bare except

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: build: Forbid bare except
......................................................................

Patch Set 2: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6120/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5333/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6223/ : FAILURE

Change in vdsm[master]: build: Forbid bare except:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

Smarten the check so it breaks only when you add forbidden code.

Change in vdsm[master]: build: Forbid bare except:

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6121/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5334/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6224/ : SUCCESS

Change in vdsm[master]: build: Forbid bare except:

Antoni Segura Puimedon has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3: Code-Review+1

Change in vdsm[master]: build: Forbid bare except:

Saggi Mizrahi has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

disregarding the fact that creating false positive that breaks the build is extremely easy.

Having a documentation file that says:

You are allowed to do stuff
except:
1. A

Would break the build.

Also I think you meant to use git diff and not git show.

Further more since it only really works for Jenkins it should be a Jenkins job. Having it in the build would only annoy people while they are developing and create weird situations since not all is comitted and sometimes people commit broken code locally.

Change in vdsm[master]: build: Forbid bare except:

Yaniv Bronhaim has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

jenkins job with that is redundant, currently we don't check syntax during the make, that's why we have pep8 and pyflaks running on the py files..if pep8 finds it valid, we also do.. document it as a standard and -1 on it is valid as well, but we can't make it mandatory imo

Change in vdsm[master]: build: Forbid bare except:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

Sagi: we can add smarter check, for example, parse git show output to detect the edited file, and apply this rule only to python files. But I don't think it worth the time.

Yaniv: we set the rules of this project, not pep8 gods :-)

Change in vdsm[master]: build: Forbid bare except:

Yaniv Bronhaim has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

Nir: where do we set such rules except the pep8 run? maybe im not aware of it

Change in vdsm[master]: build: Forbid bare except:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

We run pep8 and pylint. Running these tools and breaking the build when they complain is *our* choice. pep8 is just some rules that people like you and me set.

Change in vdsm[master]: build: Forbid bare except:

Yaniv Bronhaim has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

sounds like a way to declare standards

Change in vdsm[master]: build: Forbid bare except:

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3: Code-Review-1

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^+\s+except:\s*(#.+)?$$'; then \
It's gonna be painful for someone who just wants to change an unrelated issue in a line with "except", and force him to replace it with an "except Exception:" which is not really better.

How about having this as a boldface WARNING?

In any case, please make this test not fail for the (unfortunately rare) user building from tarball, with no git tree around.
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Change in vdsm[master]: build: Forbid bare except:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^+\s+except:\s*(#.+)?$$'; then \
Considering the amount of pain we already cause by enforcing pep8 whitespace rules, which many time make the code less readable, I don't think we add much pain with fixing a bare except once in a while.

A bare "except:" is worse then "except Exception:", because it catches SystemExit and KeyboardInterrupt, which is almost never your intent. For example:

while 1:
        try:
            time.sleep(10)
            sys.exit() # panic
        except:
            print 'nee!'

A warning is not very effective because it will be lost in the sea of messages during a build. And it requires much more work because you would like a warning for each location, and not one error on the first error.

I agree that this must use git only if git is installed, and when running inside a git repository. I think that the current implementation do work as expected - if git is not installed, or git show fails, we will not get into the if.

It seems that the right place for this is in tool like pyflakes.
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Change in vdsm[master]: build: Forbid bare except:

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^+\s+except:\s*(#.+)?$$'; then \
Indeed, too bad pyflakes does not have this kind of syntax-sensitive check.

(I know that "except Exception" is better. It's just not "really better". True improvement would come only by manual conversion of these catch-all clauses to something specific.)

You are right that with no git around, the check does not fail (I missed that), but it adds another drop to the sea of build messages. Could you avoid that?

I think Saggi mentioned another issue: if you `make check-local` on a dirty repository, you check someone else's code, not your own. How about doing

git diff || git show

?
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Change in vdsm[master]: build: Forbid bare except:

Saggi Mizrahi has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

(1 comment)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^+\s+except:\s*(#.+)?$$'; then \
The reason we accept the will of the pep8 gods is so that we don't argue about what is the right thing to do.

pep8 is the recommended way to format python code and it prevents bike-shedding over spaces\lines and other unimportant things. If pep8 accepts it than it's good enough. End of story.

doing except Exception is better as it doesn't catch InterruptError and other low level errors. On the other hand this doesn't really matter in VDSM as we have a handler for interrupts.

In any case I would much rather have this be a bit more reliable.
pep8 has a sort of a plugin system so you can use it to find bare excepts.

a quick grep shows that there are 173 cases of "except:"
Some are legitimate.

For instance in remoteFileHandler:PoolHandler.__init__

Also, I never understood why "except:" or "except Exception:" is that bad.

In c it's common practice to do:
if(some_opration() < 0) {
  log()
  goto cleanup
}

A lot of the time you don't handle specific errors differently and you just want to handle *any* failure. I'll wager that this is what you want *most* of the time.

The only way to actually prevent bare excepts or except Exception is to make errors part of the interface all throughout the system.

This is nearly impossible in python as the compiler will not check that you remembered to check for that new error all through the stack.

for example in:
remoteFileHandler:PoolHandler.stop
I don't want to care what exception os.kill might return.
All I care is that it failed and to ignore it. Why do I need to check if it's an OSError or IOError or whatever.

It could have been changed to except Exception for added verbosity and to ignore BaseException for whatever reason.
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Change in vdsm[master]: build: Forbid bare except:

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: build: Forbid bare except:
......................................................................

Patch Set 3:

(2 comments)

....................................................
File Makefile.am
Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^+\s+except:\s*(#.+)?$$'; then \
I don't have problem with "except Exception:" when you want to handle all exceptions. This is typical for any thread main loop.
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Line 78: SKIP_PYFLAKES_ERR = "./vdsm/storage/lvm.py.*: list comprehension redefines \
Line 79: 	'lv' from line .*"
Line 80: 
Line 81: check-local:
Line 82: 	if git show | egrep '^+\s+except:\s*(#.+)?$$'; then \
pyflakes may have this feature soon: https://github.com/pyflakes/pyflakes/pull/22, eliminating the need for this patch.

I'm afraid that we cannot fix the issue of checking other people code. Anything we do (git diff, git diff --cached, git show...) will fail in some way or another. For example, if you committed, git show is correct, if you did not commit, git diff is correct. If you added files to index, git diff is not correct. This is mess.

There is simple solution:
- Fix all 173 bare excepts (use except BaseException: as start, making this safe)
- Check all code like we do in current pyflakes/pep8

Later can can convert BaseException to something more appropriate.
Line 83: 		echo "error: empty "except:" is forbidden!"; \
Line 84: 		exit 1; \
Line 85: 	fi
Line 86: 	find . -path './.git' -prune -type f -o \

Change in vdsm[master]: build: Forbid bare except:

[Nir Soffer]

Nir Soffer has abandoned this change.

Change subject: build: Forbid bare except:
......................................................................

Abandoned

After discussion with the pyflakes folks, I think we should not use this. Mostly bare except is not what you want, but there are some valid case where bare except is correct and the cleanest way.