<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">So yes, in the end, let's see a run in jenkins as Harald said :)</p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><p><a href="https://gerrit.osmocom.org/10932">View Change</a></p><p>20 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py">File scripts/osmo-depcheck/buildstack.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@11">Patch Set #2, Line 11:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Generally, this file reads like a re-invention of the GNU Make wheel...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@13">Patch Set #2, Line 13:</a> <code style="font-family:monospace,monospace">def next(depends, done):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">some of the function names here read like generic tools, I think more concise names would help readability:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  next() vs. next_buildable()<br>  generate() vs. gen_build_order()<br>  print_dict() vs. print_build_order()</pre><p style="white-space: pre-wrap; word-wrap: break-word;">[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</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@20">Patch Set #2, Line 20:</a> <code style="font-family:monospace,monospace">        :param done: list of programs that would already have been built at</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">it's actually an ordered dict, not a list</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@50">Patch Set #2, Line 50:</a> <code style="font-family:monospace,monospace">        :returns: a dictionary like the following:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(an *ordered* dict)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@65">Patch Set #2, Line 65:</a> <code style="font-family:monospace,monospace">    """ Print the whole build stack.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">oh, so now it's a stack? .. an ordered dict stack? ;)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@80">Patch Set #2, Line 80:</a> <code style="font-family:monospace,monospace">    ret = tempfile.mkdtemp(prefix="depcheck")</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(maybe a separator [.-_] after 'depcheck'?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@95">Patch Set #2, Line 95:</a> <code style="font-family:monospace,monospace">    for key, value in extend.items():</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(would be more readable:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  for env_var, tempdir in extend.items()</pre><p style="white-space: pre-wrap; word-wrap: break-word;">)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@97">Patch Set #2, Line 97:</a> <code style="font-family:monospace,monospace">        os.environ[key] = old + ":" + value</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if 'old' is empty, you end up producing ":new_dir". A neat trick in these cases is join:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  paths = (os.environ.get(key), value)<br>  os.environ[key] = ':'.join(filter(bool, paths))</pre><p style="white-space: pre-wrap; word-wrap: break-word;">The ':' will be omitted when there is only one item.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@134">Patch Set #2, Line 134:</a> <code style="font-family:monospace,monospace">                    ["./configure", "--prefix", tempdir],</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">hmm, do we need specific configure options sometimes?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py">File scripts/osmo-depcheck/dependencies.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@14">Patch Set #2, Line 14:</a> <code style="font-family:monospace,monospace">def clone(gitdir, repository, version):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">clone() vs. git_clone()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@20">Patch Set #2, Line 20:</a> <code style="font-family:monospace,monospace">    # Clone when needed</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This function reads like a re-invention of the osmo-build-dep.sh wheel (osmo-ci/scripts/)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@38">Patch Set #2, Line 38:</a> <code style="font-family:monospace,monospace">def generate(gitdir, initial):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">generate() vs. gen_versioned_dependencies()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/dependencies.py@76">Patch Set #2, Line 76:</a> <code style="font-family:monospace,monospace">def print_dict(depends):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">vs. print_versioned_dependencies()</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/docker.py">File scripts/osmo-depcheck/docker.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/docker.py@15">Patch Set #2, Line 15:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this file reads like it could be a few lines of shell script instead? like docker-playground/*/jenkins.sh</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py">File scripts/osmo-depcheck/osmo-depcheck.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py@59">Patch Set #2, Line 59:</a> <code style="font-family:monospace,monospace">    if args.docker and "INSIDE_DOCKER" not in os.environ:</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py@74">Patch Set #2, Line 74:</a> <code style="font-family:monospace,monospace">main()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">the (ugly) python paradigm would be</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  if __name__ == '__main__':<br>    main()</pre><p style="white-space: pre-wrap; word-wrap: break-word;">but whatever :P</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py">File scripts/osmo-depcheck/parse.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@18">Patch Set #2, Line 18:</a> <code style="font-family:monospace,monospace">def repository(library, version):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">version has nothing to do with this function, does it?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@58">Patch Set #2, Line 58:</a> <code style="font-family:monospace,monospace">    ret = line.split(",")[1].split(")")[0].strip()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">heh, unexpected split(')')</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@61">Patch Set #2, Line 61:</a> <code style="font-family:monospace,monospace">    library = ret.split(" ")[0]</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">we probably always have these spaces, but wouldn't a regex that ignores spaces be more robust?<br>Admitted, the regex is hardly readable.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  r = re.compile(r'PKG_CHECK_MODULES\([^,]*, *([a-z-_]*) *(>= *([0-9.]*)|) *\)')</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  m = r.match('PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore  >= 0.10.0)')<br>  m.group(1), m.group(3)<br>    ('libosmocore', '0.10.0')</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  m2 = r.match('PKG_CHECK_MODULES(LIBOSMOCORE, libosmocore)')<br>  m2.group(1), m2.group(3)<br>    ('libosmocore', None)</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@80">Patch Set #2, Line 80:</a> <code style="font-family:monospace,monospace">    version = split[2]</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">py fu:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  library, operator, version = split</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/10932">change 10932</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/10932"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-ci </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I8f495dbe030775f66ac125e60ded95c5d7660b65 </div>
<div style="display:none"> Gerrit-Change-Number: 10932 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 17 Sep 2018 21:00:48 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>