search

grimbough / Rhdf5lib

Distribution of the HDF5 library in an R package
https://bioconductor.org/packages/Rhdf5lib/
6 stars 14 forks source link

Includepath only points to C headers directly #5

Closed sneumann closed 6 years ago

sneumann commented 6 years ago

Hi, The vignette says C header files do not need a subfolder specified, while C++ headers are located under c++, but existing code might have #include "H5Cpp.h". It would be great to either have the cpp headers in the same directory (are there filename conflicts ?) or add /usr/local/lib/R/site-library/Rhdf5lib/include/c++ to the include path for LinkingTo packages as well. Yours, Steffen

sneumann commented 6 years ago

Hi, update: Locally I have worked around that by mv -i c++/H5* . inside /usr/local/lib/R/site-library/Rhdf5lib/include. This worked for mzR, see also https://github.com/sneumann/mzR/issues/143. Yours, Steffen Update to the update: since it was only a single occurrence, I fixed the include call in pwiz. So no immediate need for me, but still a design question what is the "expected" behaviour for c++ includes. Yours, Steffen

grimbough commented 6 years ago

Porting existing code isn't something I had really considered.

When I first started this package I actually placed the C and C++ headers in separate folders within the Rhdf5lib/include directory. I definitely remember this causing issues, and I reverted it here. I guess after that I just stuck to the folder structure that's present following the HDF5 compilation.

It doesn't look like there are any name clashes, and my system level installation of HDF5 dumps both sets in the same folder, so I don't think I've any objection to changing this behaviour.

@LTLA any thoughts on this?

LTLA commented 6 years ago

Yes, I always thought it a bit odd that I had to do #include "c++/H5Cpp.h", but it wasn't a big issue so I never raised a fuss. I'd be happy with putting them in the same folder (I don't use the C-level API directly anyway); but if it does cause problems with compilation, I'm also happy with leaving it as it is. Existing code can just type in all of the extra 4 characters in c++/ (the horror).

sneumann commented 6 years ago

Hi, in my case I am pulling in external code using hdf5, so any update to that external library will mean I have to patch c++/ back in. Compiler will politely tell me which files I forgot.... So the change would be Highly welcome. Yours Steffen

I blame Android for the brevity and typos

LTLA commented 6 years ago

That's true. Well, let's hope that it works, then. @grimbough, give me a heads-up if/when you do it, so I can update a number of other packages that depend on Rhdf5lib.

grimbough commented 6 years ago

I've made this change, both sets of headers now appear in Rhdf5lib/include, so no need for the c++ subfolder.

It seems to work for my small test package, but if you'd like to test with the latest version here on Github and let me know if there's any issues. I'll push to BioC-devel once we're happy it's working.

LTLA commented 6 years ago

Thanks Mike, I will test it on beachmat this afternoon.

LTLA commented 6 years ago

Seems to work fine with beachmat on Ubuntu. Let me know when you commit to BioC-devel and I'll push beachmat along as well.

sneumann commented 6 years ago

Hi, mzR also works with Rhdf5lib (>= 1.1.4) and I can push to BioC right after Rhdf5lib. Yours, Steffen

grimbough commented 6 years ago

Done. Version 1.1.4 should now be in BioC devel.

mtmorgan commented 6 years ago

Sorry to come late to the party. 1.1.4 is installed on the Windows builder but beachmat and mzR fail. Also on Mac...

On windows the include/ directory structure is still c++/H5Cpp.h

grimbough commented 6 years ago

Version 1.1.5 should sort the Windows issue. Sorry about that, I should have been more thorough in my checks this end.

Might take a little longer to find the Mac problem. It looks like compilation of the C++ library is failing (or at least it can't be found), but I'm not sure what I've change that wouldn't also affect the Linux machines. I'll keep digging.

Scratch that, it doesn't even find a C++ compiler in configure. I made some changes to configure.ac so presumably it's in there somewhere.

grimbough commented 6 years ago

I think this was an issue introduced while trying to include support for the szip compression library, and using a non-platform independent command in configure.ac. Hopefully 3a8b1c6 resolves this. It now builds on both my Windows and OSX virtual machines.