Skip to content

Conversation

@planetmarshall
Copy link
Contributor

Adds a step to the Debian-10 github workflow to install the CMake project and test that the examples can be built. A few fixes were necessary to get this to work.

  • Removed the complex_io.cpp example as it failed to build with what looked like a non-trivial linking error, though someone with more familiarity with the code may be able to fix it.

  • Resolved some clang warnings by removing deprecated usages (usually unecessary declarations of default copy constructors - see https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-copy)

  • Added github workflow badge to Readme

@jkotan jkotan self-requested a review January 29, 2022 14:51
Copy link
Collaborator

@jkotan jkotan left a comment

Choose a reason for hiding this comment

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

I don't think it is good idea to remove complex_io.cpp.
It would be much better to fix it by adding

#include <h5cpp/contrib/stl/stl.hpp>

to the file. See pr_578.
After #566 one needs to include the contrib TypeTrait definitions explicitly.
It would be good also to fix the TypeTrait<Image>.get() i.e.

--- a/examples/image_h5.hpp
+++ b/examples/image_h5.hpp
@@ -72,6 +72,7 @@ class TypeTrait<Image<PixelT>>
     {
       const static DataspaceType & cref_ = Simple(hdf5::Dimensions{value.ny(),value.nx()},
                                                  hdf5::Dimensions{value.ny(),value.nx()});
+      return cref_;
     }
 
     static void *ptr(Image<PixelT> &value)

@planetmarshall
Copy link
Contributor Author

planetmarshall commented Jan 30, 2022

Also added option (off by default as this was the existing behaviour) to build the examples in the main CMake file (H5CPP_BUILD_EXAMPLES) as this makes it a bit easier to work with the example code as a developer.

@planetmarshall
Copy link
Contributor Author

Incidentally, I think that's the codebase warning-free now so may be worth adding an option (say H5CPP_COMPILE_STRICT) and have it build with warnings as errors on CI.

Copy link
Collaborator

@jkotan jkotan left a comment

Choose a reason for hiding this comment

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

Looks good to me

@jkotan
Copy link
Collaborator

jkotan commented Jan 31, 2022

Incidentally, I think that's the codebase warning-free now so may be worth adding an option (say H5CPP_COMPILE_STRICT) and have it build with warnings as errors on CI.

It sounds good. But then probably the macos tests fail because

ld: warning: object file (/Users/runner/.conan/data/zlib/1.2.11/_/_/package/647afeb69d3b0a2d3d316e80b24d38c714cc6900/lib/libz.a(compress.o)) was built for newer macOS version (11.0) than being linked (10.15)
...

/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: lib/libh5cpp.a(direct_driver.cpp.o) has no symbols

@planetmarshall
Copy link
Contributor Author

Incidentally, I think that's the codebase warning-free now so may be worth adding an option (say H5CPP_COMPILE_STRICT) and have it build with warnings as errors on CI.

It sounds good. But then probably the macos tests fail because

ld: warning: object file (/Users/runner/.conan/data/zlib/1.2.11/_/_/package/647afeb69d3b0a2d3d316e80b24d38c714cc6900/lib/libz.a(compress.o)) was built for newer macOS version (11.0) than being linked (10.15)
...

/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: lib/libh5cpp.a(direct_driver.cpp.o) has no symbols

Yes, that might be OK because it's a link not a compilation warning. I'll test it and raise a separate PR if it works.

@jkotan jkotan merged commit 9739523 into ess-dmsc:master Jan 31, 2022
jkotan added a commit that referenced this pull request Jan 31, 2022
…but the target was not found" in othe projects
jkotan added a commit that referenced this pull request Jan 31, 2022
…but the target was not found" in othe projects
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