<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><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></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Thank you for the detailed review!</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><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></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">https://jenkins.osmocom.org/jenkins/job/Osmocom-depcheck (check out the new configurable build parameters!)</p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Verified +1</span></p><p><a href="https://gerrit.osmocom.org/10932">View Change</a></p><p>19 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/.gitignore">File .gitignore:</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/.gitignore@8">Patch Set #2, Line 8:</a> <code style="font-family:monospace,monospace">__pycache__/</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">technically this would be a separate patch from the rest</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Submitted and merged here:<br>https://gerrit.osmocom.org/#/c/osmo-ci/+/11032/</p></li></ul></li><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@2">Patch Set #2, Line 2:</a> <code style="font-family:monospace,monospace"># Copyright 2018 sysmocom - s.f.m.c. GmbH <info@sysmocom.de></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(You should write "Copyright <year> ...", the (c) is just a gimmick IIUC. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">fixed in the upcoming patch</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@11">Patch Set #2, Line 11:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Generally, this file reads like a re-invention of the GNU Make wheel...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">make? never heard of that :p</p><p style="white-space: pre-wrap; word-wrap: break-word;">But seriously: now that you say it, I could have probably replaced "next()", "generate()", "print_dict()", set_environment()" and "build()" with writing a Makefile, and then let make do the resolving and execute the commands.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I am not sure if it would have been possible to generate a nice overview of the build order as written below (but then again, this was never asked for).</p><p style="white-space: pre-wrap; word-wrap: break-word;">So I'll consider this in the future, if there's a similar situation. Thanks for the hint!</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">---<br>Build order:<br> * libosmocore:0.11.0<br> * libosmo-abis:0.5.0<br> * osmo-hlr:0.2.1</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/buildstack.py@13">Patch Set #2, Line 13:</a> <code style="font-family:monospace,monospace">def next_buildable(depends, done):</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">some of the function names here read like generic tools, I think more concise names would help reada […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Replaced "next()" with "next_buildable()" in the next patchset.</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: ordered dict of programs that would already have been</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">it's actually an ordered dict, not a list</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">good catch! fixed in the upcoming patchset</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">        :param depends: return value of dependencies.generate()</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(an *ordered* dict)</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">+1</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">def print_dict(stack):</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">oh, so now it's a stack? .. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Well, the file is called "buildstack.py", so that ordered dict *is* the stack. There's no stack datatype in Python 3 from what I found, so I don't get why this is confusing :p</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">        :returns: the path to the temporary folder """</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(maybe a separator [. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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">              "LD_LIBRARY_PATH": tempdir + "/lib"}</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">(would be more readable: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'll change this to env_var, folder (so I don't overwrite "tempdir" which is already used above).</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">        old = os.environ[env_var] if env_var in os.environ else ""</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">if 'old' is empty, you end up producing ":new_dir". A neat trick in these cases is join: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Nice trick, but that seems to be a bit over-engineered here IMHO. ":path" will work just as well.</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">        # Run the build commands</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">hmm, do we need specific configure options sometimes?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">--with-systemdsystemunitdir was also needed, besides that it worked for all programs I have tested without any additional parameters. On demand we could still add a mapping in config.py for repositories and the special configure parameters they need.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/config.py">File scripts/osmo-depcheck/config.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/config.py@10">Patch Set #2, Line 10:</a> <code style="font-family:monospace,monospace">            "osmo-hlr",</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">indicate whether it's regex or shell glob or ...</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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/config.py@18">Patch Set #2, Line 18:</a> <code style="font-family:monospace,monospace"># they are mentioned with PKG_CHECK_MODULES in configure.ac.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">would make sense to use tuples instead of lists -- they aren't supposed to be mutable, right? […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Good point with the tuples.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Regarding the lists, I built them that way so they follow PEP8 (so I can run "flake8" on it without disabling anything). If you say it's not that important, then I'll keep it that way for now.</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">    """ Clone a missing git repository and checkout a specific version tag.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">clone() vs. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This function reads like a re-invention of the osmo-build-dep. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Well, this is a pretty short function (14 LOC), and it does have a few key differences:</p><ul><li>explain that config.py needs to be adjusted when git clone fails</li><li>silent output unless an error happens (and this is kind of important here, otherwise the resulting output of the dependency list would not be readable at all)</li><li>(in the upcoming patchset) allow a different git url prefix, as requested by @pespin</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Also I saw osmo-build-dep.sh and osmo-deps.sh and briefly looked at them, but they did not have a description on top of them indicating that they could be used elsewhere. I thought that they were part of some internal code somewhere. That might be a good improvement?</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">    ret = parser.parse_args()</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This script could just do what it does, does it really need to know that it is running in docker / s […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">The idea here was to make it convenient to run everything in docker by adding "-d" to the command line. But it turned out, that there's no advantage of running this in docker anyway (see the redmine issue), so I've removed all docker related code in the next patchset.</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">            project, rev = project_rev.split(":", 1)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">the (ugly) python paradigm would be […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">version has nothing to do with this function, does it?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is needed for printing purposes (see below).</p><p style="white-space: pre-wrap; word-wrap: break-word;">Mixing the printing part and the part which actually retrieves the repository is a bit unclean, I know. But if we would split that up in two functions (one extra function for printing), we would probably have the double amount of LOC here. This wouldn't be a better tradeoff in my opinion. My take on this would be refactoring it if it grows bigger.</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@80">Patch Set #2, Line 80:</a> <code style="font-family:monospace,monospace">        return (library, version)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">py fu: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">nice, thanks! :D</p></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: 3 </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-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 20 Sep 2018 12:55:39 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>