JeffersonLab / hcana

Hall C++ Analyzer
7 stars 118 forks source link

Added CMake buld #370

Closed whit2333 closed 6 years ago

whit2333 commented 6 years ago

This build works with https://github.com/whit2333/hallac_evio and https://github.com/whit2333/analyzer . Hopefully those PRs are merged.

sawjlab commented 6 years ago

We need the build process to be simple. To build hcana with make or scons, you just type the command and everything is built. The user doesn't even have to realize that evio and analyzer are being built. Also, while I don't think it is true at the moment, in the past there have been conditional compiles in analyzer so that analyzer gets built differently when part of hcana and we need the ability to do that should the need arise. The cmake build instructions should look like: "mkdir/cd - cmake - make."

hansenjo commented 6 years ago

Yes, exactly. That's my main issue with it, too. It needs to be a classic 3-line CMake idiom. If one needs to fiddle around and even install modified versions of what one already has (EVIO), it's not the kind of thing that makes out users happy.

Incidentally, it also needs to work on RHEL6 with ROOT5 (Hall C environment at the moment).

Ole

whit2333 commented 6 years ago

Hi Steve,

We need the build process to be simple.

This is a bit subjective. CMake is the de facto build tool nowadays and not following conventions makes things more difficult.

The next step is to add the hooks in hcana to build analyzer/evio.

A not so obvious reason why I am doing all this is it helps when building container images. The source and build dirs are thrown away.
Having source/build/install all cleanly separated is very useful.

The cmake build instructions should look like: "mkdir/cd - cmake - make."

They don't look this way now?

Cheers, Whit

On Thu, Jun 28, 2018 at 03:19:05PM +0000, Stephen Wood wrote:

We need the build process to be simple. To build hcana with make or scons, you just type the command and everything is built. The user doesn't even have to realize that evio and analyzer are being built. Also, while I don't think it is true at the moment, in the past there have been conditional compiles in analyzer so that analyzer gets built differently when part of hcana and we need the ability to do that should the need arise. The cmake build instructions should look like: "mkdir/cd - cmake - make."

-- Whitney R. Armstrong

sawjlab commented 6 years ago

scons is going to remain the recommended build system for hcana for a while. We can add cmake, as long as it is presented at the end of the readme as an alternative.

As far as I can tell, the cmake build instructions for hcana are clone mkdir/cd - cmake - make clone mkdir/cd - cmake - make mkdir/cd - cmake - make

whit2333 commented 6 years ago

@hansenjo RHEL6 is ancient and ROOT 5 is no longer supported! It is time to modernize!

whit2333 commented 6 years ago

@sawjlab Ah, yes currently you have to build each one, but there is a straight forwad way to build evething in one shot. This step is needed before that.

sawjlab commented 6 years ago

Also, src/CMakeLists.txt needs to not explicitly list the source and header files and HallC_LinkDef.h needs to remain autogenerated.

whit2333 commented 6 years ago

All of these things are deliberate.

Auto generating the LinkDef is a bad idea for multiple reasons, mostly
because there is no point doing so. If a dictionary is needed you edit the file. If you want to use the class in, eg, std::vector or std::map you add it. Plus there is way more to the LinkDef than the features currently "auto generated". Also, globing the source files in cmake is considered bad form.

What the source code should have is an "include" directory for the headers. This would prevent the need to list each header file (and just install the whole directory). But this would require a "breaking" change.

On Thu, Jun 28, 2018 at 10:29:29AM -0700, Stephen Wood wrote:

Also, src/CMakeLists.txt needs to not explicitly list the source and header files and HallC_LinkDef.h needs to remain autogenerated.

-- Whitney R. Armstrong

whit2333 commented 6 years ago

Again, this PR changes nothing in the existing code and just adds the option to use CMake.