| Message ID | 1399307007-15617-1-git-send-email-taras.kondratiuk@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 5 May 2014 18:23, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > Applications must not refer to includes in platform directory. > Instead they shall use headers from DESTDIR. > > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> > --- > Makefile | 3 +-- > Makefile.inc | 1 + > test/Makefile.inc | 11 +---------- > 3 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/Makefile b/Makefile > index 2512343..7d10fd5 100644 > --- a/Makefile > +++ b/Makefile > @@ -5,9 +5,8 @@ > > .DEFAULT_GOAL := default > > -ODP_ROOT = $(PWD) > +ODP_ROOT = $(CURDIR) > ODP_TESTS = $(ODP_ROOT)/test > -export DESTDIR = $(ODP_ROOT)/build > > include $(ODP_ROOT)/Makefile.inc > > diff --git a/Makefile.inc b/Makefile.inc > index a5aeb8b..d725137 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -7,6 +7,7 @@ PLATFORM ?= linux-generic > OBJ_DIR = ./obj > ODP_DIR = $(ODP_ROOT)/platform/$(PLATFORM) > > +export DESTDIR = $(ODP_ROOT)/build > > CC ?= gcc > LD ?= gcc > diff --git a/test/Makefile.inc b/test/Makefile.inc > index f001119..b732c56 100644 > --- a/test/Makefile.inc > +++ b/test/Makefile.inc > @@ -3,19 +3,10 @@ > # > # SPDX-License-Identifier: BSD-3-Clause > > -ifdef DESTDIR > - > ODP_LIB = $(DESTDIR)/lib/libodp.a > EXTRA_CFLAGS += -I$(DESTDIR)/include > EXTRA_CFLAGS += -I$(DESTDIR)/include/api > > -else > - > -ODP_LIB = $(ODP_DIR)/lib/libodp.a > -EXTRA_CFLAGS += -I$(ODP_ROOT)/include > -EXTRA_CFLAGS += -I$(ODP_DIR)/include/api > - > $(ODP_LIB): > @echo Building $@ > - $(MAKE) -C $(ODP_DIR) libs > -endif > + $(MAKE) -C $(ODP_ROOT) libs_install > -- > 1.7.9.5 > > if you change a public header file and type rebuild from inside test/*/ directory that will have no effect. there is a dependency issue here. Cheers, Anders
On 05/06/2014 01:50 PM, Anders Roxell wrote: > > > > On 5 May 2014 18:23, Taras Kondratiuk <taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>> wrote: > > Applications must not refer to includes in platform directory. > Instead they shall use headers from DESTDIR. > > Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org > <mailto:taras.kondratiuk@linaro.org>> > --- > Makefile | 3 +-- > Makefile.inc | 1 + > test/Makefile.inc | 11 +---------- > 3 files changed, 3 insertions(+), 12 deletions(-) > > diff --git a/Makefile b/Makefile > index 2512343..7d10fd5 100644 > --- a/Makefile > +++ b/Makefile > @@ -5,9 +5,8 @@ > > .DEFAULT_GOAL := default > > -ODP_ROOT = $(PWD) > +ODP_ROOT = $(CURDIR) > ODP_TESTS = $(ODP_ROOT)/test > -export DESTDIR = $(ODP_ROOT)/build > > include $(ODP_ROOT)/Makefile.inc > > diff --git a/Makefile.inc b/Makefile.inc > index a5aeb8b..d725137 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -7,6 +7,7 @@ PLATFORM ?= linux-generic > OBJ_DIR = ./obj > ODP_DIR = $(ODP_ROOT)/platform/$(PLATFORM) > > +export DESTDIR = $(ODP_ROOT)/build > > CC ?= gcc > LD ?= gcc > diff --git a/test/Makefile.inc b/test/Makefile.inc > index f001119..b732c56 100644 > --- a/test/Makefile.inc > +++ b/test/Makefile.inc > @@ -3,19 +3,10 @@ > # > # SPDX-License-Identifier: BSD-3-Clause > > -ifdef DESTDIR > - > ODP_LIB = $(DESTDIR)/lib/libodp.a > EXTRA_CFLAGS += -I$(DESTDIR)/include > EXTRA_CFLAGS += -I$(DESTDIR)/include/api > > -else > - > -ODP_LIB = $(ODP_DIR)/lib/libodp.a > -EXTRA_CFLAGS += -I$(ODP_ROOT)/include > -EXTRA_CFLAGS += -I$(ODP_DIR)/include/api > - > $(ODP_LIB): > @echo Building $@ > - $(MAKE) -C $(ODP_DIR) libs > -endif > + $(MAKE) -C $(ODP_ROOT) libs_install > -- > 1.7.9.5 > > > if you change a public header file and type rebuild from inside > test/*/ directory that will have no effect. > there is a dependency issue here. > > Cheers, > Anders > > Agree, I don't think it's good idea to include this patch. You can compile tests without doing make install. Maxim. > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp
On 05/06/2014 12:50 PM, Anders Roxell wrote: > > if you change a public header file and type rebuild from inside test/*/ > directory that will have no effect. > there is a dependency issue here. > Right. In a current patch libs_install is called from tests' Makefile only if $(DESTDIR)/lib/libodp.a does not exist. So as you pointed library and headers won't be updated if already exist in DESTDIR. Main point of this patch is to ensure that test applications use only DESTDIR content (I hope everybody agree on this?). If some headers are changed they should be installed into DESTDIR via libs_install. So to update headers and libraries from tests' Makefile we have to call libs_install. It can't be called conditionally, because we don't have information at tests directory level to make this decision. But it will add overhead if called unconditionally on each tests build. In short, IMO it is worth to remove ability to build and install ODP library from within tests directory. It adds more issues that has advantages. I'll post a next patch as RFC.
On 05/06/2014 01:22 PM, Maxim Uvarov wrote: > On 05/06/2014 01:50 PM, Anders Roxell wrote: >> >> >> >> On 5 May 2014 18:23, Taras Kondratiuk <taras.kondratiuk@linaro.org >> <mailto:taras.kondratiuk@linaro.org>> wrote: >> >> Applications must not refer to includes in platform directory. >> Instead they shall use headers from DESTDIR. >> >> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org >> <mailto:taras.kondratiuk@linaro.org>> >> --- >> Makefile | 3 +-- >> Makefile.inc | 1 + >> test/Makefile.inc | 11 +---------- >> 3 files changed, 3 insertions(+), 12 deletions(-) >> >> diff --git a/Makefile b/Makefile >> index 2512343..7d10fd5 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -5,9 +5,8 @@ >> >> .DEFAULT_GOAL := default >> >> -ODP_ROOT = $(PWD) >> +ODP_ROOT = $(CURDIR) >> ODP_TESTS = $(ODP_ROOT)/test >> -export DESTDIR = $(ODP_ROOT)/build >> >> include $(ODP_ROOT)/Makefile.inc >> >> diff --git a/Makefile.inc b/Makefile.inc >> index a5aeb8b..d725137 100644 >> --- a/Makefile.inc >> +++ b/Makefile.inc >> @@ -7,6 +7,7 @@ PLATFORM ?= linux-generic >> OBJ_DIR = ./obj >> ODP_DIR = $(ODP_ROOT)/platform/$(PLATFORM) >> >> +export DESTDIR = $(ODP_ROOT)/build >> >> CC ?= gcc >> LD ?= gcc >> diff --git a/test/Makefile.inc b/test/Makefile.inc >> index f001119..b732c56 100644 >> --- a/test/Makefile.inc >> +++ b/test/Makefile.inc >> @@ -3,19 +3,10 @@ >> # >> # SPDX-License-Identifier: BSD-3-Clause >> >> -ifdef DESTDIR >> - >> ODP_LIB = $(DESTDIR)/lib/libodp.a >> EXTRA_CFLAGS += -I$(DESTDIR)/include >> EXTRA_CFLAGS += -I$(DESTDIR)/include/api >> >> -else >> - >> -ODP_LIB = $(ODP_DIR)/lib/libodp.a >> -EXTRA_CFLAGS += -I$(ODP_ROOT)/include >> -EXTRA_CFLAGS += -I$(ODP_DIR)/include/api >> - >> $(ODP_LIB): >> @echo Building $@ >> - $(MAKE) -C $(ODP_DIR) libs >> -endif >> + $(MAKE) -C $(ODP_ROOT) libs_install >> -- >> 1.7.9.5 >> >> >> if you change a public header file and type rebuild from inside >> test/*/ directory that will have no effect. >> there is a dependency issue here. >> >> Cheers, >> Anders >> >> > Agree, I don't think it's good idea to include this patch. You can > compile tests without doing make install. I can't agree. Tests should not differ from any other ODP application in a way they use ODP library. The fact that they are placed in ODP repo do not allow them to abuse it. By using directly library and includes from platform directly we break modularity. Tests' Makefile should have information about platform include directory structure. So tests' Makefile should be updated every time structure is changed. It is the best case. What if platform include directory structure differs between implementations?
I agree that there is a strong notion that tests should run against the installed headers & libs, looking down inside a platform to pick things up is a bad idea IMHO. But if we stop Tests auto building the library, we can always still move back up to the ODP dir which *can *build and install everything as needed, its not a big overhead to build from there during development, Assuming the dependencies are correct there is minimal overhead. On 6 May 2014 07:16, Taras Kondratiuk <taras.kondratiuk@linaro.org> wrote: > On 05/06/2014 01:22 PM, Maxim Uvarov wrote: > >> On 05/06/2014 01:50 PM, Anders Roxell wrote: >> >>> >>> >>> >>> On 5 May 2014 18:23, Taras Kondratiuk <taras.kondratiuk@linaro.org >>> <mailto:taras.kondratiuk@linaro.org>> wrote: >>> >>> Applications must not refer to includes in platform directory. >>> Instead they shall use headers from DESTDIR. >>> >>> Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org >>> <mailto:taras.kondratiuk@linaro.org>> >>> --- >>> Makefile | 3 +-- >>> Makefile.inc | 1 + >>> test/Makefile.inc | 11 +---------- >>> 3 files changed, 3 insertions(+), 12 deletions(-) >>> >>> diff --git a/Makefile b/Makefile >>> index 2512343..7d10fd5 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -5,9 +5,8 @@ >>> >>> .DEFAULT_GOAL := default >>> >>> -ODP_ROOT = $(PWD) >>> +ODP_ROOT = $(CURDIR) >>> ODP_TESTS = $(ODP_ROOT)/test >>> -export DESTDIR = $(ODP_ROOT)/build >>> >>> include $(ODP_ROOT)/Makefile.inc >>> >>> diff --git a/Makefile.inc b/Makefile.inc >>> index a5aeb8b..d725137 100644 >>> --- a/Makefile.inc >>> +++ b/Makefile.inc >>> @@ -7,6 +7,7 @@ PLATFORM ?= linux-generic >>> OBJ_DIR = ./obj >>> ODP_DIR = $(ODP_ROOT)/platform/$(PLATFORM) >>> >>> +export DESTDIR = $(ODP_ROOT)/build >>> >>> CC ?= gcc >>> LD ?= gcc >>> diff --git a/test/Makefile.inc b/test/Makefile.inc >>> index f001119..b732c56 100644 >>> --- a/test/Makefile.inc >>> +++ b/test/Makefile.inc >>> @@ -3,19 +3,10 @@ >>> # >>> # SPDX-License-Identifier: BSD-3-Clause >>> >>> -ifdef DESTDIR >>> - >>> ODP_LIB = $(DESTDIR)/lib/libodp.a >>> EXTRA_CFLAGS += -I$(DESTDIR)/include >>> EXTRA_CFLAGS += -I$(DESTDIR)/include/api >>> >>> -else >>> - >>> -ODP_LIB = $(ODP_DIR)/lib/libodp.a >>> -EXTRA_CFLAGS += -I$(ODP_ROOT)/include >>> -EXTRA_CFLAGS += -I$(ODP_DIR)/include/api >>> - >>> $(ODP_LIB): >>> @echo Building $@ >>> - $(MAKE) -C $(ODP_DIR) libs >>> -endif >>> + $(MAKE) -C $(ODP_ROOT) libs_install >>> -- >>> 1.7.9.5 >>> >>> >>> if you change a public header file and type rebuild from inside >>> test/*/ directory that will have no effect. >>> there is a dependency issue here. >>> >>> Cheers, >>> Anders >>> >>> >>> Agree, I don't think it's good idea to include this patch. You can >> compile tests without doing make install. >> > > I can't agree. > Tests should not differ from any other ODP application in a way they > use ODP library. The fact that they are placed in ODP repo do not allow > them to abuse it. By using directly library and includes from platform > directly we break modularity. Tests' Makefile should have information > about platform include directory structure. So tests' Makefile should > be updated every time structure is changed. It is the best case. What > if platform include directory structure differs between implementations? > > -- > Taras Kondratiuk > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > http://lists.linaro.org/mailman/listinfo/lng-odp >
On 05/06/2014 06:43 PM, Mike Holmes wrote: > I agree that there is a strong notion that tests should run against the > installed headers & libs, looking down inside a platform to pick things > up is a bad idea IMHO. > But if we stop Tests auto building the library, we can always still move > back up to the ODP dir which _can _build and install everything as > needed, its not a big overhead to build from there during development, > Assuming the dependencies are correct there is minimal overhead. If one want to stay in tests directory and rebuild library he can just invoke it manually: $make -C ../.. libs_install && make If we add unconditional libs_install call from tests' Makefile, then headers will be copied to DESTDIR and test application will be rebuilt on each make invocation. Even if nothing was changed. That's the reason I don't want to leave this call in Makefile.
diff --git a/Makefile b/Makefile index 2512343..7d10fd5 100644 --- a/Makefile +++ b/Makefile @@ -5,9 +5,8 @@ .DEFAULT_GOAL := default -ODP_ROOT = $(PWD) +ODP_ROOT = $(CURDIR) ODP_TESTS = $(ODP_ROOT)/test -export DESTDIR = $(ODP_ROOT)/build include $(ODP_ROOT)/Makefile.inc diff --git a/Makefile.inc b/Makefile.inc index a5aeb8b..d725137 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -7,6 +7,7 @@ PLATFORM ?= linux-generic OBJ_DIR = ./obj ODP_DIR = $(ODP_ROOT)/platform/$(PLATFORM) +export DESTDIR = $(ODP_ROOT)/build CC ?= gcc LD ?= gcc diff --git a/test/Makefile.inc b/test/Makefile.inc index f001119..b732c56 100644 --- a/test/Makefile.inc +++ b/test/Makefile.inc @@ -3,19 +3,10 @@ # # SPDX-License-Identifier: BSD-3-Clause -ifdef DESTDIR - ODP_LIB = $(DESTDIR)/lib/libodp.a EXTRA_CFLAGS += -I$(DESTDIR)/include EXTRA_CFLAGS += -I$(DESTDIR)/include/api -else - -ODP_LIB = $(ODP_DIR)/lib/libodp.a -EXTRA_CFLAGS += -I$(ODP_ROOT)/include -EXTRA_CFLAGS += -I$(ODP_DIR)/include/api - $(ODP_LIB): @echo Building $@ - $(MAKE) -C $(ODP_DIR) libs -endif + $(MAKE) -C $(ODP_ROOT) libs_install
Applications must not refer to includes in platform directory. Instead they shall use headers from DESTDIR. Signed-off-by: Taras Kondratiuk <taras.kondratiuk@linaro.org> --- Makefile | 3 +-- Makefile.inc | 1 + test/Makefile.inc | 11 +---------- 3 files changed, 3 insertions(+), 12 deletions(-)