[ovs-dev] [PATCH ovn v2] Include OVS as a git submodule.
Mark Michelson
mmichels at redhat.com
Mon Jan 25 13:24:44 UTC 2021
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 <mmichels at redhat.com>
>> ---
>> 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 at 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
>
More information about the dev
mailing list