diff mbox series

[ovs-dev,v3,13/18] python: detect changes in flow formatting code

Message ID 20220311152128.3988946-14-amorenoz@redhat.com
State Changes Requested
Headers show
Series python: add flow parsing library | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/intel-ovs-compilation success test: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Adrian Moreno March 11, 2022, 3:21 p.m. UTC
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              |   9 +++
 python/build/flow-parse-deps.py | 106 ++++++++++++++++++++++++++++++++
 python/ovs/flows/deps.py        |   5 ++
 4 files changed, 121 insertions(+)
 create mode 100755 python/build/flow-parse-deps.py
 create mode 100644 python/ovs/flows/deps.py

Comments

Eelco Chaudron March 18, 2022, 10:28 a.m. UTC | #1
On 11 Mar 2022, at 16:21, 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>
> ---

Changes look good to me!

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Ilya Maximets June 3, 2022, 1:44 p.m. UTC | #2
On 3/18/22 11:28, Eelco Chaudron wrote:
> 
> 
> On 11 Mar 2022, at 16:21, 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.

Ugh.  Trying to work with this change applied is very annoying. :)
I mean, odp-util.c is getting changed frequently, and you have to
update the hash every time you change it during development, otherwise
the build fails.  I'd like to not have this patch.  The previous
two should cover most of the cases, assuming people are adding tests
for every new match/action they are adding.  And they really should.

Best regards, Ilya Maximets.

>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
> 
> Changes look good to me!
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
Adrian Moreno June 14, 2022, 6:50 a.m. UTC | #3
On 6/3/22 15:44, Ilya Maximets wrote:
> On 3/18/22 11:28, Eelco Chaudron wrote:
>>
>>
>> On 11 Mar 2022, at 16:21, 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.
> 
> Ugh.  Trying to work with this change applied is very annoying. :)
> I mean, odp-util.c is getting changed frequently, and you have to
> update the hash every time you change it during development, otherwise
> the build fails.  I'd like to not have this patch.  The previous
> two should cover most of the cases, assuming people are adding tests
> for every new match/action they are adding.  And they really should.
> 

OK. I'll drop it in the next version of the series.

> Best regards, Ilya Maximets.
> 
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>
>> Changes look good to me!
>>
>> Acked-by: Eelco Chaudron <echaudro@redhat.com>
>
diff mbox series

Patch

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 9bd7a9e2f..e5501c58e 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -28,6 +28,7 @@  ovs_pyfiles = \
 	python/ovs/fcntl_win.py \
 	python/ovs/flows/__init__.py \
 	python/ovs/flows/decoders.py \
+	python/ovs/flows/deps.py \
 	python/ovs/flows/filter.py \
 	python/ovs/flows/flow.py \
 	python/ovs/flows/kv.py \
@@ -57,6 +58,7 @@  ovs_pyfiles = \
 EXTRA_DIST += \
 	python/build/__init__.py \
 	python/build/extract_ofp_fields.py \
+	python/build/flow-parse-deps.py \
 	python/build/nroff.py \
 	python/build/soutil.py
 
@@ -77,6 +79,7 @@  FLAKE8_PYFILES += \
 	$(filter-out python/ovs/compat/% python/ovs/dirs.py,$(PYFILES)) \
 	python/build/__init__.py \
 	python/build/extract_ofp_fields.py \
+	python/build/flow-parse-deps.py \
 	python/build/nroff.py \
 	python/build/soutil.py \
 	python/ovs/dirs.py.template \
@@ -137,3 +140,9 @@  $(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)
+	$(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..848ef6bac
--- /dev/null
+++ b/python/build/flow-parse-deps.py
@@ -0,0 +1,106 @@ 
+#!/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
+import os
+
+DEPENDENCIES = ["lib/ofp-actions.c", "lib/odp-util.c"]
+DEPENDENCY_FILE = "python/ovs/flows/deps.py"
+SRC_DIR = os.path.join(os.path.dirname(__file__), "..", "..")
+
+
+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(os.path.join(SRC_DIR, 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(os.path.join(SRC_DIR, DEPENDENCY_FILE), "w") as f:
+            print(depends, file=f)
+
+    elif sys.argv[1] == "check":
+        sys.path.append(os.path.join(SRC_DIR, "python"))
+        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.
+If it has changed, modify the python flow parsing code accordingly.
+
+Once you're done, update the dependencies by running '{cmd} update'.
+After doing so, 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..9a75475b5
--- /dev/null
+++ b/python/ovs/flows/deps.py
@@ -0,0 +1,5 @@ 
+# File automatically generated. Do not modify manually!
+dependencies = {
+    "lib/ofp-actions.c": "c839d7d34a6e9ab1dcb1c0961211d3a6",
+    "lib/odp-util.c": "8fb7c5fa46ceb7e887c44ed4fcc80ee5"
+}