On 8 July 2015 at 07:28, Maxim Uvarov <maxim.uva...@linaro.org> wrote:
> On 07/07/15 21:20, Mike Holmes wrote: > > >> >> On 7 July 2015 at 06:34, Maxim Uvarov <maxim.uva...@linaro.org <mailto: >> maxim.uva...@linaro.org>> wrote: >> >> this path install helper headers to /usr/include that is not correct. >> Right place is /usr/include/odp/helper due to 2 reasons: >> >> 1. be consistent to other projects and headers: >> /usr/include/linux/udp.h >> /usr/include/netinet/udp.h >> So put it under odp. >> >> 2. C code has: >> <odp/helper/ring.h> >> Please do packaging to the same directory as source code has. >> Changing paths during install >> makes it harder to find. >> >> >> Sounds good, I missed the "nobase_" prefix and that allow some other >> clean up of the structure without changing the functionality - thanks :) >> It also meant that checkpatch requires clean up of moved fines :( >> >> in v3 it is now >> >> ├── bin >> ├── include >> │ └── odp >> │ ├── api >> │ ├── helper <----- moved it to here >> │ └── plat >> ├── lib >> │ └── pkgconfig >> └── share >> └── opendataplane >> >> >> >> And also you removed install of odph_debug.h and odph_pause.h. >> That is not correct. I think you should >> remove odph_ prefix and put files to location with all others >> helper includes. >> >> >> These were also local to the src before and are not intended to be part >> of the helper API, they just support compiling the helper lib. This is in >> line with the platforms that do not install headers for internal >> functionality. >> >> How do other people understand that it's internal? I think just as with the platforms if the code is local and the makefile treats it as local it is local, if it is in the directory specifically set aside for the public API it is. > What is criteria for external and internal? Internal such as the debug interface is something by design we do not want an application to call, the external interface is the one explicitly exported for use by others - it defines the API that full fulls the purpose of the library to others and not just for the construction of the implementation. > Should we also put that files to separate folder in src directory? We can create another internal header files for ODP dir, but I dont think we need it yet. Having the .h and the .c right there together that form the implement of the API is fairly clear to me that they are all internal, but I will move it if there is consensus. I do not want us to go adding _internal.h to everything as we have with ODP, by default it should be clear that if it is not exported from the external API directory it is internal, no need for extra typing and longer include paths. > Also I don't really understand why app developer can use odph library and > can not use odph_pause(). > The moment we make them public we have to maintain support for them and they are not central to ODPs purpose, ODP abstracts networking not logging or OS independent pause > Maxim. > > > >> Best regards, >> Maxim. >> >> >> >> On 07/07/15 09:47, Christophe Milard wrote: >> >> On 2015-07-06 15:13, Mike Holmes wrote: >> >> Remove the need to build helper source files into the >> linux-generic >> library by converting helpers to be their own library. >> >> This removes the need for all other platforms to also >> build in the >> helpers which are optional just to run the tests. >> >> Signed-off-by: Mike Holmes <mike.hol...@linaro.org >> <mailto:mike.hol...@linaro.org>> >> >> Reviewed-by: Christophe Milard <christophe.mil...@linaro.org >> <mailto:christophe.mil...@linaro.org>> >> >> --- >> >> v2: >> use srcdir to shorten paths >> >> >> .gitignore | 2 +- >> Makefile.am | 3 ++- >> configure.ac <http://configure.ac> | 1 + >> debian/libodphelper-dev.dirs | 2 ++ >> debian/libodphelper-dev.install | 4 ++++ >> debian/libodphelper.dirs | 1 + >> debian/libodphelper.install | 1 + >> example/Makefile.inc | 2 +- >> helper/Makefile.am | 30 >> +++++++++++++++++++++++++++++- >> helper/test/Makefile.am | 2 ++ >> pkgconfig/libodphelper.pc.in <http://libodphelper.pc.in> >> | 11 +++++++++++ >> platform/linux-generic/Makefile.am | 16 ---------------- >> test/Makefile.inc | 2 +- >> test/validation/Makefile.inc | 2 +- >> 14 files changed, 57 insertions(+), 22 deletions(-) >> create mode 100644 debian/libodphelper-dev.dirs >> create mode 100644 debian/libodphelper-dev.install >> create mode 100644 debian/libodphelper.dirs >> create mode 100644 debian/libodphelper.install >> create mode 100644 pkgconfig/libodphelper.pc.in >> <http://libodphelper.pc.in> >> >> >> diff --git a/.gitignore b/.gitignore >> index 6222fd9..4dbf28e 100644 >> --- a/.gitignore >> +++ b/.gitignore >> @@ -24,7 +24,7 @@ missing >> config.log >> config.status >> libtool >> -pkgconfig/libodp.pc >> +pkgconfig/libodp*.pc >> .deps/ >> cscope.out >> tags >> diff --git a/Makefile.am b/Makefile.am >> index 2c8a9d6..7ce3a3c 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -3,9 +3,10 @@ AUTOMAKE_OPTIONS = foreign >> #@with_platform@ works alone in subdir but not as part >> of a path??? >> SUBDIRS = @platform_with_platform@ \ >> + helper \ >> test \ >> @platform_with_platform_test@ \ >> - helper \ >> + helper/test \ >> doc \ >> example \ >> scripts >> diff --git a/configure.ac <http://configure.ac> >> b/configure.ac <http://configure.ac> >> index 28dad3b..29fcb18 100644 >> --- a/configure.ac <http://configure.ac> >> +++ b/configure.ac <http://configure.ac> >> @@ -296,6 +296,7 @@ AC_CONFIG_FILES([Makefile >> helper/Makefile >> helper/test/Makefile >> pkgconfig/libodp.pc >> + pkgconfig/libodphelper.pc >> platform/linux-generic/Makefile >> platform/linux-generic/test/pktio/Makefile >> scripts/Makefile >> diff --git a/debian/libodphelper-dev.dirs >> b/debian/libodphelper-dev.dirs >> new file mode 100644 >> index 0000000..4418816 >> --- /dev/null >> +++ b/debian/libodphelper-dev.dirs >> @@ -0,0 +1,2 @@ >> +usr/lib >> +usr/include >> diff --git a/debian/libodphelper-dev.install >> b/debian/libodphelper-dev.install >> new file mode 100644 >> index 0000000..b973af4 >> --- /dev/null >> +++ b/debian/libodphelper-dev.install >> @@ -0,0 +1,4 @@ >> +usr/include/* >> +usr/lib/*/lib*.so >> +usr/lib/*/lib*.a >> +usr/lib/*/pkgconfig/* >> diff --git a/debian/libodphelper.dirs >> b/debian/libodphelper.dirs >> new file mode 100644 >> index 0000000..6845771 >> --- /dev/null >> +++ b/debian/libodphelper.dirs >> @@ -0,0 +1 @@ >> +usr/lib >> diff --git a/debian/libodphelper.install >> b/debian/libodphelper.install >> new file mode 100644 >> index 0000000..3ddde58 >> --- /dev/null >> +++ b/debian/libodphelper.install >> @@ -0,0 +1 @@ >> +usr/lib/*/lib*.so.* >> diff --git a/example/Makefile.inc b/example/Makefile.inc >> index b3a9706..e1c1cb7 100644 >> --- a/example/Makefile.inc >> +++ b/example/Makefile.inc >> @@ -1,7 +1,7 @@ >> include $(top_srcdir)/Makefile.inc >> include $(top_srcdir)/platform/@with_platform@/Makefile.inc >> LIB = $(top_builddir)/lib >> -LDADD = $(LIB)/libodp.la <http://libodp.la> >> +LDADD = $(LIB)/libodp.la <http://libodp.la> >> $(LIB)/libodphelper.la <http://libodphelper.la> >> AM_CFLAGS += \ >> -I$(srcdir) \ >> -I$(top_srcdir)/example \ >> diff --git a/helper/Makefile.am b/helper/Makefile.am >> index 02af5b3..44bcc3d 100644 >> --- a/helper/Makefile.am >> +++ b/helper/Makefile.am >> @@ -1 +1,29 @@ >> -SUBDIRS = test >> +pkgconfigdir = $(libdir)/pkgconfig >> +pkgconfig_DATA = $(top_builddir)/pkgconfig/libodphelper.pc >> + >> +LIB = $(top_builddir)/lib >> +AM_CFLAGS = -I$(srcdir)/include >> +AM_CFLAGS += -I$(top_srcdir)/platform/@with_platform@ >> /include >> +AM_CFLAGS += -I$(top_srcdir)/platform/linux-generic/include >> +AM_CFLAGS += -I$(top_srcdir)/include >> + >> +include_HEADERS = \ >> + $(srcdir)/include/odp/helper/ring.h \ >> + $(srcdir)/include/odp/helper/linux.h \ >> + $(srcdir)/include/odp/helper/chksum.h\ >> + $(srcdir)/include/odp/helper/eth.h\ >> + $(srcdir)/include/odp/helper/icmp.h\ >> + $(srcdir)/include/odp/helper/ip.h\ >> + $(srcdir)/include/odp/helper/ipsec.h\ >> + $(srcdir)/include/odp/helper/tcp.h\ >> + $(srcdir)/include/odp/helper/udp.h >> + >> +noinst_HEADERS = \ >> + $(srcdir)/odph_debug.h \ >> + $(srcdir)/odph_pause.h >> + >> +__LIB__libodphelper_la_SOURCES = \ >> + linux.c \ >> + ring.c >> + >> +lib_LTLIBRARIES = $(LIB)/libodphelper.la >> <http://libodphelper.la> >> diff --git a/helper/test/Makefile.am b/helper/test/Makefile.am >> index 9ac82eb..aed97cc 100644 >> --- a/helper/test/Makefile.am >> +++ b/helper/test/Makefile.am >> @@ -24,4 +24,6 @@ bin_PROGRAMS = $(EXECUTABLES) >> $(COMPILE_ONLY) >> dist_odp_chksum_SOURCES = odp_chksum.c >> dist_odp_thread_SOURCES = odp_thread.c >> +odp_thread_LDADD = $(LIB)/libodphelper.la >> <http://libodphelper.la> $(LIB)/libodp.la <http://libodp.la> >> dist_odp_process_SOURCES = odp_process.c >> +odp_process_LDADD = $(LIB)/libodphelper.la >> <http://libodphelper.la> $(LIB)/libodp.la <http://libodp.la> >> diff --git a/pkgconfig/libodphelper.pc.in >> <http://libodphelper.pc.in> b/pkgconfig/libodphelper.pc.in >> <http://libodphelper.pc.in> >> new file mode 100644 >> index 0000000..2993d71 >> --- /dev/null >> +++ b/pkgconfig/libodphelper.pc.in <http://libodphelper.pc.in >> > >> >> @@ -0,0 +1,11 @@ >> +prefix=@prefix@ >> +exec_prefix=@exec_prefix@ >> +libdir=@libdir@ >> +includedir=@includedir@ >> + >> +Name: libodphelper >> +Description: Helper for the ODP packet processing engine >> +Version: @VERSION@ >> +Libs: -L${libdir} -lodphelper >> +Libs.private: >> +Cflags: -I${includedir} >> diff --git a/platform/linux-generic/Makefile.am >> b/platform/linux-generic/Makefile.am >> index 4f2063f..8cf03b2 100644 >> --- a/platform/linux-generic/Makefile.am >> +++ b/platform/linux-generic/Makefile.am >> @@ -127,20 +127,6 @@ noinst_HEADERS = \ >> >> ${top_srcdir}/platform/linux-generic/include/odp_timer_internal.h >> \ >> ${top_srcdir}/platform/linux-generic/Makefile.inc >> -subdirheadersdir = $(includedir)/odp/helper >> -subdirheaders_HEADERS = \ >> - $(top_srcdir)/helper/include/odp/helper/chksum.h \ >> - $(top_srcdir)/helper/include/odp/helper/eth.h \ >> - $(top_srcdir)/helper/include/odp/helper/icmp.h \ >> - $(top_srcdir)/helper/include/odp/helper/ip.h \ >> - $(top_srcdir)/helper/include/odp/helper/ipsec.h \ >> - $(top_srcdir)/helper/include/odp/helper/linux.h \ >> - $(top_srcdir)/helper/include/odp/helper/ring.h \ >> - $(top_srcdir)/helper/include/odp/helper/tcp.h \ >> - $(top_srcdir)/helper/include/odp/helper/udp.h \ >> - $(top_srcdir)/helper/odph_debug.h \ >> - $(top_srcdir)/helper/odph_pause.h >> - >> __LIB__libodp_la_SOURCES = \ >> odp_barrier.c \ >> odp_buffer.c \ >> @@ -151,14 +137,12 @@ __LIB__libodp_la_SOURCES = \ >> odp_event.c \ >> odp_init.c \ >> odp_impl.c \ >> - ../../helper/linux.c \ >> odp_packet.c \ >> odp_packet_flags.c \ >> odp_packet_io.c \ >> odp_packet_socket.c \ >> odp_pool.c \ >> odp_queue.c \ >> - ../../helper/ring.c \ >> odp_rwlock.c \ >> odp_schedule.c \ >> odp_shared_memory.c \ >> diff --git a/test/Makefile.inc b/test/Makefile.inc >> index 1eb6ed5..2fa61e4 100644 >> --- a/test/Makefile.inc >> +++ b/test/Makefile.inc >> @@ -5,7 +5,7 @@ LIB = $(top_builddir)/lib >> #in the following line, the libs using the symbols >> should come before >> #the libs containing them! The includer is given a >> chance to add things >> #before libodp by setting PRE_LDADD before the inclusion. >> -LDADD = $(PRE_LDADD) $(LIB)/libodp.la <http://libodp.la> >> +LDADD = $(PRE_LDADD) $(LIB)/libodphelper.la >> <http://libodphelper.la> $(LIB)/libodp.la <http://libodp.la> >> INCFLAGS = -I$(srcdir) \ >> -I$(top_srcdir)/test \ >> diff --git a/test/validation/Makefile.inc >> b/test/validation/Makefile.inc >> index 3cdc6a7..31729b8 100644 >> --- a/test/validation/Makefile.inc >> +++ b/test/validation/Makefile.inc >> @@ -4,4 +4,4 @@ AM_CFLAGS += >> -I$(top_srcdir)/test/validation/common >> AM_LDFLAGS += -static >> LIBCUNIT_COMMON = >> $(top_builddir)/test/validation/common/libcunit_common.a >> -LIBODP = $(LIB)/libodp.la <http://libodp.la> >> +LIBODP = $(LIB)/libodphelper.la <http://libodphelper.la> >> $(LIB)/libodp.la <http://libodp.la> >> -- 2.1.4 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> >> > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs
_______________________________________________ lng-odp mailing list lng-odp@lists.linaro.org https://lists.linaro.org/mailman/listinfo/lng-odp
