Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/10932 )
Change subject: osmo-depcheck: script to verify PKG_CHECK_MODULES ...................................................................... Patch Set 2: Code-Review+1 (20 comments) It's a fact, my code review often borderlines on nitpicking. So take my comments lightly, please, especially when in braces, when they begin with 'maybe' and/or a question mark is involved. You can read all of this review if you like, but frankly, if these scripts do what they are supposed to, let's keep them like this. I did want to spend some time to review and am interested in your style, but let's avoid too much refactoring just because I couldn't shut up. In general: in some places I think the overall design could be simpler and much shorter, but the coding style is clear and mature, and I'm looking forward to more patches from you. So yes, in the end, let's see a run in jenkins as Harald said :) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py File scripts/osmo-depcheck/buildstack.py: https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@11 PS2, Line 11: Generally, this file reads like a re-invention of the GNU Make wheel... https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@13 PS2, Line 13: def next(depends, done): some of the function names here read like generic tools, I think more concise names would help readability: next() vs. next_buildable() generate() vs. gen_build_order() print_dict() vs. print_build_order() [EDIT] Later on, when I read these names in context of its python package, some naming makes more sense to me; but still 'next()' is too unspecific IMHO https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@20 PS2, Line 20: :param done: list of programs that would already have been built at it's actually an ordered dict, not a list https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@50 PS2, Line 50: :returns: a dictionary like the following: (an *ordered* dict) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@65 PS2, Line 65: """ Print the whole build stack. oh, so now it's a stack? .. an ordered dict stack? ;) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@80 PS2, Line 80: ret = tempfile.mkdtemp(prefix="depcheck") (maybe a separator [.-_] after 'depcheck'?) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@95 PS2, Line 95: for key, value in extend.items(): (would be more readable: for env_var, tempdir in extend.items() ) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@97 PS2, Line 97: os.environ[key] = old + ":" + value if 'old' is empty, you end up producing ":new_dir". A neat trick in these cases is join: paths = (os.environ.get(key), value) os.environ[key] = ':'.join(filter(bool, paths)) The ':' will be omitted when there is only one item. https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@134 PS2, Line 134: ["./configure", "--prefix", tempdir], hmm, do we need specific configure options sometimes? https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py File scripts/osmo-depcheck/dependencies.py: https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@14 PS2, Line 14: def clone(gitdir, repository, version): clone() vs. git_clone() https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@20 PS2, Line 20: # Clone when needed This function reads like a re-invention of the osmo-build-dep.sh wheel (osmo-ci/scripts/) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@38 PS2, Line 38: def generate(gitdir, initial): generate() vs. gen_versioned_dependencies() https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@76 PS2, Line 76: def print_dict(depends): vs. print_versioned_dependencies() https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/docker.py File scripts/osmo-depcheck/docker.py: https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/docker.py@15 PS2, Line 15: this file reads like it could be a few lines of shell script instead? like docker-playground/*/jenkins.sh https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py File scripts/osmo-depcheck/osmo-depcheck.py: https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py@59 PS2, Line 59: if args.docker and "INSIDE_DOCKER" not in os.environ: This script could just do what it does, does it really need to know that it is running in docker / start a docker itself? Ideally the caller is free to launch this in a docker, or not, as desired? https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py@74 PS2, Line 74: main() the (ugly) python paradigm would be if __name__ == '__main__': main() but whatever :P https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py File scripts/osmo-depcheck/parse.py: https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@18 PS2, Line 18: def repository(library, version): version has nothing to do with this function, does it? https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@58 PS2, Line 58: ret = line.split(",")[1].split(")")[0].strip() heh, unexpected split(')') https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@61 PS2, Line 61: library = ret.split(" ")[0] we probably always have these spaces, but wouldn't a regex that ignores spaces be more robust? Admitted, the regex is hardly readable. r = re.compile(r'PKG_CHECK_MODULES\([^,]*, *([a-z-_]*) *(>= *([0-9.]*)|) *\)') m = r.match('PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore >= 0.10.0)') m.group(1), m.group(3) ('libosmocore', '0.10.0') m2 = r.match('PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore)') m2.group(1), m2.group(3) ('libosmocore', None) https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@80 PS2, Line 80: version = split[2] py fu: library, operator, version = split -- To view, visit https://gerrit.osmocom.org/10932 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ci Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f495dbe030775f66ac125e60ded95c5d7660b65 Gerrit-Change-Number: 10932 Gerrit-PatchSet: 2 Gerrit-Owner: osmith <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Comment-Date: Mon, 17 Sep 2018 21:00:48 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
