nexusformat / code

NeXus API code and helper applications
GNU Lesser General Public License v2.1
12 stars 16 forks source link

nxtranslate - alternative approach #431

Closed eugenwintersberger closed 8 years ago

eugenwintersberger commented 8 years ago

Hi folks, as Pete has done the job with the nxtranslate libraries I would like to present different approach. I don't think it is a good idea to install shared libraries required by a single program. However, CMake offers a different way to solve this issue. Stay tuned ;)

eugenwintersberger commented 8 years ago

@stuartcampbell @peterfpeterson Hi folks, would this be a reasonable solution for you. I guess this should be portable over all platforms.

jkrueger1 commented 8 years ago

What do yo think about the following solution:

'Create additionally a static library (also for installation) and link against it'

?

eugenwintersberger commented 8 years ago

Same question as for the dynamic libraries: why installing libraries that nobody is using for development? Or is there any software around which relies on the nxtranslate libraries?

jkrueger1 commented 8 years ago

Ok, now I understand. Then I would generate a static library (and not install) and link against.

eugenwintersberger commented 8 years ago

This is pretty much what OBJECT is doing right now in this branch. At least I do not see the big difference.

zjttoefs commented 8 years ago

@eugenwintersberger without actually installing from your branch, was does your solution do differently?

The binary does not need all libraries, so loading that at runtime sounds like a reasonable idea. It's maybe not standard practice, but also not the worst plan.

jkrueger1 commented 8 years ago

I know, but for me the CMake file looks a little be ugly in the change. For me the solution with the static lib would look like a little bit more elegant ;-). But in principle I agree with you.

eugenwintersberger commented 8 years ago

@zjttoefs I use add_library(blabla OBJECT ${SOURCES}) to build the different modules and then add them to the final binary with add_executable(nxtranslate ${SOURCES} $<TARGET_OBJECTS:blabla>)

eugenwintersberger commented 8 years ago

@jkrueger1 Technically, if I understand the cmake documentation correctly, the major difference between OBJECT and STATIC libraries is that the former ones cannot be installed or linked. This is pretty much what we want. Personally, I find this far more elegant than building a static library which will never be installed.

quantumsteve commented 8 years ago

@eugenwintersberger If these classes are only used by nxtranslate, should they be moved up a directory and added to the list of source files? I think that would be identical to what's being done on this branch, but without the ugly CMake syntax.

eugenwintersberger commented 8 years ago

@quantumsteve In principle you are right. The classes are only used by nxtranslate (at least to my knowledge). And yes one could move them one level up and add them to the list of sources. What I like on this OBJECT approach is that each submodule has its own separate source file list which can be maintained by the modules developer. A single source file list as you suggest it would quickly become rather extensive and hard to maintain.

Aside from its ugliness, which indeed a matter of personal taste, is there anyone having a serious problem with my suggested approach. I would really love to make a new NAPI release within this week ;)

peterfpeterson commented 8 years ago

The suggested change is reasonable. It does build and (mostly) work on my fedora23 machine, but ctest --output-on-failure gives:

$ ctest --output-on-failure
Test project ...
      Start  1: NAPI-C-HDF5-test
 1/14 Test  #1: NAPI-C-HDF5-test .................   Passed    0.03 sec
      Start  2: NAPI-C-HDF5-attra-test
 2/14 Test  #2: NAPI-C-HDF5-attra-test ...........   Passed    0.01 sec
      Start  3: NAPI-C-HDF4-test
 3/14 Test  #3: NAPI-C-HDF4-test .................   Passed    0.02 sec
      Start  4: NAPI-C-HDF4-attra-test
 4/14 Test  #4: NAPI-C-HDF4-attra-test ...........   Passed    0.01 sec
      Start  5: NAPI-C-MXML-test
 5/14 Test  #5: NAPI-C-MXML-test .................   Passed    0.03 sec
      Start  6: NAPI-C-MXML-TABLE-test
 6/14 Test  #6: NAPI-C-MXML-TABLE-test ...........   Passed    0.03 sec
      Start  7: NAPI-C-MXML-attra-test
 7/14 Test  #7: NAPI-C-MXML-attra-test ...........   Passed    0.01 sec
      Start  8: NAPI-C-leak-test-1
 8/14 Test  #8: NAPI-C-leak-test-1 ...............   Passed    0.31 sec
      Start  9: NAPI-C-test-nxunlimited
 9/14 Test  #9: NAPI-C-test-nxunlimited ..........   Passed    0.35 sec
      Start 10: NAPI-C++-HDF5-test
10/14 Test #10: NAPI-C++-HDF5-test ...............   Passed    0.03 sec
      Start 11: NAPI-C++-HDF4-test
11/14 Test #11: NAPI-C++-HDF4-test ...............***Exception: Other  0.05 sec
NXinquirefile found: napi_test_cpp.hdf
This is a HDF4 file, attribute array API is not supported here
terminate called after throwing an instance of 'NeXus::Exception'
  what():  NXgetnextattr failed

      Start 12: NAPI-C++-MXML-test
12/14 Test #12: NAPI-C++-MXML-test ...............***Exception: Other  0.56 sec
NXinquirefile found: napi_test_cpp.xml
This is an XML file, attribute array API is not supported here
terminate called after throwing an instance of 'NeXus::Exception'
  what():  NXgetnextattr failed

      Start 13: NAPI-C++-leak-test-2
13/14 Test #13: NAPI-C++-leak-test-2 .............   Passed    0.67 sec
      Start 14: NAPI-C++-leak-test-3
14/14 Test #14: NAPI-C++-leak-test-3 .............   Passed    1.53 sec

86% tests passed, 2 tests failed out of 14

Total Test time (real) =   3.65 sec

The following tests FAILED:
     11 - NAPI-C++-HDF4-test (OTHER_FAULT)
     12 - NAPI-C++-MXML-test (OTHER_FAULT)
Errors while running CTest

Since this is independent of the changes you made, I say :shipit:.

eugenwintersberger commented 8 years ago

@peterfpeterson Hi Pete. The problem here is the attribute handling. This is a well known issue (I think Tobias is on it). However, as it affects only the HDF4 and XML backend, I consider this issue not as release critical.

PS: you are right, this has nothing to do with the changes in nxtranslate.

stuartcampbell commented 8 years ago

@eugenwintersberger - I think this looks good. I have built some test rpms for the Red Hat family of distros (i.e. Fedora and RHEL) and used them to compile Mantid and all seems to work! :smile:

eugenwintersberger commented 8 years ago

Ok. In this case I consider this issue as closed.