[WIP] Some modernization and this should fix #68, #29 #89

flagarde · 2020-07-17T04:14:14Z

Some modernization and this should fix FindROOT.cmake should be removed #68, LCIO project should not have a top-level bin directory in its source tree #29

… version

CMakeLists.txt

flagarde · 2020-07-20T12:07:47Z

Done

rete · 2020-07-22T12:41:02Z

CMakeLists.txt

+if( BUILD_ROOTDICT )
+  find_package( ROOT 6.04 REQUIRED )
+  ##Need to check if ROOT has been compiled with other CXX_STANDARD
+  get_property(ROOT_CXX_STANDARD TARGET ROOT::Core PROPERTY INTERFACE_COMPILE_FEATURES)


I don't think this is a good idea to overwrite the CXX standard here depending on ROOT. This should be up to the user to make sure things are compatible.

If we don't do this it cannot compile.. other option is to just compile thé Root code with such property. Like this the project Can still be cpp11 but the root code that must follow root properties can be built

rete · 2020-07-22T12:41:59Z

CMakeLists.txt

-IF( NOT SH )
-    MESSAGE( FATAL_ERROR "unix shell not found" )
-ENDIF()
+#FIND_PROGRAM( SH sh ${CYGWIN_INSTALL_PATH}/bin /bin /usr/bin /usr/local/bin /sbin )


Why did you remove this? It is required

I didn't find anywhere where you use the sh program

rete · 2020-07-22T12:44:29Z

CMakeLists.txt



 # generate and install following configuration files
 GENERATE_PACKAGE_CONFIGURATION_FILES( LCIOConfig.cmake LCIOConfigVersion.cmake )

-INSTALL( FILES cmake/MacroCheckPackageLibs.cmake cmake/MacroCheckPackageVersion.cmake DESTINATION lib/cmake)
+INSTALL( FILES cmake/MacroCheckPackageLibs.cmake cmake/MacroCheckPackageVersion.cmake DESTINATION "${CMAKE_INSTALL_PREFIX}/lib/cmake")


The CMake documentation says:

DESTINATION Specify the directory on disk to which a file will be installed. If a full path (with a leading slash or drive letter) is given it is used directly. If a relative path is given it is interpreted relative to the value of the CMAKE_INSTALL_PREFIX variable. The prefix can be relocated at install time using the DESTDIR mechanism explained in the CMAKE_INSTALL_PREFIX variable documentation.

The variable CMAKE_INSTALL_PREFIX is not useful here.

Yes i had a doubt

rete · 2020-07-22T12:47:34Z

cmake/MacroGeneratePackageConfigFiles.cmake

-                #IF( EXISTS "${_current_dir}/MacroExportPackageDeps.cmake" )
-                #    INSTALL( FILES "${_current_dir}/MacroExportPackageDeps.cmake" DESTINATION cmake )
-                #ENDIF()
+                INSTALL( FILES "${PROJECT_BINARY_DIR}/${arg}" DESTINATION "lib/cmake" )


The CMake config files should be installed in the top level directory. I know that it might not be the best practice or the recommended location but other packages still rely on that at the time being. Please switch it back to .

OK, would you agree to put it by default in the top level but add and option to allow the user to change the location ? Or populate in the top directory and in thé standard /lib/cmake/LCIO one? Doing so I think cmake will be able to find LCIO is the user decide to install the project in the default paths given by cmake

rete · 2020-07-22T12:47:42Z

cmake/MacroGeneratePackageConfigFiles.cmake

-                #IF( EXISTS "${_current_dir}/MacroCheckPackageVersion.cmake" )
-                #    INSTALL( FILES "${_current_dir}/MacroCheckPackageVersion.cmake" DESTINATION cmake )
-                #ENDIF()
+                INSTALL( FILES "${PROJECT_BINARY_DIR}/${arg}" DESTINATION "lib/cmake" )


rete · 2020-07-22T12:53:56Z

sio/CMakeLists.txt

@@ -37,6 +33,6 @@ INSTALL_SHARED_LIBRARY( sio DESTINATION lib )

 # mimic the SIOConfig.cmake variables for the LCIO project
 SET( SIO_VERSION                  "${SIO_VERSION}"                       CACHE STRING "The SIO version" )
-SET( SIO_LIBRARIES                "$<TARGET_FILE:sio>"                   CACHE STRING "The SIO library" )
+SET( SIO_LIBRARIES                "sio"                   CACHE STRING "The SIO library" )


No. This is processed by the CMake generator and forms a variable which is interpreted later ON. Please changed it back.

Actually, using the generator expression causes the problem solved in #91 . As far as we understand, because otherwise no dependency is formed beween the lcio target and the sio target. Though I am not sure about the use of quotes.

I guess it comes from the SIO package where it was needed, but maybe not here indeed. I'll have to look in the SIO package. The CMakeLists.txt is a bit different

Do you still want to have a sio folder on this package ? As it's now standalone it could be download by cmake using externalproject for example ... but it will allow to fix this kind of problems and avoid to change codes in two project... by the way what is thé cmake version you want to use?

About using SIO through a directory / external_project:
This has been discussed internally and we believe it was currently the best solution. The use of external_project make the thing a bit complicated to handle because of CMake variables that are imported from SIOConfig.cmake.

About the CMake version:
The minimum required here must refer to the minimum we need to make these scripts working, not the one we want to use. By the way, we currently use CMake 3.14 I think.

Yes it's more complicated I agree
I mean what is the version you want to put in cmake_minimum_required

rete · 2020-07-22T12:56:31Z

src/cpp/CMakeLists.txt

-  ./src/CPPFORT/lcwrt.cc
-)
+if( BUILD_FORTRAN )
+  set( LCIO_CPPFORT_SRCS


CMake commands in upper case please. Look also in other places

rete · 2020-07-22T12:57:41Z

src/cpp/CMakeLists.txt

@@ -347,7 +351,7 @@ IF( BUILD_ROOTDICT )
    LIST( REMOVE_ITEM UTIL_CXX_HEADERS_LIST ${exclude_headers} )


-    MESSAGE( STATUS "********************* UTIL_CXX_HEADERS_LIST:  ${UTIL_CXX_HEADERS_LIST}" )
+    #MESSAGE( STATUS "********************* UTIL_CXX_HEADERS_LIST:  ${UTIL_CXX_HEADERS_LIST}" )


Thanks! An old forgotten one! ;-)

rete · 2020-07-22T12:59:25Z

src/cpp/CMakeLists.txt

+  ADD_LCIO_EXAMPLE( lcio_parallel_processing ) 
+endif()
+
+IF( BUILD_ROOTDICT AND BUILD_LCIO_EXAMPLES)


Change

IF( BUILD_ROOTDICT AND BUILD_LCIO_EXAMPLES)

to

IF( BUILD_ROOTDICT AND INSTALL_APPS)

rete · 2020-07-22T13:01:19Z

src/cpp/src/UTIL/lXDR.cc

-   strncpy(_fileName, filename, n);
-   _fileName[n] = '\0';
+   _fileName = new char [n+1];
+   strcpy(_fileName, filename);


strcpy stops if it finds a null character during the copy, which is not what we want. Please change it back

rete · 2020-07-22T13:01:59Z

@flagarde Also TravisCI is failing. Please have a look

rete · 2020-07-22T13:03:13Z

Please give a bit detailed explanation about you changes with different bullet points.
LCIO is core package. Keeping track of changes at this level is crucial

flagarde added 19 commits July 17, 2020 10:49

Move bin to scripts

f975946

Add .gitignore

6e6fc59

Allow to disable apps compilation

e34cd36

Add options

3691b44

Supress some warnings

82f1b41

Use default ROOTConfig

8d4ef74

Install in bin by default

728ee8d

Fix

528b2fd

Option to install python codes

6e2b9c3

CMAKE_CXX_STANDARD need at least 3. something so use the project with…

8f193e9

… version

Fix doc

bc931da

Comment some noise

8cfeb92

Check if Fortran code can be compiled

ae4f53b

Don't crash if doxygen or latex are not found

c718c46

Fix warning

b2d951e

Fix?

532349a

Install in lib/cmake

3660905

Add dependency

07fbc53

Install in lib/cmake

92f8090

andresailer reviewed Jul 17, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Update CMakeLists.txt

37f8524

rete requested changes Jul 22, 2020

View reviewed changes

rete changed the title ~~Some modernization and this should fix #68, #29~~ [WIP] Some modernization and this should fix #68, #29 Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Some modernization and this should fix #68, #29 #89

[WIP] Some modernization and this should fix #68, #29 #89

flagarde commented Jul 17, 2020 •

edited

Loading

flagarde commented Jul 20, 2020

rete Jul 22, 2020

flagarde Jul 22, 2020 •

edited

Loading

rete Jul 22, 2020

flagarde Jul 22, 2020

rete Jul 22, 2020

flagarde Jul 22, 2020

rete Jul 22, 2020

flagarde Jul 22, 2020 •

edited

Loading

rete Jul 22, 2020

rete Jul 22, 2020

andresailer Jul 22, 2020

rete Jul 22, 2020

flagarde Jul 22, 2020 •

edited

Loading

rete Jul 22, 2020

flagarde Jul 22, 2020

rete Jul 22, 2020

rete Jul 22, 2020

rete Jul 22, 2020

rete Jul 22, 2020

rete commented Jul 22, 2020

rete commented Jul 22, 2020

[WIP] Some modernization and this should fix #68, #29 #89

Are you sure you want to change the base?

[WIP] Some modernization and this should fix #68, #29 #89

Conversation

flagarde commented Jul 17, 2020 • edited Loading

flagarde commented Jul 20, 2020

Choose a reason for hiding this comment

flagarde Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flagarde Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flagarde Jul 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rete commented Jul 22, 2020

rete commented Jul 22, 2020

flagarde commented Jul 17, 2020 •

edited

Loading

flagarde Jul 22, 2020 •

edited

Loading

flagarde Jul 22, 2020 •

edited

Loading

flagarde Jul 22, 2020 •

edited

Loading