| Message ID | 20211122112256.2011194-14-amorenoz@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | python: add flow parsing library | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 22 Nov 2021, at 12:22, Adrian Moreno wrote: > In order to minimize the risk of having the python flow parsing code and > the C flow formatting code divert, add a target that checks if the > formatting code has been changed since the last revision and warn the > developer if it has. > > The script also makes it easy to update the dependency file so hopefully > it will not cause too much trouble for a developer that has modifed the > file without changing the flow string format. > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > .gitignore | 1 + > python/automake.mk | 13 +++- > python/build/flow-parse-deps.py | 101 ++++++++++++++++++++++++++++++++ > python/ovs/flows/deps.py | 5 ++ > 4 files changed, 118 insertions(+), 2 deletions(-) > create mode 100755 python/build/flow-parse-deps.py > create mode 100644 python/ovs/flows/deps.py > > diff --git a/.gitignore b/.gitignore > index f1cdcf124..e6bca1cd2 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -79,3 +79,4 @@ testsuite.tmp.orig > /Documentation/_build > /.venv > /cxx-check > +/flowparse-deps-check > diff --git a/python/automake.mk b/python/automake.mk > index 21aa897f2..da6ef08b4 100644 > --- a/python/automake.mk > +++ b/python/automake.mk > @@ -50,7 +50,8 @@ ovs_pyfiles = \ > python/ovs/flows/ofp.py \ > python/ovs/flows/ofp_act.py \ > python/ovs/flows/odp.py \ > - python/ovs/flows/filter.py > + python/ovs/flows/filter.py \ > + python/ovs/flows/deps.py Add in alphabetical order > > # These python files are used at build time but not runtime, > # so they are not installed. > @@ -58,7 +59,8 @@ EXTRA_DIST += \ > python/build/__init__.py \ > python/build/nroff.py \ > python/build/soutil.py \ > - python/build/extract_ofp_fields.py > + python/build/extract_ofp_fields.py \ > + python/build/flow-parse-deps.py > Add in alphabetical order, and also add it to FLAKE8_PYFILES. > # PyPI support. > EXTRA_DIST += \ > @@ -135,3 +137,10 @@ $(srcdir)/python/ovs/flows/ofp_fields.py: $(srcdir)/build-aux/gen_ofp_field_deco > EXTRA_DIST += python/ovs/flows/ofp_fields.py > CLEANFILES += python/ovs/flows/ofp_fields.py > > +ALL_LOCAL += flowparse-deps-check > +DEPS = $(shell $(AM_V_GEN)$(run_python) $(srcdir)/python/build/flow-parse-deps.py list) > +flowparse-deps-check: $(srcdir)/python/build/flow-parse-deps.py $(DEPS) > + echo $(DEPS) > + $(AM_V_GEN)$(run_python) $(srcdir)/python/build/flow-parse-deps.py check > + touch $@ > +CLEANFILES += flowparse-deps-check > diff --git a/python/build/flow-parse-deps.py b/python/build/flow-parse-deps.py > new file mode 100755 > index 000000000..9adb1d89c > --- /dev/null > +++ b/python/build/flow-parse-deps.py > @@ -0,0 +1,101 @@ > +#!/usr/bin/env python3 > +# Copyright (c) 2021 Red Hat, Inc. > +# > +# Licensed under the Apache License, Version 2.0 (the "License"); > +# you may not use this file except in compliance with the License. > +# You may obtain a copy of the License at: > +# > +# http://www.apache.org/licenses/LICENSE-2.0 > +# > +# Unless required by applicable law or agreed to in writing, software > +# distributed under the License is distributed on an "AS IS" BASIS, > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > +# See the License for the specific language governing permissions and > +# limitations under the License. > + > +# Breaks lines read from stdin into groups using blank lines as > +# group separators, then sorts lines within the groups for > +# reproducibility. > + > + > +# ovs-test-ofparse is just a wrapper around ovs-ofctl > +# that also runs the python flow parsing utility to check that flows are > +# parseable > + > +import hashlib > +import sys > + > +DEPENDENCIES = ["lib/ofp-actions.c", "lib/odp-util.c"] > +DEPENDENCY_FILE = "python/ovs/flows/deps.py" > + > + > +def usage(): > + print( > + """ > +Usage {cmd} [check | update | list] > +Tool to verify flow parsing python code is kept in sync with > +flow printing C code. > + > +Commands: > + check: check the dependencies are met > + update: update the dependencies based on current file content > + list: list the dependency files Nit, as my OCD kicked in. Can you align (if not, that’s fine, I can try to suppress my OCD ;) check: check the dependencies are met update: update the dependencies based on current file content list: list the dependency files > +""".format( > + cmd=sys.argv[0] > + ) > + ) > + > + > +def digest(filename): > + with open(filename, "rb") as f: > + return hashlib.md5(f.read()).hexdigest() > + > + > +def main(): > + if len(sys.argv) != 2: > + usage() > + sys.exit(1) > + > + if sys.argv[1] == "list": > + print(" ".join(DEPENDENCIES)) > + elif sys.argv[1] == "update": > + dep_str = list() > + for dep in DEPENDENCIES: > + dep_str.append( > + ' "{dep}": "{digest}"'.format(dep=dep, digest=digest(dep)) > + ) > + > + depends = """# File automatically generated. Do not modify manually depends = """# File automatically generated. Do not modify manually! > +dependencies = {{ > +{dependencies_dict} > +}}""".format( > + dependencies_dict=",\n".join(dep_str) > + ) > + with open(DEPENDENCY_FILE, "w") as f: > + print(depends, file=f) > + > + elif sys.argv[1] == "check": > + from ovs.flows.deps import dependencies > + > + for dep in DEPENDENCIES: > + expected = dependencies.get(dep) > + if not expected or expected != digest(dep): > + print( > + """ > +Dependency file {dep} has changed. > +Please verify the flow output format has not changed or modify the python flow parsing code accordingly. Please verify the flow output format has not changed. If it has changed, modify the python flow parsing code accordingly. > + > +Once you're done, update the dependencies by running '{cmd} update' and check in the new dependency file. Please fix line length here to <= 79, so flake8 is not complaining. > +""".format( > + dep=dep, > + cmd=sys.argv[0], > + ) > + ) > + return 2 > + else: > + usage() > + sys.exit(1) > + > + > +if __name__ == "__main__": > + sys.exit(main()) > diff --git a/python/ovs/flows/deps.py b/python/ovs/flows/deps.py > new file mode 100644 > index 000000000..aaf428917 > --- /dev/null > +++ b/python/ovs/flows/deps.py > @@ -0,0 +1,5 @@ > +# File automatically generated. Do not modify manually > +dependencies = { > + "lib/ofp-actions.c": "f108b3e119f03b3373064589aecdeaf0", > + "lib/odp-util.c": "c946c21d8f644869ce778842167f9129" > +} > -- Running check from from the root does not work, maybe we can fix the ovs path to make it work from here? $ ./python/build/flow-parse-deps.py check Traceback (most recent call last): File "/home/echaudron/Documents/review/ovs_amorenoz_python/./python/build/flow-parse-deps.py", line 101, in <module> sys.exit(main()) File "/home/echaudron/Documents/review/ovs_amorenoz_python/./python/build/flow-parse-deps.py", line 78, in main from ovs.flows.deps import dependencies ModuleNotFoundError: No module named 'ovs'
On 12/23/21 17:06, Eelco Chaudron wrote: > > > On 22 Nov 2021, at 12:22, Adrian Moreno wrote: > >> In order to minimize the risk of having the python flow parsing code and >> the C flow formatting code divert, add a target that checks if the >> formatting code has been changed since the last revision and warn the >> developer if it has. >> >> The script also makes it easy to update the dependency file so hopefully >> it will not cause too much trouble for a developer that has modifed the >> file without changing the flow string format. >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> .gitignore | 1 + >> python/automake.mk | 13 +++- >> python/build/flow-parse-deps.py | 101 ++++++++++++++++++++++++++++++++ >> python/ovs/flows/deps.py | 5 ++ >> 4 files changed, 118 insertions(+), 2 deletions(-) >> create mode 100755 python/build/flow-parse-deps.py >> create mode 100644 python/ovs/flows/deps.py >> >> diff --git a/.gitignore b/.gitignore >> index f1cdcf124..e6bca1cd2 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -79,3 +79,4 @@ testsuite.tmp.orig >> /Documentation/_build >> /.venv >> /cxx-check >> +/flowparse-deps-check >> diff --git a/python/automake.mk b/python/automake.mk >> index 21aa897f2..da6ef08b4 100644 >> --- a/python/automake.mk >> +++ b/python/automake.mk >> @@ -50,7 +50,8 @@ ovs_pyfiles = \ >> python/ovs/flows/ofp.py \ >> python/ovs/flows/ofp_act.py \ >> python/ovs/flows/odp.py \ >> - python/ovs/flows/filter.py >> + python/ovs/flows/filter.py \ >> + python/ovs/flows/deps.py > > Add in alphabetical order > >> >> # These python files are used at build time but not runtime, >> # so they are not installed. >> @@ -58,7 +59,8 @@ EXTRA_DIST += \ >> python/build/__init__.py \ >> python/build/nroff.py \ >> python/build/soutil.py \ >> - python/build/extract_ofp_fields.py >> + python/build/extract_ofp_fields.py \ >> + python/build/flow-parse-deps.py >> > > Add in alphabetical order, and also add it to FLAKE8_PYFILES. > >> # PyPI support. >> EXTRA_DIST += \ >> @@ -135,3 +137,10 @@ $(srcdir)/python/ovs/flows/ofp_fields.py: $(srcdir)/build-aux/gen_ofp_field_deco >> EXTRA_DIST += python/ovs/flows/ofp_fields.py >> CLEANFILES += python/ovs/flows/ofp_fields.py >> >> +ALL_LOCAL += flowparse-deps-check >> +DEPS = $(shell $(AM_V_GEN)$(run_python) $(srcdir)/python/build/flow-parse-deps.py list) >> +flowparse-deps-check: $(srcdir)/python/build/flow-parse-deps.py $(DEPS) >> + echo $(DEPS) >> + $(AM_V_GEN)$(run_python) $(srcdir)/python/build/flow-parse-deps.py check >> + touch $@ >> +CLEANFILES += flowparse-deps-check >> diff --git a/python/build/flow-parse-deps.py b/python/build/flow-parse-deps.py >> new file mode 100755 >> index 000000000..9adb1d89c >> --- /dev/null >> +++ b/python/build/flow-parse-deps.py >> @@ -0,0 +1,101 @@ >> +#!/usr/bin/env python3 >> +# Copyright (c) 2021 Red Hat, Inc. >> +# >> +# Licensed under the Apache License, Version 2.0 (the "License"); >> +# you may not use this file except in compliance with the License. >> +# You may obtain a copy of the License at: >> +# >> +# http://www.apache.org/licenses/LICENSE-2.0 >> +# >> +# Unless required by applicable law or agreed to in writing, software >> +# distributed under the License is distributed on an "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. >> +# See the License for the specific language governing permissions and >> +# limitations under the License. >> + >> +# Breaks lines read from stdin into groups using blank lines as >> +# group separators, then sorts lines within the groups for >> +# reproducibility. >> + >> + >> +# ovs-test-ofparse is just a wrapper around ovs-ofctl >> +# that also runs the python flow parsing utility to check that flows are >> +# parseable >> + >> +import hashlib >> +import sys >> + >> +DEPENDENCIES = ["lib/ofp-actions.c", "lib/odp-util.c"] >> +DEPENDENCY_FILE = "python/ovs/flows/deps.py" >> + >> + >> +def usage(): >> + print( >> + """ >> +Usage {cmd} [check | update | list] >> +Tool to verify flow parsing python code is kept in sync with >> +flow printing C code. >> + >> +Commands: >> + check: check the dependencies are met >> + update: update the dependencies based on current file content >> + list: list the dependency files > > Nit, as my OCD kicked in. Can you align (if not, that’s fine, I can try to suppress my OCD ;) > > > check: check the dependencies are met > update: update the dependencies based on current file content > list: list the dependency files > Sure! >> +""".format( >> + cmd=sys.argv[0] >> + ) >> + ) >> + >> + >> +def digest(filename): >> + with open(filename, "rb") as f: >> + return hashlib.md5(f.read()).hexdigest() >> + >> + >> +def main(): >> + if len(sys.argv) != 2: >> + usage() >> + sys.exit(1) >> + >> + if sys.argv[1] == "list": >> + print(" ".join(DEPENDENCIES)) >> + elif sys.argv[1] == "update": >> + dep_str = list() >> + for dep in DEPENDENCIES: >> + dep_str.append( >> + ' "{dep}": "{digest}"'.format(dep=dep, digest=digest(dep)) >> + ) >> + >> + depends = """# File automatically generated. Do not modify manually > > depends = """# File automatically generated. Do not modify manually! > >> +dependencies = {{ >> +{dependencies_dict} >> +}}""".format( >> + dependencies_dict=",\n".join(dep_str) >> + ) >> + with open(DEPENDENCY_FILE, "w") as f: >> + print(depends, file=f) >> + >> + elif sys.argv[1] == "check": >> + from ovs.flows.deps import dependencies >> + >> + for dep in DEPENDENCIES: >> + expected = dependencies.get(dep) >> + if not expected or expected != digest(dep): >> + print( >> + """ >> +Dependency file {dep} has changed. >> +Please verify the flow output format has not changed or modify the python flow parsing code accordingly. > > Please verify the flow output format has not changed. > If it has changed, modify the python flow parsing code accordingly. > >> + >> +Once you're done, update the dependencies by running '{cmd} update' and check in the new dependency file. > > Please fix line length here to <= 79, so flake8 is not complaining. > >> +""".format( >> + dep=dep, >> + cmd=sys.argv[0], >> + ) >> + ) >> + return 2 >> + else: >> + usage() >> + sys.exit(1) >> + >> + >> +if __name__ == "__main__": >> + sys.exit(main()) >> diff --git a/python/ovs/flows/deps.py b/python/ovs/flows/deps.py >> new file mode 100644 >> index 000000000..aaf428917 >> --- /dev/null >> +++ b/python/ovs/flows/deps.py >> @@ -0,0 +1,5 @@ >> +# File automatically generated. Do not modify manually >> +dependencies = { >> + "lib/ofp-actions.c": "f108b3e119f03b3373064589aecdeaf0", >> + "lib/odp-util.c": "c946c21d8f644869ce778842167f9129" >> +} >> -- > > Running check from from the root does not work, maybe we can fix the ovs path to make it work from here? > > $ ./python/build/flow-parse-deps.py check > Traceback (most recent call last): > File "/home/echaudron/Documents/review/ovs_amorenoz_python/./python/build/flow-parse-deps.py", line 101, in <module> > sys.exit(main()) > File "/home/echaudron/Documents/review/ovs_amorenoz_python/./python/build/flow-parse-deps.py", line 78, in main > from ovs.flows.deps import dependencies > ModuleNotFoundError: No module named 'ovs' > Sure, will do.
diff --git a/.gitignore b/.gitignore index f1cdcf124..e6bca1cd2 100644 --- a/.gitignore +++ b/.gitignore @@ -79,3 +79,4 @@ testsuite.tmp.orig /Documentation/_build /.venv /cxx-check +/flowparse-deps-check diff --git a/python/automake.mk b/python/automake.mk index 21aa897f2..da6ef08b4 100644 --- a/python/automake.mk +++ b/python/automake.mk @@ -50,7 +50,8 @@ ovs_pyfiles = \ python/ovs/flows/ofp.py \ python/ovs/flows/ofp_act.py \ python/ovs/flows/odp.py \ - python/ovs/flows/filter.py + python/ovs/flows/filter.py \ + python/ovs/flows/deps.py # These python files are used at build time but not runtime, # so they are not installed. @@ -58,7 +59,8 @@ EXTRA_DIST += \ python/build/__init__.py \ python/build/nroff.py \ python/build/soutil.py \ - python/build/extract_ofp_fields.py + python/build/extract_ofp_fields.py \ + python/build/flow-parse-deps.py # PyPI support. EXTRA_DIST += \ @@ -135,3 +137,10 @@ $(srcdir)/python/ovs/flows/ofp_fields.py: $(srcdir)/build-aux/gen_ofp_field_deco EXTRA_DIST += python/ovs/flows/ofp_fields.py CLEANFILES += python/ovs/flows/ofp_fields.py +ALL_LOCAL += flowparse-deps-check +DEPS = $(shell $(AM_V_GEN)$(run_python) $(srcdir)/python/build/flow-parse-deps.py list) +flowparse-deps-check: $(srcdir)/python/build/flow-parse-deps.py $(DEPS) + echo $(DEPS) + $(AM_V_GEN)$(run_python) $(srcdir)/python/build/flow-parse-deps.py check + touch $@ +CLEANFILES += flowparse-deps-check diff --git a/python/build/flow-parse-deps.py b/python/build/flow-parse-deps.py new file mode 100755 index 000000000..9adb1d89c --- /dev/null +++ b/python/build/flow-parse-deps.py @@ -0,0 +1,101 @@ +#!/usr/bin/env python3 +# Copyright (c) 2021 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at: +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Breaks lines read from stdin into groups using blank lines as +# group separators, then sorts lines within the groups for +# reproducibility. + + +# ovs-test-ofparse is just a wrapper around ovs-ofctl +# that also runs the python flow parsing utility to check that flows are +# parseable + +import hashlib +import sys + +DEPENDENCIES = ["lib/ofp-actions.c", "lib/odp-util.c"] +DEPENDENCY_FILE = "python/ovs/flows/deps.py" + + +def usage(): + print( + """ +Usage {cmd} [check | update | list] +Tool to verify flow parsing python code is kept in sync with +flow printing C code. + +Commands: + check: check the dependencies are met + update: update the dependencies based on current file content + list: list the dependency files +""".format( + cmd=sys.argv[0] + ) + ) + + +def digest(filename): + with open(filename, "rb") as f: + return hashlib.md5(f.read()).hexdigest() + + +def main(): + if len(sys.argv) != 2: + usage() + sys.exit(1) + + if sys.argv[1] == "list": + print(" ".join(DEPENDENCIES)) + elif sys.argv[1] == "update": + dep_str = list() + for dep in DEPENDENCIES: + dep_str.append( + ' "{dep}": "{digest}"'.format(dep=dep, digest=digest(dep)) + ) + + depends = """# File automatically generated. Do not modify manually +dependencies = {{ +{dependencies_dict} +}}""".format( + dependencies_dict=",\n".join(dep_str) + ) + with open(DEPENDENCY_FILE, "w") as f: + print(depends, file=f) + + elif sys.argv[1] == "check": + from ovs.flows.deps import dependencies + + for dep in DEPENDENCIES: + expected = dependencies.get(dep) + if not expected or expected != digest(dep): + print( + """ +Dependency file {dep} has changed. +Please verify the flow output format has not changed or modify the python flow parsing code accordingly. + +Once you're done, update the dependencies by running '{cmd} update' and check in the new dependency file. +""".format( + dep=dep, + cmd=sys.argv[0], + ) + ) + return 2 + else: + usage() + sys.exit(1) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/python/ovs/flows/deps.py b/python/ovs/flows/deps.py new file mode 100644 index 000000000..aaf428917 --- /dev/null +++ b/python/ovs/flows/deps.py @@ -0,0 +1,5 @@ +# File automatically generated. Do not modify manually +dependencies = { + "lib/ofp-actions.c": "f108b3e119f03b3373064589aecdeaf0", + "lib/odp-util.c": "c946c21d8f644869ce778842167f9129" +}
In order to minimize the risk of having the python flow parsing code and the C flow formatting code divert, add a target that checks if the formatting code has been changed since the last revision and warn the developer if it has. The script also makes it easy to update the dependency file so hopefully it will not cause too much trouble for a developer that has modifed the file without changing the flow string format. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- .gitignore | 1 + python/automake.mk | 13 +++- python/build/flow-parse-deps.py | 101 ++++++++++++++++++++++++++++++++ python/ovs/flows/deps.py | 5 ++ 4 files changed, 118 insertions(+), 2 deletions(-) create mode 100755 python/build/flow-parse-deps.py create mode 100644 python/ovs/flows/deps.py