travelping / upg-vpp

User Plane Gateway (UPG) based on VPP
Apache License 2.0
146 stars 51 forks source link

Compilation failure for upg-vpp on host system (outside docker) #354

Closed anshulthakur closed 1 year ago

anshulthakur commented 1 year ago

Hi, I'm trying to compile the VPP source outside the docker environment. Building it gives the following error:

/home/test/upf/upg-vpp/upf/pfcp.c:429:3: error: expression is not assignable
  put_u8 (*vec, *v);
  ^~~~~~~~~~~~~~~~~
/home/test/upf/upg-vpp/upf/pfcp.c:238:19: note: expanded from macro 'put_u8'
    _vec_len((V)) += sizeof(u8);                \

Please help fix this issue.

I've followed these steps:

  1. Cloned fpp-vpp and checked out to its last release(this sets vpp image to 22.01)
  2. Run ./hack/update-vpp.sh
  3. Compiled vpp:
    cd vpp
    make install-dep
    make build
    make install-ext-deps
    make pkg-deb
    ls build-root/*.deb
    sudo dpkg -i build-root/*.deb
  4. Cloned upg-vpp and checked out to most recent release (1.9.0)
  5. To allow the Make scripts to find the source folder (it tries to find the folder at mount point /src, which is fine for docker, but not true for a host build), modified the hack/build-internal.sh as
 : ${BUILD_TYPE:=release}
+: ${SRC_DIR:=/src}

 cd "$(dirname "${BASH_SOURCE}")/.."

@@ -22,6 +23,6 @@ case "${BUILD_TYPE}" in
      ;;
 esac

-cmake -DCMAKE_BUILD_TYPE="${btype}" -DCMAKE_INSTALL_PREFIX=/usr /src
+cmake -DCMAKE_BUILD_TYPE="${btype}" -DCMAKE_INSTALL_PREFIX=/usr ${SRC_DIR}

-make -C /src/build-root "$@"
+make -C ${SRC_DIR}/build-root "$@"

(Note that this doesn't change anything inside the docker environment, which builds fine)

  1. Run make install

This gives me an error:

/home/test/upf/upg-vpp/upf/pfcp.c:429:3: error: expression is not assignable
  put_u8 (*vec, *v);
  ^~~~~~~~~~~~~~~~~
/home/test/upf/upg-vpp/upf/pfcp.c:238:19: note: expanded from macro 'put_u8'
    _vec_len((V)) += sizeof(u8);                \

On looking into the implementation of _vec_len in /usr/include/vppinfra/vec_bootstrap.h, the code says:

/** \brief Number of elements in vector (rvalue-only, NULL tolerant)

    vec_len (v) checks for NULL, but cannot be used as an lvalue.
    If in doubt, use vec_len...
*/
#define _vec_find(v)    ((vec_header_t *) (v) - 1)

static_always_inline u32
__vec_len (void *v)
{
  return _vec_find (v)->len;
}

#define _vec_len(v)     __vec_len ((void *) (v))

From the file vec_bootstrap.h, it does seem that the assignment is not proper. How do I go about fixing this issue?

Also, the compiler complains (warning) of a re-definition:

home/test/upf/upg-vpp/upf/pfcp.h:13:9: warning: 'SET_BIT' macro redefined [-Wmacro-redefined]
#define SET_BIT(mask, n)    ((mask) |= BIT(n))
        ^
/usr/include/vppinfra/bitops.h:41:9: note: previous definition is here
#define SET_BIT(i)    (1 << i)

UPDATE I think I've figured out the problem. The images that docker builds uses the archived VPP image forked into https://github.com/travelping/vpp . That uses the old definitions. So, if I switch to this repo, things should work. Am I right? Also, in that case, how would the build pass with fpp-vpp repo using the images from the official sources?

mitmitmitm commented 1 year ago

1 Cloned fpp-vpp and checked out to its last release(this sets vpp image to 22.01)

Did you misread and meant to write 22.10 here?

2 Run ./hack/update-vpp.sh 3 Compiled vpp:

6 Run make install

This gives me an error:

/home/test/upf/upg-vpp/upf/pfcp.c:429:3: error: expression is not assignable put_u8 (vec, v); ^~~~~ /home/test/upf/upg-vpp/upf/pfcp.c:238:19: note: expanded from macro 'put_u8' _vec_len((V)) += sizeof(u8); \

On looking into the implementation of _vec_len in /usr/include/vppinfra/vec_bootstrap.h, the code says:

/** \brief Number of elements in vector (rvalue-only, NULL tolerant)

vec_len (v) checks for NULL, but cannot be used as an lvalue.
If in doubt, use vec_len...

*/

define _vec_find(v) ((vec_header_t *) (v) - 1)

static_always_inline u32 __vec_len (void *v) { return _vec_find (v)->len; }

define _vec_len(v) __vec_len ((void *) (v))

Definition of _vec_len() was changed to this read-only version in vpp version v22.10. I think that you should check out fpp-vpp's 'stable/22.02' branch which comes with vpp version v22.02 that still has a read-write definition of _vec_len().

The version of vpp that fpp-vpp will use is specified in fpp-vpp's 'vpp.spec' file. You can also look at upg-vpp's 'vpp.spec' file to see which version of fpp-vpp upg-vpp expects.

anshulthakur commented 1 year ago

Did you misread and meant to write 22.10 here?

Sorry, my bad. Yes, I meant to write 22.10.

Also, checking out to the appropriate branch fixed the compilation issue. So, thank you for helping figure that out!

anshulthakur commented 1 year ago

In the meanwhile, I also fixed the compilation issues by replacing the _vec_len(V) calls to _vec_find(V)->len, and other API-related changes without thinking much about whether this would break the code. I'll test them with the unit tests to verify. Is it okay to generate a PR for that if the changes don't break anything or is it already in the pipeline?

mitmitmitm commented 1 year ago

In the meanwhile, I also fixed the compilation issues by replacing the _vec_len(V) calls to _vec_find(V)->len, and other API-related changes without thinking much about whether this would break the code. I'll test them with the unit tests to verify. Is it okay to generate a PR for that if the changes don't break anything or is it already in the pipeline?

Thanks. You may want to see upg-vpp branch 'feature/vpp-22.10' where they seem to have already started work in this direction. Specifically commit 24d605fb54486419a8bff221c598e645870a44ef, where they replaced '_vec_len(v) = foo' with '_vec_set_len(v, foo)', though I don't know about the state of the branch.