On 1/25/21 5:44 AM, Dumitru Ceara wrote:
On 1/22/21 10:21 PM, Mark Michelson wrote:
OVN developers have had isssues with the current method by which OVS
source code is used by OVN.
* There is no way to record the minimum commit/version of OVS to use
when compiling OVN.
* When debugging issues, bisecting OVN commits may also requires
simultaneously changing OVS commits. This makes for multiple moving
targets to try to track.
* Performance improvements made to OVS libraries and OVSDB may benefit
OVN. However, there's no way to encourage the use of the improved OVS
source.
By using a submodule, it allows for OVN to record a specific commit of
OVS that is expected to be used.
Signed-off-by: Mark Michelson <[email protected]>
---
v1 -> v2:
* Updated CI scripts to use the submodule.
* Added AM_DISTCHECK_CONFIGURE_FLAGS setting so `make distcheck`
succeeds.
* Added an extra 'xargs' call to submodule discovery in Makefile.am
since for some reason on OSX, there was extra whitespace around the
discovered "ovs" submodule. xargs is a simple way to strip that away.
* Updated documentation to more accurately reflect the necessary steps
to use the OVS submodule. Also changed wording to indicate the
submodule is the minimum recommended version, rather than the minimum
required version of OVS.
---
Hi Mark,
Overall this revision looks OK to me and it passes github CI. A few
comments inline though.
.ci/linux-build.sh | 8 +++---
.ci/osx-build.sh | 8 +++---
.gitmodules | 3 +++
Documentation/intro/install/general.rst | 36 +++++++++++++++++++------
Makefile.am | 23 +++++++++-------
acinclude.m4 | 2 +-
build-aux/initial-tab-whitelist | 1 +
ovs | 1 +
8 files changed, 55 insertions(+), 27 deletions(-)
create mode 100644 .gitmodules
create mode 160000 ovs
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 2a711a1b9..af81de920 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -10,8 +10,8 @@ EXTRA_OPTS="--enable-Werror"
function configure_ovs()
{
- git clone https://github.com/openvswitch/ovs.git ovs_src
- pushd ovs_src
+ git submodule update --init
We could use the builtin GH actions support instead, to automatically
clone the submodules [0]. We could update the "checkout" step in
test.yml to:
- name: Checkout repository and submodules
uses: actions/checkout@v2
with:
submodules: recursive
Any reason to not do this?
[0] https://github.com/marketplace/actions/checkout-submodules
Thanks Dumitru. Simply put, I didn't know that was an option :)
I'll update it.
+ pushd ovs
./boot.sh && ./configure $* || { cat config.log; exit 1; }
make -j4 || { cat config.log; exit 1; }
popd
@@ -22,7 +22,7 @@ function configure_ovn()
configure_ovs $*
export OVS_CFLAGS="${OVS_CFLAGS} ${OVN_CFLAGS}"
- ./boot.sh && ./configure --with-ovs-source=$PWD/ovs_src $* || \
+ ./boot.sh && ./configure $* || \
{ cat config.log; exit 1; }
}
@@ -54,7 +54,7 @@ if [ "$TESTSUITE" ]; then
# Now we only need to prepare the Makefile without
sparse-wrapped CC.
configure_ovn
- export DISTCHECK_CONFIGURE_FLAGS="$OPTS
--with-ovs-source=$PWD/ovs_src"
+ export DISTCHECK_CONFIGURE_FLAGS="$OPTS"
if ! make distcheck -j4 TESTSUITEFLAGS="-j4" RECHECK=yes; then
# testsuite.log is necessary for debugging.
cat */_build/sub/tests/testsuite.log
diff --git a/.ci/osx-build.sh b/.ci/osx-build.sh
index 6617f0b9d..423d82c1d 100755
--- a/.ci/osx-build.sh
+++ b/.ci/osx-build.sh
@@ -7,8 +7,8 @@ EXTRA_OPTS=""
function configure_ovs()
{
- git clone https://github.com/openvswitch/ovs.git ovs_src
- pushd ovs_src
+ git submodule update --init
Same here.
+ pushd ovs
./boot.sh && ./configure $*
make -j4 || { cat config.log; exit 1; }
popd
@@ -17,7 +17,7 @@ function configure_ovs()
function configure_ovn()
{
configure_ovs $*
- ./boot.sh && ./configure $* --with-ovs-source=$PWD/ovs_src
+ ./boot.sh && ./configure $*
}
configure_ovn $EXTRA_OPTS $*
@@ -32,7 +32,7 @@ if ! "$@"; then
exit 1
fi
if [ "$TESTSUITE" ] && [ "$CC" != "clang" ]; then
- export DISTCHECK_CONFIGURE_FLAGS="$EXTRA_OPTS
--with-ovs-source=$PWD/ovs_src"
+ export DISTCHECK_CONFIGURE_FLAGS="$EXTRA_OPTS"
if ! make distcheck RECHECK=yes; then
# testsuite.log is necessary for debugging.
cat */_build/sub/tests/testsuite.log
diff --git a/.gitmodules b/.gitmodules
new file mode 100644
index 000000000..f0d1f8cbe
--- /dev/null
+++ b/.gitmodules
@@ -0,0 +1,3 @@
+[submodule "ovs"]
+ path = ovs
+ url = https://github.com/openvswitch/ovs.git
diff --git a/Documentation/intro/install/general.rst
b/Documentation/intro/install/general.rst
index 65b1f4a40..e077bd8fb 100644
--- a/Documentation/intro/install/general.rst
+++ b/Documentation/intro/install/general.rst
@@ -66,6 +66,9 @@ To compile the userspace programs in the OVN
distribution, you will
need the following software:
- Open vSwitch (https://docs.openvswitch.org/en/latest/intro/install/).
+ Open vSwitch is included as a submodule in the OVN source code. It is
+ kept at the minimum version required in order for OVN to compile. See
I still think we should rephrase this part. We now mention further down
below that the "OVS submodule is guaranteed to be the minimum
recommended version of OVS to ensure OVN's optimal operation" which
slightly contradicts with this statement: "kept at the minimum version
required in order for OVN to compile".
Hm, I thought I had removed any wording about minimum version required
to compile. I guess I missed this one.
+ below for instructions about how to use a different OVS source
location.
- GNU make
@@ -140,27 +143,44 @@ Bootstrapping
-------------
This step is not needed if you have downloaded a released tarball. If
-you pulled the sources directly from an Open vSwitch Git tree or got a
-Git tree snapshot, then run boot.sh in the top source directory to build
+you pulled the sources directly from an OVN Git tree or got a Git tree
+snapshot, then run boot.sh in the top source directory to build
the "configure" script::
$ ./boot.sh
-Before configuring OVN, clone, configure and build Open vSwitch.
+Before configuring OVN, build Open vSwitch. The easiest way to do this
+is to use the included OVS submodule in the OVN source::
+
+ $ git submodule update --init
+ $ cd ovs
+ $ ./boot.sh
+ $ ./configure
+ $ make
+ $ cd ..
+
+It is not required to use the included OVS submodule; however the OVS
+submodule is guaranteed to be the minimum recommended version of OVS
+to ensure OVN's optimal operation. If you wish to use OVS source code
+from a different location on the file system, then be sure to configure
+and build OVS before building OVN.
.. _general-configuring:
Configuring
-----------
-Configure the package by running the configure script. You need to
-invoke configure with atleast the argument --with-ovs-source.
-For example::
+Then configure the package by running the configure script::
+
+ $ ./configure
+
+If your OVS source directory is not the included OVS submodule,
specify the
+location of the OVS source code using --with-ovs-source::
$ ./configure --with-ovs-source=/path/to/ovs/source
-If you have built Open vSwitch in a separate directory, then you
-need to provide that path in the option - --with-ovs-build.
+If you have built Open vSwitch in a separate directory from its source
+code, then you need to provide that path in the option -
--with-ovs-build.
By default all files are installed under ``/usr/local``. OVN expects
to find
its database in ``/usr/local/etc/ovn`` by default.
diff --git a/Makefile.am b/Makefile.am
index 7ce3d27e4..436de8ff2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -48,6 +48,8 @@ AM_CFLAGS = -Wstrict-prototypes
AM_CFLAGS += $(WARNING_FLAGS)
AM_CFLAGS += $(OVS_CFLAGS)
+AM_DISTCHECK_CONFIGURE_FLAGS = --with-ovs-source=$(PWD)/ovs
+
if NDEBUG
AM_CPPFLAGS += -DNDEBUG
AM_CFLAGS += -fomit-frame-pointer
@@ -157,6 +159,7 @@ noinst_HEADERS += $(EXTRA_DIST)
ro_c = echo '/* -*- mode: c; buffer-read-only: t -*- */'
ro_shell = printf '\043 Generated automatically -- do not modify!
-*- buffer-read-only: t -*-\n'
+submodules = $(shell grep 'path =' .gitmodules | sed -E 's/[\t ]*path
=\s*(.*)/\1/g' | xargs)
SUFFIXES += .in
.in:
@@ -213,9 +216,12 @@ CLEAN_LOCAL += clean-pycov
# file that is in Git is distributed.
ALL_LOCAL += dist-hook-git
dist-hook-git: distfiles
+ @echo "submodules is \"$(submodules)\""
I suppose this was for debugging and it's not really needed anymore.
Yes. I had to do about 138947239 test runs while debugging OSX and this
helped to figure out the whitespace issue that OSX had.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
