Adding loading macros validation #292
Conversation
Co-authored-by: Juan Antonio García <80903717+juanangp@users.noreply.github.com>
|
Perhaps we should also add a way to check if all macros are loaded? |
|
There are some errors from REST_Geant4_FindGammasEmitted.C and REST_Geant4_ViewEvent.C. Looks like some methods are renamed? @lobis |
PR rest-for-physics/geant4lib#63 should fix the errors. |
|
Great! This validation test already hunted an issue before even being merged! |
|
Finally pipeline is green, plz approve |
.github/workflows/validation.yml
Outdated
| export DISPLAY=localhost:0.0 | ||
| echo $GARFIELD_INSTALL | ||
| ls $GARFIELD_INSTALL | ||
| export ROOT_INCLUDE_PATH=$ROOT_INCLUDE_PATH:/usr/local/garfieldpp/install/Include |
There was a problem hiding this comment.
Probably the problem is that we are not adding the Garfield headers to
ROOT_INCLUDE_PATH
@jgalan ROOT_INCLUDE_PATH already points to the Garfield install on the docker image, you can check this on the image itself looking at the environment variables.
There was a problem hiding this comment.
For quick reference here are the env variables. I used /bin/sh as the shell to avoid loading variables from the .bashrc so I think this should be the exact same variables active when running the pipeline in a non-interactive shell:
# docker run -it --rm ghcr.io/lobis/root-geant4-garfield-dev:rest-for-physics /bin/sh
# printenv
GARFIELD_INSTALL=/usr/local/garfieldpp/install
G4INCLDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4INCL1.0
HOSTNAME=6d130f6981a5
LD_LIBRARY_PATH=/usr/local/geant4/install/lib:/usr/local/garfieldpp/install/lib:/usr/local/root/install/lib:
HOME=/root
G4PARTICLEXSDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4PARTICLEXS4.0
G4LEDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4EMLOW8.0
G4LEVELGAMMADATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/PhotonEvaporation5.7
G4RADIOACTIVEDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/RadioactiveDecay5.6
G4PIIDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4PII1.3
G4SAIDXSDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4SAIDDATA2.0
G4REALSURFACEDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/RealSurface2.2
GARFIELD_VERSION=eb1b5403
GEANT4_VERSION=v11.0.3
TERM=xterm
ROOT_VERSION=v6-26-06
ROOTSYS=/usr/local/root/install
G4ABLADATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4ABLA3.1
PATH=/usr/local/geant4/install/bin:/usr/local/root/install/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HEED_DATABASE=/usr/local/garfieldpp/install/share/Heed/database
G4NEUTRONHPDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4NDL4.6
DEBIAN_FRONTEND=noninteractive
ROOT_INCLUDE_PATH=/usr/local/garfieldpp/install/include/Garfield:/usr/local/garfieldpp/install/include:
G4ENSDFSTATEDATA=/usr/local/geant4/install/share/Geant4-11.0.3/data/G4ENSDFSTATE2.3
CMAKE_CXX_STANDARD=17
PWD=/
PYTHONPATH=/usr/local/root/install/lib:
TINI_VERSION=v0.19.0
CMAKE_PREFIX_PATH=/usr/local/garfieldpp/install:
There was a problem hiding this comment.
However, when you source thisREST.sh it will be overwritten by the ROOT_INCLUDE_PATH definition found there.
There was a problem hiding this comment.
If that was the problem, the commit 9005bb1 should solve it.
There was a problem hiding this comment.
It must be thisREST.sh who sets that up, so that the user does not need to care about setting any env variables
cmake/thisREST.cmake
Outdated
| @@ -84,7 +84,7 @@ fi | |||
|
|
|||
| export REST_SOURCE=${CMAKE_CURRENT_SOURCE_DIR} | |||
| export REST_PATH=\\\${thisdir} | |||
| export ROOT_INCLUDE_PATH=\\\${thisdir}/include:${garfieldDir}/include | |||
| export ROOT_INCLUDE_PATH=\\\$ROOT_INCLUDE_PATH:\\\${thisdir}/include | |||
There was a problem hiding this comment.
You need to define the garfield path in ROOT_INCLUDE_PATH. It will only work in your docker
There was a problem hiding this comment.
On the other hand, is better that we get control of the variables to avoid problems with user defined system variables
There was a problem hiding this comment.
For example, if you add :${ROOT_INCLUDE_PATH} behind as you did in next commits, you are hiding a problem in the validation pipeline, because you defined in your docker ROOT_INCLUDE_PATH externally, but this will not be the case for most of the users, so my opinion is that is better that we overwrite ROOT_INCLUDE_PATH.
Because the only scenario where this would cause problems is that someone else writes another ROOT library that has nothing to do with REST, and wants to use it. Ok, but I believe this will not be the case in the 99.9999%.
Somehow, we are conditioning the validation tests to the way you defined the variables inside the docker, which might not be the same for any user.
I agree with other env variables there is no other way, such as LD_LIBRARY_PATH
|
Should we approve and move to the next page? |
This PR adds a validation routine to check that macros are loaded without any "warning" or "error".