osmith has posted comments on this change. ( https://gerrit.osmocom.org/10932 )
Change subject: osmo-depcheck: script to verify PKG_CHECK_MODULES ...................................................................... Patch Set 3: Verified+1 (19 comments) > 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. Thank you for the detailed review! > So yes, in the end, let's see a run in jenkins as Harald said :) https://jenkins.osmocom.org/jenkins/job/Osmocom-depcheck (check out the new configurable build parameters!) https://gerrit.osmocom.org/#/c/10932/2/.gitignore File .gitignore: https://gerrit.osmocom.org/#/c/10932/2/.gitignore@8 PS2, Line 8: __pycache__/ > technically this would be a separate patch from the rest Submitted and merged here: https://gerrit.osmocom.org/#/c/osmo-ci/+/11032/ 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@2 PS2, Line 2: # Copyright 2018 sysmocom - s.f.m.c. GmbH <[email protected]> > (You should write "Copyright <year> ...", the (c) is just a gimmick IIUC. […] fixed in the upcoming patch 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... make? never heard of that :p 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. 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). So I'll consider this in the future, if there's a similar situation. Thanks for the hint! --- Build order: * libosmocore:0.11.0 * libosmo-abis:0.5.0 * osmo-hlr:0.2.1 https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@13 PS2, Line 13: def next_buildable(depends, done): > some of the function names here read like generic tools, I think more concise > names would help reada […] Replaced "next()" with "next_buildable()" in the next patchset. https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@20 PS2, Line 20: :param done: ordered dict of programs that would already have been > it's actually an ordered dict, not a list good catch! fixed in the upcoming patchset https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@50 PS2, Line 50: :param depends: return value of dependencies.generate() > (an *ordered* dict) +1 https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@65 PS2, Line 65: def print_dict(stack): > oh, so now it's a stack? .. […] 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 https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@80 PS2, Line 80: :returns: the path to the temporary folder """ > (maybe a separator [. […] Ack https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@95 PS2, Line 95: "LD_LIBRARY_PATH": tempdir + "/lib"} > (would be more readable: […] I'll change this to env_var, folder (so I don't overwrite "tempdir" which is already used above). https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@97 PS2, Line 97: old = os.environ[env_var] if env_var in os.environ else "" > if 'old' is empty, you end up producing ":new_dir". A neat trick in these > cases is join: […] Nice trick, but that seems to be a bit over-engineered here IMHO. ":path" will work just as well. https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/buildstack.py@134 PS2, Line 134: # Run the build commands > hmm, do we need specific configure options sometimes? --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. https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/config.py File scripts/osmo-depcheck/config.py: https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/config.py@10 PS2, Line 10: "osmo-hlr", > indicate whether it's regex or shell glob or ... Ack https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/config.py@18 PS2, Line 18: # they are mentioned with PKG_CHECK_MODULES in configure.ac. > would make sense to use tuples instead of lists -- they aren't supposed to be > mutable, right? […] Good point with the tuples. 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. 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: """ Clone a missing git repository and checkout a specific version tag. > clone() vs. […] Ack 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. […] Well, this is a pretty short function (14 LOC), and it does have a few key differences: * explain that config.py needs to be adjusted when git clone fails * 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) * (in the upcoming patchset) allow a different git url prefix, as requested by @pespin 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? 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: ret = parser.parse_args() > This script could just do what it does, does it really need to know that it > is running in docker / s […] 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. https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/osmo-depcheck.py@74 PS2, Line 74: project, rev = project_rev.split(":", 1) > the (ugly) python paradigm would be […] Ack 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? This is needed for printing purposes (see below). 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. https://gerrit.osmocom.org/#/c/10932/2/scripts/osmo-depcheck/parse.py@80 PS2, Line 80: return (library, version) > py fu: […] nice, thanks! :D -- 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: 3 Gerrit-Owner: osmith <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-Comment-Date: Thu, 20 Sep 2018 12:55:39 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
