Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External hipo #63

Closed
wants to merge 22 commits into from
Closed

External hipo #63

wants to merge 22 commits into from

Conversation

dglazier
Copy link
Collaborator

Provisional changes for using external hipo

@c-dilks
Copy link
Member

c-dilks commented Feb 1, 2024

Here are HIPO builder steps you can adapt for your CI workflow:

      - name: checkout hipo
        uses: actions/checkout@v4
        with:
          repository: gavalian/hipo
          ref: 4.0.1 # if you want to use a fixed tag; remove if you want to use `main`
          path: hipo_src

      - name: build hipo
        run: |
          cmake -S hipo_src -B hipo_build --install-prefix $HIPO  # you might prefer a different prefix
          cmake --build hipo_build -j4
          cmake --install hipo_build

#include "utils.h"
#include "dictionary.h"
#include <hipo4/utils.h>
#include <hipo4/bank.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean dictionary.h here?

Suggested change
#include <hipo4/bank.h>
#include <hipo4/dictionary.h>

@@ -5,7 +5,7 @@
*/

#include "ftbparticle.h"
#include "dictionary.h"
#include <hipo4/bank.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <hipo4/bank.h>
#include <hipo4/dictionary.h>

#include "bank.h"
#include "dictionary.h"
#include <hipo4/bank.h>
#include <hipo4/bank.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <hipo4/bank.h>
#include <hipo4/dictionary.h>

@@ -5,7 +5,7 @@
*/

#include "particle.h"
#include "dictionary.h"
#include <hipo4/bank.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <hipo4/bank.h>
#include <hipo4/dictionary.h>

#include "reader.h"
#include "dictionary.h"
#include <hipo4/reader.h>
#include <hipo4/bank.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#include <hipo4/bank.h>
#include <hipo4/dictionary.h>

@@ -12,8 +12,6 @@ void LoadClas12Root(){

TString CLAS12ROOT=gSystem->Getenv("CLAS12ROOT");
TString LIB=CLAS12ROOT+"/lib/";
gSystem->Load(LIB+"liblz4");
gSystem->Load(LIB+"libHipo4");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will these library loads still be needed, regardless of whether or not they are externally built?

CMakeLists.txt Outdated
@@ -11,7 +11,7 @@ set(CMAKE_CXX_FLAGS "-fPIC -O3")

##########For local ROOT cmake files comment in the line below and remove the lines commented ##USEROOTSYS
#####include("cmake/FindROOT.cmake")
list(APPEND CMAKE_PREFIX_PATH $ENV{ROOTSYS}) ############USEROOTSYS
list(APPEND CMAKE_PREFIX_PATH $ENV{ROOTSYS} /home/dglazier/Dropbox/clas12/gagik/hipo) ############USEROOTSYS
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ENV{HIPO}? Or instruct users to set -DCMAKE_PREFIX_PATH?

CMakeLists.txt Outdated
Comment on lines 26 to 27
MESSAGE( STATUS "HIPO4 INCLUDE_DIR : " ${hipo4_INCLUDE_DIRS} )
MESSAGE( STATUS "HIPO4 LIBRARIES : " ${hipo4_LIBRARIES} )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These variables aren't needed since the PkgConfig::hipo4 target can be used.

Suggested change
MESSAGE( STATUS "HIPO4 INCLUDE_DIR : " ${hipo4_INCLUDE_DIRS} )
MESSAGE( STATUS "HIPO4 LIBRARIES : " ${hipo4_LIBRARIES} )

@c-dilks
Copy link
Member

c-dilks commented Feb 1, 2024

I also had to modify Clas12Root/src/CMakeLists.txt to include and link hipo4. Perhaps not all of the executables need it, but here's a change that got things working on my end:

--- a/Clas12Root/src/CMakeLists.txt
+++ b/Clas12Root/src/CMakeLists.txt
@@ -1,16 +1,20 @@
 set(CLAS12ROOT Clas12Root)

 add_executable (clas12root clas12root.cpp)
-target_link_libraries(clas12root  ${ROOT_LIBRARIES})
+target_link_libraries(clas12root  ${ROOT_LIBRARIES} PkgConfig::hipo4)
+target_include_directories(clas12root SYSTEM PUBLIC PkgConfig::hipo4)

 add_executable (clas12proof clas12proof.cpp)
-target_link_libraries(clas12proof ${ROOT_LIBRARIES})
+target_link_libraries(clas12proof ${ROOT_LIBRARIES} PkgConfig::hipo4)
+target_include_directories(clas12proof SYSTEM PUBLIC PkgConfig::hipo4)

 add_executable (particleDraw particleDraw.cpp)
-target_link_libraries(particleDraw ${ROOT_LIBRARIES})
+target_link_libraries(particleDraw ${ROOT_LIBRARIES} PkgConfig::hipo4)
+target_include_directories(particleDraw SYSTEM PUBLIC PkgConfig::hipo4)

 add_executable (particleTree particleTree.cpp)
-target_link_libraries(particleTree ${ROOT_LIBRARIES})
+target_link_libraries(particleTree ${ROOT_LIBRARIES} PkgConfig::hipo4)
+target_include_directories(particleTree SYSTEM PUBLIC PkgConfig::hipo4)
 install (TARGETS clas12root clas12proof particleDraw particleTree DESTINATION ${CMAKE_INSTALL_BINDIR})

 add_executable (makeHipoSelector makeHipoSelector.cpp)

@c-dilks
Copy link
Member

c-dilks commented Feb 1, 2024

Perhaps the hipo4/ subdirectory can be removed, since those files appear to be an older version.

@dglazier dglazier closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants