On 7 March 2018 at 22:18, Wookey wookey@wookware.org wrote:

I've been packaging libopenCSD for debian. That has resulted in various changes, largely to the makefiles. Most of those changes should go upstream, rather than exist only in the debian version.

I'll post my issues/changes here for discussion so we can decide what is upstreamable.

In the meantime anyone interested can try the debian packages here: http://wookware.org/software/repo/ either manually from: http://wookware.org/software/repo/pool/main/libo/libopencsd/

or with deb [ trusted=yes ] http://wookware.org/software/repo unstable main apt update; apt install libopencsd0 (and/or libopencsd-dev libopencsd-doc libopencsd-dbgsym)

I have test this and things install and work seamlessly.

(for debian unstable, amd64 only) (package is signed, but repo isn't. Sorry) Or get the source and rebuild for a different debian-based distro/release.

I've not separated all my changes into proper standalone patches yet, but some are so let's start with those.

1. The doxygen doc build doesn't find all the components because they moved.

This fixes that: Index: libopencsd-0.8.0/decoder/docs/doxygen_config.dox =================================================================== --- libopencsd-0.8.0.orig/decoder/docs/doxygen_config.dox +++ libopencsd-0.8.0/decoder/docs/doxygen_config.dox @@ -765,11 +765,11 @@ WARN_LOGFILE =

INPUT = ../include \ ../include/interfaces \

•                     ../include/etmv3 \
  

•                     ../include/etmv4 \
  

•                     ../include/ptm \
  

•                     ../include/c_api \
  

•                     ../include/stm \
  

•                     ../include/opencsd/etmv3 \
  

•                     ../include/opencsd/etmv4 \
  

•                     ../include/opencsd/ptm \
  

•                     ../include/opencsd/c_api \
  

•                     ../include/opencsd/stm \
                      ../include/mem_acc \
                      ../../README.md \
                      . \
  
  
  

The makefile provides no doc-build. The patch below adds that.

I didn't include an install-docs target, although if you added one with a variable to set the target dir then I'd use it.

Index: libopencsd-0.8.0/decoder/build/linux/makefile

--- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -155,6 +155,13 @@ tests: libs cd $(OCSD_ROOT)/tests/build/linux/trc_pkt_lister && make cd $(OCSD_ROOT)/tests/build/linux/c_api_pkt_print_test && make

+# +# build docs +.PHONY: docs +docs:

•   (cd $(OCSD_ROOT/docs); doxygen doxygen_config.dox)
  

############################################################# # clean targets # @@ -176,3 +183,4 @@ clean_install: rm -f $(INSTALL_LIB_DIR)/lib$(LIB_BASE_NAME).so rm -f $(INSTALL_LIB_DIR)/lib$(LIB_CAPI_NAME).so rm -rf $(INSTALL_INCLUDE_DIR)/$(LIB_UAPI_INC_DIR)

•   rm -rf $(OCSD_ROOT)/docs/html
  
  
  

3. The static libraries are built but not installed.

Now static libraries aren't much use to anyone these days, but if you build them then you might as well install them.

This patch does that: Index: libopencsd-0.8.0/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -136,6 +136,8 @@ mkdir -p $(INSTALL_LIB_DIR) $(INSTALL_INCLUDE_DIR) $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so $(INSTALL_LIB_DIR)/ $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so $(INSTALL_LIB_DIR)/

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).a $(INSTALL_LIB_DIR)/
  

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).a $(INSTALL_LIB_DIR)/
    cd $(OCSD_ROOT)/build/linux/rctdl_c_api_lib && make install_inc
  
  

################################ @@ -192,6 +194,6 @@ cd $(OCSD_ROOT)/tests/build/linux/c_api_pkt_print_test && make clean

clean_install:

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_BASE_NAME).so
  

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_CAPI_NAME).so
  

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_BASE_NAME).{so,a}
  

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_CAPI_NAME).{so,a}
    rm -rf $(INSTALL_INCLUDE_DIR)/$(LIB_UAPI_INC_DIR)
  
  
  

4. The makefile arbitrarily restricts the build to arm and x86

architectures. The only reason for this is in order to build into a particular named directory, and to set -m32/-m64 flags for x86 which should be defaulted correctly on any sensible toolchain or cross-toolchain anyway.

This code can build, and be used, on any arch and the makefile should allow that, so this should be fixed.

I don't see the need to build into a host-arch named PLAT_DIR directory. Is there one? It only does one build at a time, whether crossing or not, so I see no reason not to use a fixed name for this dir, such as 'builddir'. This has no effect on the ability to cross or not. If you really want to change the name of PLAT_DIR then find out the HOST triplet and use that. On debian this is dpkg-architecture -q DEB_HOST_GNU_TYPE, (which is also the same prefix as would be used to specify the toolchain prefix for crossing). But like I say, I think a fixed builddir is actually all you need unless I'm missing something.

So this patch lets the build work on any arch: Index: libopencsd-0.8.0/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -92,27 +92,6 @@ BUILD_VARIANT=rel endif

-# platform bit size variant -ifeq ($(ARCH),x86)

• MFLAG:="-m32"
• BIT_VARIANT=32

-else ifeq ($(ARCH),x86_64)

• MFLAG:="-m64"
• BIT_VARIANT=64

-else ifeq ($(ARCH),arm)

• BIT_VARIANT=-arm

-else ifeq ($(ARCH),arm64)

• BIT_VARIANT=-arm64

-else ifeq ($(ARCH),aarch64)

• BIT_VARIANT=-arm64

-else ifeq ($(ARCH),aarch32)

• BIT_VARIANT=-arm

-endif

-MASTER_CC_FLAGS += $(MFLAG) -MASTER_CPP_FLAGS += $(MFLAG) -MASTER_LINKER_FLAGS += $(MFLAG)

# export build flags export MASTER_CC_FLAGS export MASTER_CPP_FLAGS @@ -120,7 +99,7 @@ export MASTER_LINKER_FLAGS export MASTER_LIB_FLAGS

# target directories -export PLAT_DIR=linux$(BIT_VARIANT)/$(BUILD_VARIANT) +export PLAT_DIR=builddir export LIB_TARGET_DIR=$(OCSD_LIB_ROOT)/$(PLAT_DIR) export LIB_TEST_TARGET_DIR=$(OCSD_TESTS)/lib/$(PLAT_DIR) export BIN_TEST_TARGET_DIR=$(OCSD_TESTS)/bin/$(PLAT_DIR)

I'm in agreement with the proposed changes - Mike please review on your side.

Thanks, Mathieu

All these patches included as files too.

Wookey

Principal hats: Linaro, Debian, Wookware, ARM http://wookware.org/

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

Hi Wookey,

Unfortunately for some reason, two of the four patch files would not apply - to either tag v0.8 or the current v0.8.1 (should have worked with both as the makefile had no significant change between 0.8.0 and 0.8.1. However, I've had a look and comments inline,,,

On 9 March 2018 at 17:58, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On 7 March 2018 at 22:18, Wookey wookey@wookware.org wrote:

...

I've been packaging libopenCSD for debian. That has resulted in various changes, largely to the makefiles. Most of those changes should go upstream, rather than exist only in the debian version.

I'll post my issues/changes here for discussion so we can decide what is upstreamable.

In the meantime anyone interested can try the debian packages here: http://wookware.org/software/repo/ either manually from: http://wookware.org/software/repo/pool/main/libo/libopencsd/

or with deb [ trusted=yes ] http://wookware.org/software/repo unstable main apt update; apt install libopencsd0

I was under the impression (per the docs links you sent) that the library naming convention was to have the version as libopencsd.so.0.8.0 (and variants) rather than libopencsd0.... Not sure what the 0 postfixed to the name is for - though I may be missing something here.

...

(and/or libopencsd-dev libopencsd-doc libopencsd-dbgsym)

I have test this and things install and work seamlessly.

...

(for debian unstable, amd64 only) (package is signed, but repo isn't. Sorry) Or get the source and rebuild for a different debian-based distro/release.

I've not separated all my changes into proper standalone patches yet, but some are so let's start with those.

1. The doxygen doc build doesn't find all the components because they moved.

This fixes that: Index: libopencsd-0.8.0/decoder/docs/doxygen_config.dox =================================================================== --- libopencsd-0.8.0.orig/decoder/docs/doxygen_config.dox +++ libopencsd-0.8.0/decoder/docs/doxygen_config.dox @@ -765,11 +765,11 @@ WARN_LOGFILE =

INPUT = ../include \ ../include/interfaces \

•                     ../include/etmv3 \
  

•                     ../include/etmv4 \
  

•                     ../include/ptm \
  

•                     ../include/c_api \
  

•                     ../include/stm \
  

•                     ../include/opencsd/etmv3 \
  

•                     ../include/opencsd/etmv4 \
  

•                     ../include/opencsd/ptm \
  

•                     ../include/opencsd/c_api \
  

•                     ../include/opencsd/stm \
                      ../include/mem_acc \
                      ../../README.md \
                      . \
  
  
  

The makefile provides no doc-build. The patch below adds that.

I didn't include an install-docs target, although if you added one with a variable to set the target dir then I'd use it.

No problem here - and this one did apply.

...

Index: libopencsd-0.8.0/decoder/build/linux/makefile

--- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -155,6 +155,13 @@ tests: libs cd $(OCSD_ROOT)/tests/build/linux/trc_pkt_lister && make cd $(OCSD_ROOT)/tests/build/linux/c_api_pkt_print_test && make

+# +# build docs +.PHONY: docs +docs:

•   (cd $(OCSD_ROOT/docs); doxygen doxygen_config.dox)
  

############################################################# # clean targets # @@ -176,3 +183,4 @@ clean_install: rm -f $(INSTALL_LIB_DIR)/lib$(LIB_BASE_NAME).so rm -f $(INSTALL_LIB_DIR)/lib$(LIB_CAPI_NAME).so rm -rf $(INSTALL_INCLUDE_DIR)/$(LIB_UAPI_INC_DIR)

•   rm -rf $(OCSD_ROOT)/docs/html
  
  

Happy with this too - for some reason the indexing was out compared to my system - did not apply.

...

3. The static libraries are built but not installed.

Now static libraries aren't much use to anyone these days, but if you build them then you might as well install them.

This patch does that: Index: libopencsd-0.8.0/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -136,6 +136,8 @@ mkdir -p $(INSTALL_LIB_DIR) $(INSTALL_INCLUDE_DIR) $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so $(INSTALL_LIB_DIR)/ $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so $(INSTALL_LIB_DIR)/

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).a $(INSTALL_LIB_DIR)/
  

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).a $(INSTALL_LIB_DIR)/
    cd $(OCSD_ROOT)/build/linux/rctdl_c_api_lib && make install_inc
  
  

################################ @@ -192,6 +194,6 @@ cd $(OCSD_ROOT)/tests/build/linux/c_api_pkt_print_test && make clean

clean_install:

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_BASE_NAME).so
  

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_CAPI_NAME).so
  

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_BASE_NAME).{so,a}
  

•   rm -f $(INSTALL_LIB_DIR)/lib$(LIB_CAPI_NAME).{so,a}
    rm -rf $(INSTALL_INCLUDE_DIR)/$(LIB_UAPI_INC_DIR)
  
  
  

Fine with this, though again as above did not apply.

...

4. The makefile arbitrarily restricts the build to arm and x86

architectures. The only reason for this is in order to build into a particular named directory, and to set -m32/-m64 flags for x86 which should be defaulted correctly on any sensible toolchain or cross-toolchain anyway.

Not sure this actually restricts the build architecture - merely customises target directory names. If ARCH is set to anything other than those filtered then the output will be in an undecorated ./linux/ directory, I'd agree with the comments re "sensible" defaults, but I've never seen any reason not to be specific to avoid unnecessary support calls.

...

This code can build, and be used, on any arch and the makefile should allow that, so this should be fixed.

I don't see the need to build into a host-arch named PLAT_DIR directory. Is there one? It only does one build at a time, whether crossing or not, so I see no reason not to use a fixed name for this dir, such as 'builddir'. This has no effect on the ability to cross or not. If you really want to change the name of PLAT_DIR then find out the HOST triplet and use that. On debian this is dpkg-architecture -q DEB_HOST_GNU_TYPE, (which is also the same prefix as would be used to specify the toolchain prefix for crossing). But like I say, I think a fixed builddir is actually all you need unless I'm missing something.

The reason behind this is twofold. 1) having explicit dbg/rel directories in my development environment allows me to build clients and pick out precisely the library version I want. This means that a command sequence such as: make clean make make DEBUG=1 will work, without the explicit dbg/rel dirs, the last command simply re-links the release code - which is not what I want.

2) having explicit bit / arch suffixed directories again helps in development - both on linux and windows (yes - it is a cross platform project so consistency is important). At ARM we have a system where we batch build libraries and keep all the build variants in sub-directories for later client builds. This reflects that practice too, so I guess for me there is a certain amount of "force of habit" there.

I have no objection to either the packager building with ARCH=<something-else> to get a predictable directory name, or indeed adding in extra branch on the filter to create an additional standard name. e.g. .... else BIT_VARIANT=-std endif ... so the default build will build to .../linux-std/rel

so although this patch does apply - I'd really rather not!

Thanks for the input.

Mike

...

So this patch lets the build work on any arch: Index: libopencsd-0.8.0/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -92,27 +92,6 @@ BUILD_VARIANT=rel endif

-# platform bit size variant -ifeq ($(ARCH),x86)

• MFLAG:="-m32"
• BIT_VARIANT=32

-else ifeq ($(ARCH),x86_64)

• MFLAG:="-m64"
• BIT_VARIANT=64

-else ifeq ($(ARCH),arm)

• BIT_VARIANT=-arm

-else ifeq ($(ARCH),arm64)

• BIT_VARIANT=-arm64

-else ifeq ($(ARCH),aarch64)

• BIT_VARIANT=-arm64

-else ifeq ($(ARCH),aarch32)

• BIT_VARIANT=-arm

-endif

-MASTER_CC_FLAGS += $(MFLAG) -MASTER_CPP_FLAGS += $(MFLAG) -MASTER_LINKER_FLAGS += $(MFLAG)

# export build flags export MASTER_CC_FLAGS export MASTER_CPP_FLAGS @@ -120,7 +99,7 @@ export MASTER_LINKER_FLAGS export MASTER_LIB_FLAGS

# target directories -export PLAT_DIR=linux$(BIT_VARIANT)/$(BUILD_VARIANT) +export PLAT_DIR=builddir export LIB_TARGET_DIR=$(OCSD_LIB_ROOT)/$(PLAT_DIR) export LIB_TEST_TARGET_DIR=$(OCSD_TESTS)/lib/$(PLAT_DIR) export BIN_TEST_TARGET_DIR=$(OCSD_TESTS)/bin/$(PLAT_DIR)

I'm in agreement with the proposed changes - Mike please review on your side.

Thanks, Mathieu

...

All these patches included as files too.

Wookey

Principal hats: Linaro, Debian, Wookware, ARM http://wookware.org/

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

On 14 March 2018 at 23:03, Wookey wookey@wookware.org wrote:

On 2018-03-13 16:19 +0000, Wookey wrote:

...

On 2018-03-08 05:18 +0000, Wookey wrote: (this is not working quite right, so I'll send this mail now, as relevant to the discussion or the first set of patches, and a finished patch later)

OK. Attached is the patch. As well as adding the SONAME setting, and installing, it sets out the actual dependencies for the links and libraries, using order-only dependencies for the directories. This is -j safe, unlike using mkdir -p, and avoids using a sentinel file for each dir (which would also work if you prefer that for some reason).

I've set it to use the release version, rather than 0.0.0, assuming that you want to go with that. (I guess I should move on to 0.8.1 before uploading this?)

What would be the rationale for moving to 0.8.1? Or perhaps I missed something...

No doubt this could be improved, but it is now at least reasonably correct and only builds things that need building.

I note that the changed sections for the two submakefiles in rctdl_c_api_lib and ref_trace_decode_lib only differ by one word (the rctdl_c_api_lib shared lib build has $(SO_LIB_DEPS) where ref_trace_decode_lib doesn't). I do wonder if there is room for some merging to only have one set of rules for all that?

Index: libopencsd-0.8.0/decoder/build/linux/makefile

--- libopencsd-0.8.0.orig/decoder/build/linux/makefile +++ libopencsd-0.8.0/decoder/build/linux/makefile @@ -104,6 +104,13 @@ export LIB_TARGET_DIR=$(OCSD_LIB_ROOT)/$ export LIB_TEST_TARGET_DIR=$(OCSD_TESTS)/lib/$(PLAT_DIR) export BIN_TEST_TARGET_DIR=$(OCSD_TESTS)/bin/$(PLAT_DIR)

+# verion +export SO_MAJOR_VER = 0 +SO_MINOR_VER = 8 +SO_PATCH_VER = 0 +export SO_VER = $(SO_MAJOR_VER).$(SO_MINOR_VER).$(SO_PATCH_VER)

########################################################### # build targets

@@ -113,8 +120,12 @@ libs: $(LIB_BASE_NAME)_lib $(LIB_CAPI_N

install: libs mkdir -p $(INSTALL_LIB_DIR) $(INSTALL_INCLUDE_DIR)

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so $(INSTALL_LIB_DIR)/
  

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so $(INSTALL_LIB_DIR)/
  

•   cp -d $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so $(INSTALL_LIB_DIR)/
  

•   cp -d $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so.$(SO_MAJOR_VER) $(INSTALL_LIB_DIR)/
  

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so.$(SO_VER) $(INSTALL_LIB_DIR)/
  

•   cp -d $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so $(INSTALL_LIB_DIR)/
  

•   cp -d $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so.$(SO_MAJOR_VER) $(INSTALL_LIB_DIR)/
  

•   $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so.$(SO_VER) $(INSTALL_LIB_DIR)/
    $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).a $(INSTALL_LIB_DIR)/
    $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).a $(INSTALL_LIB_DIR)/
    cd $(OCSD_ROOT)/build/linux/rctdl_c_api_lib && make install_inc
  

Index: libopencsd-0.8.0/decoder/build/linux/rctdl_c_api_lib/makefile

--- libopencsd-0.8.0.orig/decoder/build/linux/rctdl_c_api_lib/makefile +++ libopencsd-0.8.0/decoder/build/linux/rctdl_c_api_lib/makefile @@ -55,13 +55,31 @@ OBJECTS=$(BUILD_DIR)/ocsd_c_api.o \ INST_INC_SRC=$(OCSD_INCLUDE)/$(LIB_UAPI_INC_DIR) INST_INC_DST=$(INSTALL_INCLUDE_DIR)/$(LIB_UAPI_INC_DIR)

-all: build_dir $(OBJECTS)

•   mkdir -p $(LIB_TARGET_DIR)
  

+all: links

+links: $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_MAJOR_VER) $(LIB_TARGET_DIR)/$(LIB_NAME).so +.PHONY: links

+LIBS:= $(LIB_TARGET_DIR)/$(LIB_NAME).a $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_VER)

+$(LIB_TARGET_DIR):

•   mkdir $(LIB_TARGET_DIR)
  

+$(BUILD_DIR):

•   mkdir $(BUILD_DIR)
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).a: $(OBJECTS) | $(BUILD_DIR) $(LIB_TARGET_DIR) $(LIB) $(LIB_FLAGS) $(LIB_TARGET_DIR)/$(LIB_NAME).a $(OBJECTS)

•   $(LINKER) $(LINKER_FLAGS) -o $(LIB_TARGET_DIR)/$(LIB_NAME).so -Wl,-soname,$(LIB_NAME).so $(OBJECTS) $(SO_LIB_DEPS)
  
  

-build_dir:

•   mkdir -p $(BUILD_DIR)
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_VER): $(OBJECTS) | $(BUILD_DIR) $(LIB_TARGET_DIR)

•   $(LINKER) $(LINKER_FLAGS) -o $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_VER) -Wl,-soname,$(LIB_NAME).so.$(SO_MAJOR_VER) $(OBJECTS) $(SO_LIB_DEPS)
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_MAJOR_VER): $(LIBS) | $(LIB_TARGET_DIR)

•   ( cd $(LIB_TARGET_DIR); ln -s $(LIB_NAME).so.$(SO_VER) $(LIB_NAME).so.$(SO_MAJOR_VER) )
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).so: $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_MAJOR_VER) | $(LIB_TARGET_DIR)

•   ( cd $(LIB_TARGET_DIR); ln -s $(LIB_NAME).so.$(SO_MAJOR_VER) $(LIB_NAME).so )
  

##### build rules

@@ -81,7 +99,7 @@ clean: rm -f $(OBJECTS) rm -f $(DEPS) rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).a

•   rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).so
  

•   rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).so*
  
  

#### install the necessary include files for the c-api library on linux install_inc: Index: libopencsd-0.8.0/decoder/build/linux/ref_trace_decode_lib/makefile =================================================================== --- libopencsd-0.8.0.orig/decoder/build/linux/ref_trace_decode_lib/makefile +++ libopencsd-0.8.0/decoder/build/linux/ref_trace_decode_lib/makefile @@ -114,13 +114,31 @@ OBJECTS=$(BUILD_DIR)/ocsd_code_follower. $(PTMOBJ) \ $(PKTPRNTOBJ)

-all: build_dir $(OBJECTS)

•   mkdir -p $(LIB_TARGET_DIR)
  

+all: links

+links: $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_MAJOR_VER) $(LIB_TARGET_DIR)/$(LIB_NAME).so +.PHONY: links

+LIBS:= $(LIB_TARGET_DIR)/$(LIB_NAME).a $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_VER)

+$(LIB_TARGET_DIR):

•   mkdir $(LIB_TARGET_DIR)
  

+$(BUILD_DIR):

•   mkdir $(BUILD_DIR)
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).a: $(OBJECTS) | $(BUILD_DIR) $(LIB_TARGET_DIR) $(LIB) $(LIB_FLAGS) $(LIB_TARGET_DIR)/$(LIB_NAME).a $(OBJECTS)

•   $(LINKER) $(LINKER_FLAGS) -o $(LIB_TARGET_DIR)/$(LIB_NAME).so -Wl,-soname,$(LIB_NAME).so $(OBJECTS)
  
  

-build_dir:

•   mkdir -p $(BUILD_DIR)
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_VER): $(OBJECTS) | $(BUILD_DIR) $(LIB_TARGET_DIR)

•   $(LINKER) $(LINKER_FLAGS) -o $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_VER) -Wl,-soname,$(LIB_NAME).so.$(SO_MAJOR_VER) $(OBJECTS)
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_MAJOR_VER): $(LIBS) | $(LIB_TARGET_DIR)

•   ( cd $(LIB_TARGET_DIR); ln -s $(LIB_NAME).so.$(SO_VER) $(LIB_NAME).so.$(SO_MAJOR_VER) )
  

+$(LIB_TARGET_DIR)/$(LIB_NAME).so: $(LIB_TARGET_DIR)/$(LIB_NAME).so.$(SO_MAJOR_VER) | $(LIB_TARGET_DIR)

•   ( cd $(LIB_TARGET_DIR); ln -s $(LIB_NAME).so.$(SO_MAJOR_VER) $(LIB_NAME).so )
  

##### build rules

@@ -140,4 +158,4 @@ clean: rm -f $(OBJECTS) rm -f $(DEPS) rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).a

•   rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).so
  

•   rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).so*
  
  
  
  

I'll look at the flags next.

Wookey

Principal hats: Linaro, Debian, Wookware, ARM http://wookware.org/

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

On 2018-03-08 05:18 +0000, Wookey wrote:

I'll post my issues/changes here for discussion so we can decide what is upstreamable.

TESTS

it's very easy to build the tests: make -f $(CURDIR)/decoder/build/linux/makefile tests

but I'm not sure what, if anything, to do with them. I see no machinery for running the tests, just an explanation of how to do it by hand.

Is there any point packaging the test programs (are they useful at development time?) or are they purely for testing that the code works?

If the latter then they should ideally be run at build-time to check that the built code does indeed do what was intended.

Alternatively if they are not really designed as build-time tests, then they can be run as 'autopackage tests'. This is a (debian-based distro) mechanism for running tests on the installed package, like building a test program against it.

I'm not quite sure how best to proceed here.

Firstly, we can just uploaded the package as-is without worrying about the tests. Testing is optional (but if we run them at build-time they will need to pass on all arches).

Secondly for the existing test to be useful as build-time tests we'd need to do something to check the output against the expected output. Not sure how easy that is (does it change on different arches at all?)

It seems that the current test stuff sets up a local test environment with local copies of the library. It might make more sense to run these as autopackage tests, which run these tests on an installed copy of the library, but I think that means including the test programs in the package. I'm not sure about that as I've never actually set up an autopackage test - I think you can arrange to use to use build-time artifacts and source artificats as well as packaged artifacts, but I'll need to check.

Log/ppl files are generated by the test programs, but I don't see 'correct' test-case copies of those files in the source. Presumably a pair of thesefor each snapshot would work, assuming that they don't vary too much with architecture/date/whatever? (The path variance noted below could be a pain).

It does feel like we should do something with the tests as the great bulk of the package source is lots of snapshot files for testing. it's currently 25MB of test files and 2MB of package (unpacked).

Running ./c_api_pkt_print_test fails, because the hardcoded path goes one level too far back up: decoder/tests/bin/builddir$ ./c_api_pkt_print_test Unable to open file ../../../snapshots/juno_r1_1/cstrace.bin to process trace data

Running it from a temp directory one layer down works:

(Ah yes, this used to be correct, (and I guess remains so upstream) because of your two-level platform build-dir naming <arch-bits>/{rel,dbg})

decoder/tests/bin/builddir/tmp/$ ../c_api_pkt_print_test Library Version 0.8.0

../c_api_pkt_print_test

Idx:86; I_NOT_SYNC : I Stream not synchronised Idx:1650; I_ASYNC : Alignment Synchronisation. Idx:1662; I_TRACE_INFO : Trace Info.; INFO=0x0 ...

This makes it work: Index: libopencsd-0.8.0/decoder/tests/source/c_api_pkt_print_test.c =================================================================== --- libopencsd-0.8.0.orig/decoder/tests/source/c_api_pkt_print_test.c +++ libopencsd-0.8.0/decoder/tests/source/c_api_pkt_print_test.c @@ -63,11 +63,11 @@

/* path to test snapshots, relative to tests/bin/<plat>/<dbg|rel> build output dir */ #ifdef _WIN32 -const char *default_path_to_snapshot = "..\..\..\snapshots\juno_r1_1\"; -const char *tc2_snapshot = "..\..\..\snapshots\TC2\"; +const char *default_path_to_snapshot = "..\..\snapshots\juno_r1_1\"; +const char *tc2_snapshot = "..\..\snapshots\TC2\"; #else -const char *default_path_to_snapshot = "../../../snapshots/juno_r1_1/"; -const char *tc2_snapshot = "../../../snapshots/TC2/"; +const char *default_path_to_snapshot = "../../snapshots/juno_r1_1/"; +const char *tc2_snapshot = "../../snapshots/TC2/"; #endif

/* trace data and memory file dump names and values - taken from snapshot metadata */

Similarly, adjusting the example in the docs for both linux path separators and relative depth, this works: ./trc_pkt_lister -ss_dir ../../snapshots/TC2 -o_raw_unpacked

So, what do you think is best to do about this from an

a) upstream point of view (build-time tests generally a good idea) b) distro point of view (seems like installed-package autopackagtsts might make more sense here). We can of course have both.

Currently I am inclined (for the Debian package) to include that path fix, and adjust the docs correspondingly, and build the tests, but will not wait for a run/check-the-tests implementation of any sort before uploading an initial package to debian. If we decide to do one it can be included in due course.

Wookey

On 2018-03-15 15:42 +0000, Wookey wrote:

On 2018-03-15 15:21 +0000, Mike Leach wrote:

...

Strictly speaking, the current version of OpenCSD is 0.8.1 (as of 26/01/2018), so when any patches for adjusting makefiles for packaging purposes are added in, then the release number will increment to 0.8.2.

Right. I'll update to 0.8.1 so at least my patches are uptodate. I started with 0.8.0 because something pointed me at it when I started, but there is no good reason for that SFAIK.

So you are OK with the idea that release versioning from here in will follow the GNU library compitibility rules, and thus we can use the release number for the library versioning, right? I thnk that's sensible.

OK. I'm working from 0.8.1 now.

I put in some slightly scary make/shell to fish the version numbers out of the version header file, so that it doesn't need maintaining in two places.

The whole updated soname patch is attached, but this is the changed bit: Index: libopencsd-0.8.1/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/makefile +++ libopencsd-0.8.1/decoder/build/linux/makefile @@ -104,6 +104,14 @@ export LIB_TARGET_DIR=$(OCSD_LIB_ROOT)/$ export LIB_TEST_TARGET_DIR=$(OCSD_TESTS)/lib/$(PLAT_DIR) export BIN_TEST_TARGET_DIR=$(OCSD_TESTS)/bin/$(PLAT_DIR)

+# Fish version out of header file (converting from hex) +getver:=printf "%d" $$(awk '/#define VARNAME/ { print $$3 }' $(OCSD_ROOT)/include/ocsd_if_version.h) +export SO_MAJOR_VER := $(shell $(subst VARNAME,OCSD_VER_MAJOR,$(getver))) +SO_MINOR_VER := $(shell $(subst VARNAME,OCSD_VER_MINOR,$(getver))) +SO_PATCH_VER := $(shell $(subst VARNAME,OCSD_VER_PATCH,$(getver))) +export SO_VER := $(SO_MAJOR_VER).$(SO_MINOR_VER).$(SO_PATCH_VER) + + ########################################################### # build targets

This is trickier than one would like because the numbers in the header file are C defines in hex (0x0), and need converting to unadorned decimal.

There are probably various ways of doing this. Above is the neatest I could come up with, but it's still a bit cryptic due to nestedness, escaping, and putting it into a function to avoid repeating it 3 times (and it took me a while to get right). So I'll explain.

awk '/#define OCSD_VER_MAJOR/ { print $3 }' $(OCSD_ROOT)/include/ocsd_if_version.h pulls the '0x0' number out of the file. printf "%d" 0x0 turns it into decimal. I can't see a way to pipe this, so two shell substitutions are used: awk reult into print command and print command into SO_ variable.

When the awk bit is put into a subshell the # needs escaping otherwise rest of line turns into a comment (and terminating bracket gets lost) Both the subshell call '$(' and the '$3' need the dollars escaping because this is make.

It's a bit ugly to repeat 3 times for different defines, so I put it in a 'getver' fn, then used $(subst to replace VARNAME for each define.

Wookey

On 2018-03-19 03:38 +0000, Wookey wrote:

Setting hardening flags for the build reveals a potential issue:

error: format not a string literal and no format arguments in decoder/tests/ext_dcd_test_eg/c_api_echo_test/ext_dcd_echo_test.c:333:20

This fixes it: Index: libopencsd-0.8.1/decoder/tests/ext_dcd_test_eg/c_api_echo_test/ext_dcd_echo_test.c =================================================================== --- libopencsd-0.8.1.orig/decoder/tests/ext_dcd_test_eg/c_api_echo_test/ext_dcd_echo_test.c +++ libopencsd-0.8.1/decoder/tests/ext_dcd_test_eg/c_api_echo_test/ext_dcd_echo_test.c @@ -330,7 +330,7 @@ void print_test_cov_results(echo_decoder if (coverage[TEST_COV_MSGLOG_CB] == TEST_RES_OK) /* check we can use the msg logger for outputting the results */ lib_cb_LogMsg(p_fns, OCSD_ERR_SEV_ERROR, coverage_message); else - printf(coverage_message); + printf("%s", coverage_message); } }

Wookey

On 2018-03-19 03:38 +0000, Wookey wrote:

On 2018-03-16 16:54 +0000, Mike Leach wrote: I'll do the 'mechanism to pass in flags' thing next.

OK. This is the simplest patch that allow passing-in of external flags (by defining the flags on the make command line):

Index: libopencsd-0.8.1/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/makefile +++ libopencsd-0.8.1/decoder/build/linux/makefile @@ -76,9 +76,12 @@ INSTALL_LIB_DIR=$(PREFIX)/lib/$(DEB_HOST export INSTALL_INCLUDE_DIR=$(PREFIX)/include

# compile flags -MASTER_CC_FLAGS := -c -Wall -DLINUX -MASTER_CXX_FLAGS := -c -Wall -DLINUX -std=c++11 -MASTER_LINKER_FLAGS := -Wl,-z,defs +CFLAGS += -c -Wall -DLINUX +CXXFLAGS += -c -Wall -DLINUX -std=c++11 +LDFLAGS += -Wl,-z,defs +MASTER_CC_FLAGS := $(CFLAGS) +MASTER_CXX_FLAGS := $(CXXFLAGS) +MASTER_LINKER_FLAGS := $(LDFLAGS) MASTER_LIB_FLAGS := rcs

# debug variant

The current flags used in various places are: CC_FLAGS (CFLAGS) := -c -Wall -DLINUX CXX_FLAGS (CXXFLAGS):= -c -Wall -DLINUX -std=c++11 LINKER_FLAGS (LDFLAGS) := -Wl,-z,defs LIB_FLAGS (ARFLAGS) := rcs

rctdl_c_api_lib: CXXFLAGS += -fpic -Wno-switch LDFLAGS += -shared

ref_trace_decode_lib: CXXFLAGS += -fpic -Wno-switch LDFLAGS += -shared

echo_test_dcd_lib: CFLAGS += -fpic -Wno-switch ARFLAGS

snapshot_parser_lib: CFLAGS += -fpic -Wno-switch -Wno-deprecated-declarations -Wno-unused-variable -Wno-reorder ARFLAGS

c_api_pkt_print_test: CFLAGS += -Wno-switch LDFLAGS += -Wl,--start-group $(LIBS) -Wl,--end-group

trc_pkt_lister: CXXFLAGS += -Wno-switch LDFLAGS

So that shows that -Wno-switch should be in the defaults -fpic is not used in two places, but in fact this is now the compiler default, at least in debian, and enabling it everywhere seems to be fine. Was there some reason why it was turned off for c_api_pkt_print_test and trc_pkt_lister?

LDFLAGS is used without -shared in two places, and those seem to make sense, so putting -shared on the 'link .so' lines, the special -Wl,--start-group in the one place it's needed and just using LDFLAGS default everywhere works fine.

So there is no reason not to use standard flag variable names thoughout and skip the MASTER->local copying. I've done this, using these defaults: CFLAGS := $(CPPFLAGS) -c -Wall -DLINUX -Wno-switch -fpic CXXFLAGS := $(CPPFLAGS) -c -Wall -DLINUX -Wno-switch -fpic -std=c++11 LDFLAGS := -Wl,-z,defs ARFLAGS := rcs

The attached patch makes these changes, allowing extra options to be passed in from outside.

One other thing. Should we enable --as-needed in the link? I've done this for the debian package but if it's a good idea it should go upstream.

Wookey

HI Wookey,

On 16 March 2018 at 18:54, Wookey wookey@wookware.org wrote:

On 2018-03-08 05:18 +0000, Wookey wrote:

...

I'll post my issues/changes here for discussion so we can decide what is upstreamable.

TESTS

it's very easy to build the tests: make -f $(CURDIR)/decoder/build/linux/makefile tests

but I'm not sure what, if anything, to do with them. I see no machinery for running the tests, just an explanation of how to do it by hand.

The tests as they stand are largely manual at present.

Is there any point packaging the test programs (are they useful at development time?) or are they purely for testing that the code works?

The test programs can be used at development time - but the trc_pkt_lister is also a handy utility to examine captured trace. Snapshots can be generated by other tools. The coresight access library(CSAL) - also on github - is one example.

c_api_pkt_print test was developed purely as a check that the C-API interfaces worked - so is less sophisticated in terms of command line options - hence the hard-coded paths you found.

If the latter then they should ideally be run at build-time to check that the built code does indeed do what was intended.

Alternatively if they are not really designed as build-time tests, then they can be run as 'autopackage tests'. This is a (debian-based distro) mechanism for running tests on the installed package, like building a test program against it.

I'm not quite sure how best to proceed here.

Firstly, we can just uploaded the package as-is without worrying about the tests. Testing is optional (but if we run them at build-time they will need to pass on all arches).

Secondly for the existing test to be useful as build-time tests we'd need to do something to check the output against the expected output. Not sure how easy that is (does it change on different arches at all?)

I am working on some updates to create a library self test mechanism that is not reliant on the previously captured snapshot data. That work is probably a couple of months off at best. Once this is ready then the tests will be suitable for run after build.

It seems that the current test stuff sets up a local test environment with local copies of the library. It might make more sense to run these as autopackage tests, which run these tests on an installed copy of the library, but I think that means including the test programs in the package. I'm not sure about that as I've never actually set up an autopackage test - I think you can arrange to use to use build-time artifacts and source artificats as well as packaged artifacts, but I'll need to check.

That is really a result of the project initially not having a make install - plus it ensures that debug tests and libraries are run together when gdb/ddd is used to step through code when debugging.

Log/ppl files are generated by the test programs, but I don't see 'correct' test-case copies of those files in the source. Presumably a pair of thesefor each snapshot would work, assuming that they don't vary too much with architecture/date/whatever? (The path variance noted below could be a pain).

At one point it was intended to have some "correct" versions of the logs - the manual testing is currently to cast an experienced eye over the first couple of pages of data, and initially to manually compare with trace dumps from another decode - e..g the one in ARM's debug tool DS-5. Otherwise I can also check out a previous release to generate a before and after.

The problem with the dumps, is that the amount of data makes it impossible to check every line, so while I believe the decode is all correct, we have no real way of double checking this - hence the change of direction to create self tests where known packets will be generated to push though the library and then check for the results.

It does feel like we should do something with the tests as the great bulk of the package source is lots of snapshot files for testing. it's currently 25MB of test files and 2MB of package (unpacked).

Running ./c_api_pkt_print_test fails, because the hardcoded path goes one level too far back up: decoder/tests/bin/builddir$ ./c_api_pkt_print_test Unable to open file ../../../snapshots/juno_r1_1/cstrace.bin to process trace data

Running it from a temp directory one layer down works:

(Ah yes, this used to be correct, (and I guess remains so upstream) because of your two-level platform build-dir naming <arch-bits>/{rel,dbg})

decoder/tests/bin/builddir/tmp/$ ../c_api_pkt_print_test Library Version 0.8.0

../c_api_pkt_print_test

Idx:86; I_NOT_SYNC : I Stream not synchronised Idx:1650; I_ASYNC : Alignment Synchronisation. Idx:1662; I_TRACE_INFO : Trace Info.; INFO=0x0 ...

This makes it work: Index: libopencsd-0.8.0/decoder/tests/source/c_api_pkt_print_test.c =================================================================== --- libopencsd-0.8.0.orig/decoder/tests/source/c_api_pkt_print_test.c +++ libopencsd-0.8.0/decoder/tests/source/c_api_pkt_print_test.c @@ -63,11 +63,11 @@

/* path to test snapshots, relative to tests/bin/<plat>/<dbg|rel> build output dir */ #ifdef _WIN32 -const char *default_path_to_snapshot = "..\..\..\snapshots\juno_r1_1\"; -const char *tc2_snapshot = "..\..\..\snapshots\TC2\"; +const char *default_path_to_snapshot = "..\..\snapshots\juno_r1_1\"; +const char *tc2_snapshot = "..\..\snapshots\TC2\"; #else -const char *default_path_to_snapshot = "../../../snapshots/juno_r1_1/"; -const char *tc2_snapshot = "../../../snapshots/TC2/"; +const char *default_path_to_snapshot = "../../snapshots/juno_r1_1/"; +const char *tc2_snapshot = "../../snapshots/TC2/"; #endif

/* trace data and memory file dump names and values - taken from snapshot metadata */

Similarly, adjusting the example in the docs for both linux path separators and relative depth, this works: ./trc_pkt_lister -ss_dir ../../snapshots/TC2 -o_raw_unpacked

So, what do you think is best to do about this from an

a) upstream point of view (build-time tests generally a good idea) b) distro point of view (seems like installed-package autopackagtsts might make more sense here). We can of course have both.

Currently I am inclined (for the Debian package) to include that path fix, and adjust the docs correspondingly, and build the tests, but will not wait for a run/check-the-tests implementation of any sort before uploading an initial package to debian. If we decide to do one it can be included in due course.

I'd be fine with this. I would also be fine with only including the snapshot that is directly referenced in the C-API program. The other snapshots will always be available on Github.

Mike

Wookey

Principal hats: Linaro, Debian, Wookware, ARM http://wookware.org/

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

On 2018-03-20 04:27 +0000, Wookey wrote:

On 2018-03-08 05:18 +0000, Wookey wrote:

...

I'll post my issues/changes here for discussion so we can decide what is upstreamable.

It's conventional to be able to specify $PREFIX for where things get installed to.

A closely related matter is the need to specify the install path for libraries. This is particularly used in debian-dervied distros to support multiarch (the co-installability of libraries and headers to support cross-building and 32/64bit support, amongst other things).

It's a simple patch but it conflicts with this one rather so both are included.

This is used in the debian packaging to install librries into /usr/lib/<triplet>. (or more accurately $(PREFIX)/lib/<triplet>

Recognising that the upstream default remains just to put things in /usr/lib the easilest way to deal with this is just to make it simple to override the library install path at build time.

Index: libopencsd-0.8.1/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/makefile +++ libopencsd-0.8.1/decoder/build/linux/makefile @@ -71,7 +71,8 @@ export MASTER_LIB=$(CROSS_COMPILE)ar export INSTALL=install

# installation directory -INSTALL_LIB_DIR=/usr/lib/ +LIB_PATH ?= lib +INSTALL_LIB_DIR=/usr/$(LIB_PATH) export INSTALL_INCLUDE_DIR=/usr/include/

# compile flags

Wookey

On 2018-03-22 12:41 +0000, Wookey wrote:

On 2018-03-22 08:38 +0000, Mike Leach wrote:

...

HI Wookey,

The test programs can be used at development time - but the trc_pkt_lister is also a handy utility to examine captured trace.

OK. I'll include that in the package then.

To do that it really needs to be installed, so here's a patch to do that.

I've ended up putting this in its own package (libopencsd-bin), which is a little excessive, but having binaries (as opposed to libraries) in libfoo-dev packages breaks multiarch, and we like things to be crossbuildable, especially kernel-related tools, so I think this makes sense. And more tools might turn up in the future?

I realise that all these patches to the main makefile tend to affect each other, so you've now got a pile of patches where the order matters and they needs refreshing as new ones arrive. I have just about finished messing now, and you are probably best off getting a consistent set from: http://wookware.org/software/repo/pool/main/libo/libopencsd/libopencsd_0.8.1... if piecing the changes together from this thread gets tiresome.

Index: libopencsd-0.8.1/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/makefile +++ libopencsd-0.8.1/decoder/build/linux/makefile @@ -74,6 +74,7 @@ export INSTALL=install PREFIX ?=/usr LIB_PATH ?= lib INSTALL_LIB_DIR=$(PREFIX)/$(LIB_PATH) +INSTALL_BIN_DIR=$(PREFIX)/bin export INSTALL_INCLUDE_DIR=$(PREFIX)/include/

# compile flags @@ -134,13 +135,15 @@ all: libs tests

libs: $(LIB_BASE_NAME)_lib $(LIB_CAPI_NAME)_lib

-install: libs - mkdir -p $(INSTALL_LIB_DIR) $(INSTALL_INCLUDE_DIR) +install: libs tests + mkdir -p $(INSTALL_LIB_DIR) $(INSTALL_INCLUDE_DIR) $(INSTALL_BIN_DIR) $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).so $(INSTALL_LIB_DIR)/ $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).so $(INSTALL_LIB_DIR)/ $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_BASE_NAME).a $(INSTALL_LIB_DIR)/ $(INSTALL) --mode=644 $(LIB_TARGET_DIR)/lib$(LIB_CAPI_NAME).a $(INSTALL_LIB_DIR)/ cd $(OCSD_ROOT)/build/linux/rctdl_c_api_lib && make install_inc + $(INSTALL) --mode=755 $(BIN_TEST_TARGET_DIR)/trc_pkt_lister $(INSTALL_BIN_DIR)/ +

################################ # build OpenCSD trace decode library

Wookey

Hi Wookey,

Not added the above yet as there doesn't appear to be an actual attached .patch file, and it does appear that the patch may remove the reliance on including DEPS - which I think is required to ensure that we only build changed files.

However, the previous patches break the build in two fundamental ways.

1) running make from the build\linux directory results in awk failing and thus the entire build failing

awk: cannot open /disk-2/mleach/opencsd-master/decoder/build/linux/decoder/include/ocsd_if_version.h (No such file or directory) awk: cannot open /disk-2/mleach/opencsd-master/decoder/build/linux/decoder/include/ocsd_if_version.h (No such file or directory) awk: cannot open /disk-2/mleach/opencsd-master/decoder/build/linux/decoder/include/ocsd_if_version.h (No such file or directory) cd /disk-2/mleach/opencsd-master/decoder/build/linux/decoder/build/linux/ref_trace_decode_lib && make clean /bin/sh: 1: cd: can't cd to /disk-2/mleach/opencsd-master/decoder/build/linux/decoder/build/linux/ref_trace_decode_lib

2) editing a single source file and rebuilding (without clean so that _only_ the changed files are altered) make results in an error:-

( cd /disk-2/mleach/opencsd-master/decoder/lib/builddir; ln -s libopencsd.so.0.8.3 libopencsd.so.0 ) ln: failed to create symbolic link 'libopencsd.so.0': File exists makefile:133: recipe for target '/disk-2/mleach/opencsd-master/decoder/lib/builddir/libopencsd.so.0' failed make[1]: *** [/disk-2/mleach/opencsd-master/decoder/lib/builddir/libopencsd.so.0] Error 1

The whole point of the dependencies etc.is to allow incremental build and development. Thus this has to be fixed.

These really need to be fixed before I can release a patchset to upstream - so any suggestions welcome.

Regards

Mike

On 8 April 2018 at 14:46, Wookey wookey@wookware.org wrote:

On 2018-04-05 07:50 +0000, Mike Leach wrote:

...

On 2018-03-22 02:59 +0000, Wookey wrote:

...

...

The .d problem:

...

...

My understanding is that because of the -MMD option the .d files should be built alongside the .o files as part of the compilation. Why might that not happen?

Not sure - that is my understanding too.

Not managed to reproduce this yet - but will revisit as soon as I get back from vacation (Mon 9th).

I had a proper peer at the entrails and eventually worked out how to reproduce this. It's the presence or otherwise of the decoder/build/linux/ref_trace_decode_lib/builddir build directory.

That needs to exist _before_ the first g++ command otherwise it fails, confusingly talking about the .d file it can't read, when really the problem is that it didn't _create_ it.

The fix is simple enough: add this dependency. This was an ommission from the rearangement of the dependcies in the sonames patch.

And it wasn't revealed (and thus much more obvious) because those two sub-builddirs weren't getting cleaned, so once they existed that source tree would always build.

This patch fixes both issues. Index: libopencsd-0.8.1/decoder/build/linux/rctdl_c_api_lib/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/rctdl_c_api_lib/makefile +++ libopencsd-0.8.1/decoder/build/linux/rctdl_c_api_lib/makefile @@ -85,7 +85,7 @@ DEPS := $(OBJECTS:%.o=%.d) -include $(DEPS)

## object compile -$(BUILD_DIR)/%.o : %.cpp +$(BUILD_DIR)/%.o : %.cpp | $(BUILD_DIR) $(CXX) $(CXXFLAGS) $(CXX_INCLUDES) -MMD $< -o $@

@@ -96,6 +96,7 @@ clean: rm -f $(DEPS) rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).a rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).so*

•   -rmdir $(BUILD_DIR)
  
  

#### install the necessary include files for the c-api library on linux install_inc: Index: libopencsd-0.8.1/decoder/build/linux/ref_trace_decode_lib/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/ref_trace_decode_lib/makefile +++ libopencsd-0.8.1/decoder/build/linux/ref_trace_decode_lib/makefile @@ -144,7 +144,7 @@ DEPS := $(OBJECTS:%.o=%.d) -include $(DEPS)

## object compile -$(BUILD_DIR)/%.o : %.cpp +$(BUILD_DIR)/%.o : %.cpp | $(BUILD_DIR) $(CXX) $(CXXFLAGS) $(CXX_INCLUDES) -MMD $< -o $@

@@ -155,3 +155,4 @@ clean: rm -f $(DEPS) rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).a rm -f $(LIB_TARGET_DIR)/$(LIB_NAME).so*

•   -rmdir $(BUILD_DIR)
  
  
  
  
  

Wookey

Principal hats: Linaro, Debian, Wookware, ARM http://wookware.org/

---

CoreSight mailing list CoreSight@lists.linaro.org https://lists.linaro.org/mailman/listinfo/coresight

On 2018-04-11 23:13 +0100, Mike Leach wrote:

Hi Wookey,

I've attached an updated patchset - largely based on your patches, adjusted to fix the above issues, and includes a development makefile.dev that retains the original arch/dbg|rel directory structure, but allows the master makefile to have the single builddir needed for the Debian distro. Other than that it should be largely what you developed over the past couple of weeks.

Thanks muhcly for all that. Looks good.

Should apply to 0.8.1

It does.

The final upstream patchset will contain additional stuff (some expanded user docs and a couple of source fixes that have been made since 0.8.1), but if you could check that this set doesn't break the packaging build, then we will be almost there.

OK, I need a one-line patch to make it work, and there are a few other minor things which don't break the build but could be better:

Vital fix: Index: libopencsd-0.8.1/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.1.orig/decoder/build/linux/makefile +++ libopencsd-0.8.1/decoder/build/linux/makefile @@ -35,7 +35,7 @@

# Set project root - relative to build directory ifeq ($(OCSD_ROOT),) -OCSD_ROOT := $(shell pwd | sed 's,/build/linux.*,,') +OCSD_ROOT := $(CURDIR)/decoder export OCSD_ROOT endif

without that all the paths are one directory too high: cd /home/wookey/packages/libopencsd/debian/libopencsd-0.8.3pre.test/build/linux/ref_trace_decode_lib && make clean (instead of: cd /home/wookey/packages/libopencsd/debian/libopencsd-0.8.3pre.test/decoder/build/linux/ref_trace_decode_lib && make clean (and nothing builds)

I don't quite understand why what you have works upstream, but it must be something to do with using relative paths, and your platform dirs having two levels? There is probably a way to do this that works with both one and two-level platform dirnames and both relative and absolute paths. It just feels like "$(shell pwd | sed 's,/build/linux.*,,')" is making unwarranted assumptions about the layout. Is "$(CURDIR)/decoder" ever wrong for your purposes? Is there a reason you need to use relative paths?

I possibly never actually sent you this snippet of the patchset before - sorry about that.

Also the doc build is not getting cleaned: -clean: clean_libs clean_tests +clean: clean_libs clean_tests clean_docs

Even if you don't do the doc build by default, cleaning it doesn't do any harm, but I guess you see a warning and that was unpleasing so you removed it? If so I guess the clean way to do this is have a BUILD_DOCS flag to pass in which does and build and clean if set, but neither if not?

everything else looks sensible, except I'd do: ( cd $(LIB_TARGET_DIR); ln -sf $(LIB_NAME).so.$(SO_VER) $(LIB_NAME).so.$(SO_MAJOR_VER) )

rather than ( cd $(LIB_TARGET_DIR); rm -f $(LIB_NAME).so.$(SO_MAJOR_VER); ln -s $(LIB_NAME).so.$(SO_VER) $(LIB_NAME).so.$(SO_MAJOR_VER) )

(smae pattern on 3 lines in: decoder/build/linux/rctdl_c_api_lib/makefile decoder/build/linux/ref_trace_decode_lib/makefile

(I can see why you need that for non-clean rebuilds).

And finally there is trailing whitespace on: decoder/tests/build/linux/c_cpi_pkt_print_test/makefile:82 decoder/tests/build/linux/echo_test_dcd_lib/makefile:74 decoder/tests/build/linux/trc_pkt_lister/makefile:36

I need to dash now so I haven't got time to make you better suggested patches for 'BUILD_DOCS' of the OCSD_ROOT thing, but can have a look next week if you don;t beat me to it.

Wookey

On 2018-04-13 10:18 +0100, Mike Leach wrote:

...

...

The final upstream patchset will contain additional stuff (some expanded user docs and a couple of source fixes that have been made since 0.8.1), but if you could check that this set doesn't break the packaging build, then we will be almost there.

Whilst checking everything was as it should be, I noticed one more minor issue: The tests buildir (LIB_TEST_TARGET_DIR) is not cleaned up:

It could be fixed in this rather crude way in the main makefile:

Index: libopencsd-0.8.3~pre/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.3~pre.orig/decoder/build/linux/makefile +++ libopencsd-0.8.3~pre/decoder/build/linux/makefile @@ -187,6 +187,7 @@ clean_tests: cd $(OCSD_ROOT)/tests/build/linux/snapshot_parser_lib && make clean cd $(OCSD_ROOT)/tests/build/linux/trc_pkt_lister && make clean cd $(OCSD_ROOT)/tests/build/linux/c_api_pkt_print_test && make clean + -rm -r $(OCSD_TESTS)/lib

clean_docs: -rm -r $(OCSD_ROOT)/docs/html

But this is probably a more 'correct/modular' fix:

Index: libopencsd-0.8.3~pre/decoder/tests/build/linux/echo_test_dcd_lib/makefile =================================================================== --- libopencsd-0.8.3~pre.orig/decoder/tests/build/linux/echo_test_dcd_lib/makefile +++ libopencsd-0.8.3~pre/decoder/tests/build/linux/echo_test_dcd_lib/makefile @@ -79,6 +79,6 @@ clean: -rm $(OBJECTS) -rm $(LIB_TEST_TARGET_DIR)/$(LIB_NAME).a -rm $(DEPS) - -rmdir $(BUILD_DIR) + -rmdir $(BUILD_DIR) $(LIB_TEST_TARGET_DIR)

# end of file makefile Index: libopencsd-0.8.3~pre/decoder/tests/build/linux/snapshot_parser_lib/makefile =================================================================== --- libopencsd-0.8.3~pre.orig/decoder/tests/build/linux/snapshot_parser_lib/makefile +++ libopencsd-0.8.3~pre/decoder/tests/build/linux/snapshot_parser_lib/makefile @@ -89,4 +89,4 @@ clean: -rm $(OBJECTS) -rm $(DEPS) -rm $(LIB_TEST_TARGET_DIR)/$(LIB_NAME).a - -rmdir $(BUILD_DIR) + -rmdir $(BUILD_DIR) $(LIB_TEST_TARGET_DIR) Index: libopencsd-0.8.3~pre/decoder/build/linux/makefile =================================================================== --- libopencsd-0.8.3~pre.orig/decoder/build/linux/makefile +++ libopencsd-0.8.3~pre/decoder/build/linux/makefile @@ -187,6 +187,7 @@ clean_tests: cd $(OCSD_ROOT)/tests/build/linux/snapshot_parser_lib && make clean cd $(OCSD_ROOT)/tests/build/linux/trc_pkt_lister && make clean cd $(OCSD_ROOT)/tests/build/linux/c_api_pkt_print_test && make clean + -rmdir $(OCSD_TESTS)/lib

clean_docs: -rm -r $(OCSD_ROOT)/docs/html

Wookey