Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11735 )
Change subject: [build] Centralize project version to version.txt ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/11735/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11735/1//COMMIT_MSG@15 PS1, Line 15: to include updating the example project poms. Why bother? The examples will still work even if they're on a slightly older Java client, and we can upgrade it on an as-needed basis (i.e. when an example needs a new client feature). This is actually more safe if folks copy the examples verbatim to start their projects; it reduces the likelihood that someone will accidentally depend on a SNAPSHOT kudu-client. http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py File python/setup.py: http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@46 PS1, Line 46: r"^(?P<version>(?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+))(?:-(?P<label>.+))?", Please provide some examples of what this matches against in a comment. Also what's the point of naming all the subgroups if we only ever read version and label? Seems like it clutters the regex unnecessarily. http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@48 PS1, Line 48: re.M, re.MULTILINE is way more understandable. But why do we need it? version.txt only has one line in it. http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@52 PS1, Line 52: raise RuntimeError("Unable to parse version string.") Can you include the contents of version_file in this case? http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@54 PS1, Line 54: def write_version_py(filename=os.path.join(setup_dir, 'kudu/version.py')): What is version.py used for? It seems like it dumps a __version__ variable into the namespace of any program that 'imports kudu'; is this just informational? http://gerrit.cloudera.org:8080/#/c/11735/1/python/setup.py@189 PS1, Line 189: version=parse_version_and_label()[0], So it's not possible to get here without first calling write_version_py() on L163, which also parses version.txt. Could we avoid the double parsing? -- To view, visit http://gerrit.cloudera.org:8080/11735 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib654be8657ae3dc2f193056484b5d0743aa1ce95 Gerrit-Change-Number: 11735 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 16:37:13 +0000 Gerrit-HasComments: Yes
