cms-gem-daq-project / ctp7_modules

0 stars 13 forks source link

[feature] templated RPC + package structure #167

Closed jsturdy closed 4 years ago

jsturdy commented 4 years ago

Description

Supersedes #164, includes all modules migrated as well as consistency updates to the package structure.

Types of changes

Motivation and Context

164

How Has This Been Tested?

Compiles for arm, lacks x86_64 due to #166

jsturdy commented 4 years ago

Many comments which are not a good reason to postpone this PR. However, I cannot compile it seamlessly on gemdaq-build-xdaq15. This is possibly an issue with the VM, but I cannot really approve the PR if I cannot successfully compile the code. :-/

I have actually never tested this on the xdaq15 VM because there is no explicit dependency on xdaq15 (the only package which has that is cmsgemos, everything else simply reuses xdaq provided packages, e.g., log4cplus, and should be able to compile regardless of the provided version)

So, related to build system, here is a list a issues/flaws I encountered:

  • The build target is not defined, while it existed in this repository and still exists in cmsgemos.

build still exists for a specific target, e.g.,

TargetArch=arm make build -f ctp7_modules.mk
  • It is required to set the PETA_STAGE environment variable while I would have expected a sesible default value if the gem-peta-stage-ctp7 RPM is installed:
[lpetre@gemdaq-build-xdaq15 ctp7_modules]$ make
TargetArch=arm make -f ctp7_modules.mk build
Using BUILD_HOME=/afs/cern.ch/user/l/lpetre/GEM
OS Detected: centos7
BUILD_VERSION 1.0.462.dev
PREREL_VERSION -final.dev462
make[1]: Entering directory `/afs/cern.ch/user/l/lpetre/GEM/ctp7_modules'
/afs/cern.ch/user/l/lpetre/GEM/ctp7_modules/config/mfZynq.mk:2: *** "Error: PETA_STAGE environment variable not set.".  Stop.
make[1]: Leaving directory `/afs/cern.ch/user/l/lpetre/GEM/ctp7_modules'
make: *** [arm] Error 2

This is now "fixed", provided the fix (PETA_STAGE=$PETA_PATH/$BOARD) is robust enough to handle the potential targets

* Does compile on `gemdaq-build-xdaq15`:
[lpetre@gemdaq-build-xdaq15 ctp7_modules]$ make
TargetArch=arm make -f ctp7_modules.mk build
[...]
XHAL_ROOT /opt/xhal
BUILD_HOME /afs/cern.ch/user/l/lpetre/GEM
make[2]: Entering directory `/afs/cern.ch/user/l/lpetre/GEM/ctp7_modules'
mkdir -p arm/src/linux/arm
arm-linux-gnueabihf-g++ -fomit-frame-pointer -pipe -fno-common -fno-builtin -Wall -std=c++14 -march=armv7-a -mfpu=neon -mfloat-abi=hard -mthumb-interwork -mtune=cortex-a9 -DEMBED -Dlinux -D__linux__ -Dunix -fPIC --sysroot=/opt/gem-peta-stage/ctp7/ -pthread -DGEM_VARIANT="ge11" -std=c++14 -std=gnu++14 -g -O2 -c -Iinclude -I/opt/xhal/include -I/opt/wiscrpcsvc/include -I/opt/reedmuller/include -MT arm/src/linux/arm/memhub.o -MMD -MP -MF arm/src/linux/arm/memhub.Td -o arm/src/linux/arm/memhub.o src/memhub.cpp
cc1plus: fatal error: /opt/wiscrpcsvc/include/stdc-predef.h: Permission denied
[...]

I'm not sure why stdc-predef.h is searched for in /opt/wiscrpcsvc/include, but the directory seems to be in a bad state:

[lpetre@gemdaq-build-xdaq15 ctp7_modules]$ ll -a /opt/wiscrpcsvc/include/
ls: cannot access /opt/wiscrpcsvc/include/.: Permission denied
ls: cannot access /opt/wiscrpcsvc/include/..: Permission denied
ls: cannot access /opt/wiscrpcsvc/include/wiscRPCMsg.h: Permission denied
ls: cannot access /opt/wiscrpcsvc/include/wiscrpcsvc.h: Permission denied
total 0
d????????? ? ? ? ?            ? .
d????????? ? ? ? ?            ? ..
-????????? ? ? ? ?            ? wiscRPCMsg.h
-????????? ? ? ? ?            ? wiscrpcsvc.h

I need to check the RPM, the permissions are set wrong, probably because of my umask when building the RPM...

Again, I've never tested on that VM (I've only used gem904daq02 with locally installed versions of necessary dependencies (xhal, and reedmuller for the PETA_STAGE library, need to provide the equivalent reedmuller-peta-ctp7 package...) (note about doing this in a separate comment)

Also, more generally:

  • What is the purpose of passing integral types as constant references?

I think it was culled from your example

  • Shouldn't the Doxygen commands be changed from \ to @ for consistency with cmsgemos?

No change to the style was made to this codebase, I don't really have a strong preference, but if a future clang-tidy/clang-format could apply this globally, let me know and i'll add it to the style file i'm preparing (it will be added to gembuild along with a couple of targets to perform the actions).

  • Does this PR really targets develop? I would have expected it to target feature/templated-rpc-methods.

indeed, i had probably misread my graph and thought that feature/templated-rpc-methods was behind develop but fast-forwardable.

None of this is blocking, except that I did not manager to compile the PR on the dedicated VM.

jsturdy commented 4 years ago

N.B., for building against non-system installed packages, which have ARM libraries, you should do the following:

## install local xhal
INSTALL_PREFIX=/local/install/path make install
## install local reedmuller-c
INSTALL_PREFIX=/local/install/path make install
cd /local/install/path/opt/gem-peta-stage/ctp7
ln -s /opt/gem-peta-stage/ctp7/lib .
ln -s /opt/gem-peta-stage/ctp7/usr .
## do the same for other boards, if they exist
cd /path/to/ctp7_modules
export PETA_PATH=/opt/gem-peta-stage
make
## or make only a specific target
make arm
lpetre-ulb commented 4 years ago

I have actually never tested this on the xdaq15 VM because there is no explicit dependency on xdaq15 (the only package which has that is cmsgemos, everything else simply reuses xdaq provided packages, e.g., log4cplus, and should be able to compile regardless of the provided version)

I wrongly thought that all new developments were focusing towards xDAQ 15 and so were done on the VM (among others because of the log4cplus potential issue you cite).

  • It is required to set the PETA_STAGE environment variable while I would have expected a sesible default value if the gem-peta-stage-ctp7 RPM is installed:

This is now "fixed", provided the fix (PETA_STAGE=$PETA_PATH/$BOARD) is robust enough to handle the potential targets

:+1:

  • What is the purpose of passing integral types as constant references?

I think it was culled from your example

Right, copies are avoided in the templated RPC framework when references are used. However, this is useless for the simple types for which the reference accesses cost more than the copy itself.

  • Shouldn't the Doxygen commands be changed from \ to @ for consistency with cmsgemos?

No change to the style was made to this codebase, I don't really have a strong preference, but if a future clang-tidy/clang-format could apply this globally, let me know and i'll add it to the style file i'm preparing (it will be added to gembuild along with a couple of targets to perform the actions).

I don't have a strong preference either, I just remembered having seen the conversion being done somewhere. If there is a survey for \ vs @, I'd vote \ (not coming from Java world... :)

N.B., for building against non-system installed packages, which have ARM libraries, you should do the following: [...]

I also had to export XHAL_ROOT to point to the xhal directory in INSTALL_PREFIX, but thanks for the instructions! Will some documentation be available somewhere? The process to setup a local environment doesn't seem trivial (but it is already much simpler than what currently exists).

jsturdy commented 4 years ago

I also had to export XHAL_ROOT to point to the xhal directory in INSTALL_PREFIX, but thanks for the instructions! Will some documentation be available somewhere? The process to setup a local environment doesn't seem trivial (but it is already much simpler than what currently exists).

Ah, yeah, anything that is not system installed needs to be specified, as the defaults pick up the system versions. I think once all these things are merged in an updated doc can be provided, being based on the comment here