Libdap #3333

kneumiller · 2020-01-16T18:02:24Z

Part of the pull requests for OPeNDAP:
These new additions will search for the libdap libraries through cmake. Libdap is necessary for reading data from a url, which is what the "url reader" pull request adds.

Before your first pull request:

[ X ] I have read the guidelines in our CONTRIBUTING.md document.

For all pull requests:

[ X ] I have followed the instructions for indenting my code.

For new features/models or changes of existing features:

[ X ] I have tested my new feature locally to ensure it is correct.
[ X ] I have created a testcase for the new feature/benchmark in the tests/ directory.
[ X ] I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.

… up the libdap libraries/directories if the user sets LIBDAP_ON=ON.

…ml2 fix Timo suggested

tjhei

I have a few comments after reading through it. Let us know when this is ready for review/testing.

tjhei · 2020-01-17T18:04:44Z

cmake/modules/FindLIBDAP.cmake

+        )
+
+FIND_LIBRARY(LIBDAP_LIBRARY
+        NAMES libdap.dylib


This seems to be OSX specific. Can you try if this works when using "NAMES libdap" ?

I couldn't get it to work with just "NAMES libdap", but I can list different options so that it searches for our library with different extensions in the order given: NAMES libdap.so libdap.dylib libdap.la

at least .so and .dylib. I am not sure about ".la".

tjhei · 2020-01-17T18:04:48Z

doc/modules/changes/20200116_kneumiller

+Added: A module in the cmake directory that looks for, and adds the path of, the libdap libraries.
+A lookup was also added in CMakeLists for the libdap, libxml, and curl libraries.
+<br>
+(Kodi Neumiller, 2020/01/16)


please add a newline at the end.

Do you mean another newline after my name/date?

tjhei · 2020-01-17T18:05:20Z

CMakeLists.txt

+# Added: 10/16/18
+IF(LIBDAP)
+  LIST(APPEND CMAKE_MODULE_PATH "${CMAKE_MODULE_PATH}/cmake/modules")
+  MESSAGE(STATUS "cmake module path = ${CMAKE_MODULE_PATH}")


remove this

If I remove this then CMakeLists appears to not be able to run FindLIBDAP.cmake which is in the cmake/modules/ directory. If I remove the LIST(APPEND CMAKE_MODULE_PATH "${CMAKE_MODULE_PATH}/cmake/modules") then I have to put the FindLIBDAP.cmake file in the aspect directory, unless there is another way to do this.

I can remove the MESSAGE(STATUS "cmake module path = ${CMAKE_MODULE_PATH}") if you want though, that was just there so the user knew where CMakeLists.txt was calling its other, external 'Find.cmake' files.

Yes, sorry, I meant the MESSAGE. I don't think it is particularly useful for the user to know.

tjhei · 2020-01-17T18:05:35Z

CMakeLists.txt

+  FIND_PACKAGE(LIBDAP)
+  IF(${LIBDAP_FOUND})
+    FIND_PACKAGE(LibXml2)
+    FIND_PACKAGE(CURL)


Same here. Also, what if one of these checks fails?

Right now if the FIND_PACKAGE fails it just returns: "Could not find a package configuration file provided by CURL/LibXml2".

However it looks like it still finds the libdap libraries if they are installed. Without CURL or libxml2 then the code we added to utilities.cc in url_reader won't be able to read data from a url. I could add a warning or an error if one or both of the CURL/libxml2 packages aren't found.

Then it is probably best to find the packages first and only enable it if you find all three. I am still confused though, shouldn't libdab provide you with all information including what libraries we need to link to? This can be handled inside FindLIBDAP if needed.
I just checked my installation of libdab and found ./bin/dap-config --libs, which returns the complete link line. That would be one way to find the necessary information.

not exactly the same as curl doesn't have other libs as dependencies, but this might help: https://github.com/Kitware/CMake/blob/master/Modules/FindCURL.cmake#L136
Let me know if you need more help.

I've updated the code to not build if one or both CURL and libxml2 are not found. If you think there's more to be done please let me know. In your latest comment are you saying I could add the dependent library lookups to FindLIBDAP?

Thanks for all of the help by the way

tjhei · 2020-01-17T18:05:54Z

CMakeLists.txt

+  MESSAGE(STATUS "cmake module path = ${CMAKE_MODULE_PATH}")
+  FIND_PACKAGE(LIBDAP)
+  IF(${LIBDAP_FOUND})
+    FIND_PACKAGE(LibXml2)


What if this check find a different libxml than libdap was configured with?

tjhei · 2020-01-17T18:06:27Z

CMakeLists.txt

+# Find the libdap package so that Aspect can connect to the OPeNDAP servers
+# Author: Kodi Neumiller
+# Added: 10/16/18
+IF(LIBDAP)


what does this IF do?

We put this in there in case the user wants to compile aspect without libdap. We added an option for when cmake is run: 'LIBDAP=ON'. The default is set to OFF, but if the user wishes to search for and use the libdap package they will need to run cmake with the option '-DLIBDAP=ON'.

I can take this out which would just display "libdap package not found" if the user ran cmake without libdap packages installed. It's up to you, we had it in there so that the user could decide if they wanted aspect to search for libdap libraries and to avoid more clutter when cmake is run and the user doesn't care about the libdap libraries.

oh, okay. Can you use something like ASPECT_WITH_LIBDAP and create a cache variable like https://github.com/geodynamics/aspect/blob/master/CMakeLists.txt#L116-L118 ? This way it can be toggled inside "ccmake".

That looks like a much better idea, I'll make those changes.

tjhei · 2020-01-17T18:08:35Z

CMakeLists.txt

+    FIND_PACKAGE(CURL)
+    INCLUDE_DIRECTORIES(${LIBDAP_INCLUDE_DIR} ${LIBXML2_INCLUDE_DIR} ${LIBXML2_INCLUDE_DIR}/libxml2 ${CURL_INCLUDE_DIR})
+    TARGET_LINK_LIBRARIES(${TARGET} ${LIBDAP_LIBRARIES} ${LIBXML2_LIBRARIES} ${CURL_LIBRARIES})
+    ADD_DEFINITIONS(-DHAVE_LIBDAP=TRUE)


can you use something like https://github.com/geodynamics/aspect/blob/master/CMakeLists.txt#L441 instead please?

…g for libraries in this way will make it so that the correct library is found based on the system the user is using.

…CT_WITH_LIBDAP'. Changed the way the 'HAVE_LIBDAP' definition is added to fit how others are added. Added a newline after my name in the doc/module/ file

tjhei · 2020-01-22T13:25:47Z

I've updated the code to not build if one or both CURL and libxml2 are not found. If you think there's more to be done please let me know. In your latest comment are you saying I could add the dependent library lookups to FindLIBDAP?

This is a good start, but I think that you need to discover from libdap where its dependencies are. For example, my system has different versions of libcurl installed and the one linked to by libdap is not the one in /usr/lib/.
Is there documentation available how libdap likes people to link to it?

…FindLIBDAP cmake file rather than in the CMakeLists

kneumiller · 2020-01-23T19:35:51Z

but I think that you need to discover from libdap where its dependencies are.

I don't think we support that in libdap itself. Unless I'm misunderstanding and there is a way to do it in CMakeLists or FindLibdap.

Is there documentation available how libdap likes people to link to it?

there is dap-config which is a script that takes a number of switches like --libs and --cflags that tell poeple how to link and compile code that will use libdap.

tjhei · 2020-01-23T21:14:54Z

there is dap-config which is a script that takes a number of switches like --libs and --cflags that tell poeple how to link and compile code that will use libdap.

To get correct results, we probably need to parse exactly that information. Let me explain with my machine as an example:

$ cmake -D ASPECT_WITH_LIBDAP=ON -D LIBDAP_DIR=/ssd/spack/opt/spack/linux-ubuntu16.04-broadwell/gcc-5.4.0/libdap4-3.20.4-2dzjmq6onqavz3cc5gbdeh3jgppkv7lu  ..
[...]
-- Found CURL: /usr/lib/x86_64-linux-gnu/libcurl.so (found version "7.47.0")  
[...]

but

$ ldd $DAP_PATH/libdapclient.so | grep curl
        libcurl.so.4 => /ssd/spack/opt/spack/linux-ubuntu16.04-broadwell/gcc-5.4.0/curl-7.63.0-776fukubmmxkthgfnaohu2ntw36ukseq/lib/libcurl.so.4 (0x00007f0a38a94000

which means dap is compiled with a different version of libcurl than the one you detect. It is even worse for libxml2: the script can not find it, as there is no system installation (I have all 3 installed using spack).

Let me know if you want me to help with making this work.

kneumiller · 2020-01-23T21:33:07Z

I would greatly appreciate the help, what did you have in mind?

tjhei · 2020-01-27T15:57:17Z

I have a draft in this branch: https://github.com/tjhei/aspect/tree/libdap_dep
Can you please take a look and test if this works for you on OSX?

kneumiller · 2020-01-27T18:10:29Z

Looks good! I will push the changes up. Do we want to keep all of the output of libs and flags? It looks like a lot of information being displayed at once.

If so then I think it should be ready, correct?

…for the curl and libxml2 libraries.

tjhei · 2020-01-27T18:29:25Z

Do we want to keep all of the output of libs and flags? It looks like a lot of information being displayed at once.

It helped me with debugging the output, but I don't feel strongly about it. You can remove the output inside the loop if you prefer.

tjhei · 2020-01-27T18:29:38Z

.gitignore

@@ -61,3 +61,4 @@

 .idea/
 cmake-build-debug/
+/.autotools


is this still needed?

tjhei · 2020-01-27T18:31:01Z

doc/modules/changes/20200116_kneumiller

+A lookup was also added in CMakeLists for the libdap, libxml, and curl libraries.
+<br>
+(Kodi Neumiller, 2020/01/16)
+<br>


can you remove this
line please?

tjhei · 2020-01-27T18:31:29Z

If so then I think it should be ready, correct?

with the changes above, I think we are finally good to go.

- fix list of libraries linked to - add ASPECT header - fix changelog and add myself - fix include path detection

- fix list of libraries linked to - add ASPECT header - fix changelog and add myself - fix include path detection - cleaner output

followup to #3333: libdap configuration

- fix list of libraries linked to - add ASPECT header - fix changelog and add myself - fix include path detection - cleaner output

kneumiller added 4 commits January 14, 2020 12:06

Added Libdap to the list of packages in Aspect. Aspect will only look…

fd9d7da

… up the libdap libraries/directories if the user sets LIBDAP_ON=ON.

Added a check before setting LIBDAP_FOUND to TRUE, and added the libx…

bde9db2

…ml2 fix Timo suggested

Changed the libraries that are searched for to be the dynamic libraries

872faf8

Added the docs for contributing.

25b956e

tjhei reviewed Jan 17, 2020

View reviewed changes

kneumiller added 3 commits January 17, 2020 14:51

Told FindLIBDAP to search for different libraries of libdap. Searchin…

35e0db9

…g for libraries in this way will make it so that the correct library is found based on the system the user is using.

Modified the check for the command line option from 'LIBDAP' to 'ASPE…

7735a10

…CT_WITH_LIBDAP'. Changed the way the 'HAVE_LIBDAP' definition is added to fit how others are added. Added a newline after my name in the doc/module/ file

Added a warning if curl or libxml is not found.

ee27c32

Made it so that the curl and libxml2 libraries are looked for in the …

d5978ba

…FindLIBDAP cmake file rather than in the CMakeLists

Updated the cmake files with Timo's changes. FindLIBDAP now searches …

39cc135

…for the curl and libxml2 libraries.

tjhei added the ready to test label Jan 27, 2020

tjhei reviewed Jan 27, 2020

View reviewed changes

Clean up

2429933

tjhei merged commit 5bf8002 into geodynamics:master Jan 28, 2020

tjhei added a commit to tjhei/aspect that referenced this pull request Jan 28, 2020

followup to geodynamics#3333

040b7e6

- fix list of libraries linked to - add ASPECT header - fix changelog and add myself - fix include path detection

tjhei added a commit to tjhei/aspect that referenced this pull request Jan 28, 2020

followup to geodynamics#3333

2153914

- fix list of libraries linked to - add ASPECT header - fix changelog and add myself - fix include path detection - cleaner output

gassmoeller added a commit that referenced this pull request Jan 29, 2020

Merge pull request #3355 from tjhei/libdap_followup

d42f2db

followup to #3333: libdap configuration

gassmoeller mentioned this pull request Feb 10, 2020

Adiabatic boundary #3204

Closed

1 task

naliboff pushed a commit to naliboff/aspect that referenced this pull request Feb 11, 2020

followup to geodynamics#3333

26fb68c

- fix list of libraries linked to - add ASPECT header - fix changelog and add myself - fix include path detection - cleaner output

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Libdap #3333

Libdap #3333

kneumiller commented Jan 16, 2020 •

edited

Loading

tjhei left a comment

tjhei Jan 17, 2020

kneumiller Jan 17, 2020

tjhei Jan 17, 2020

tjhei Jan 17, 2020

kneumiller Jan 17, 2020

tjhei Jan 17, 2020

tjhei Jan 17, 2020

kneumiller Jan 17, 2020

kneumiller Jan 17, 2020

tjhei Jan 17, 2020

tjhei Jan 17, 2020

kneumiller Jan 17, 2020

tjhei Jan 18, 2020 •

edited

Loading

tjhei Jan 18, 2020 •

edited

Loading

kneumiller Jan 22, 2020 •

edited

Loading

tjhei Jan 17, 2020

tjhei Jan 17, 2020

kneumiller Jan 17, 2020 •

edited

Loading

tjhei Jan 17, 2020

kneumiller Jan 17, 2020

tjhei Jan 17, 2020

tjhei commented Jan 22, 2020

kneumiller commented Jan 23, 2020

tjhei commented Jan 23, 2020 •

edited

Loading

kneumiller commented Jan 23, 2020 •

edited

Loading

tjhei commented Jan 27, 2020

kneumiller commented Jan 27, 2020 •

edited

Loading

tjhei commented Jan 27, 2020

tjhei Jan 27, 2020

tjhei Jan 27, 2020

tjhei commented Jan 27, 2020

Libdap #3333

Libdap #3333

Conversation

kneumiller commented Jan 16, 2020 • edited Loading

Before your first pull request:

For all pull requests:

For new features/models or changes of existing features:

tjhei left a 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

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

tjhei Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

tjhei Jan 18, 2020 • edited Loading

Choose a reason for hiding this comment

kneumiller Jan 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

kneumiller Jan 17, 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

tjhei commented Jan 22, 2020

kneumiller commented Jan 23, 2020

tjhei commented Jan 23, 2020 • edited Loading

kneumiller commented Jan 23, 2020 • edited Loading

tjhei commented Jan 27, 2020

kneumiller commented Jan 27, 2020 • edited Loading

tjhei commented Jan 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjhei commented Jan 27, 2020

kneumiller commented Jan 16, 2020 •

edited

Loading

tjhei Jan 18, 2020 •

edited

Loading

tjhei Jan 18, 2020 •

edited

Loading

kneumiller Jan 22, 2020 •

edited

Loading

kneumiller Jan 17, 2020 •

edited

Loading

tjhei commented Jan 23, 2020 •

edited

Loading

kneumiller commented Jan 23, 2020 •

edited

Loading

kneumiller commented Jan 27, 2020 •

edited

Loading