Conversation
597b52a to
6d74645
Compare
6d1d6b1 to
1c7d1cb
Compare
|
Thank you for merging #30 @tehrengruber , @havogt . |
b5f514e to
3281313
Compare
|
I only did a brief walk through the code so far. One question popped up and I didn't follow the code to figure it out myself: how is the version managed? Is it taking always the version of atlas? |
atlas4py version is not managed by the found atlas library, but that could be a strategy. I did not deviate from what was in place. I removed however duplicate places where the version was defined with only a single place remaining, in the There is still the extra variable |
3281313 to
7ef5085
Compare
|
Note, just rebased again on master. |
| @@ -1,5 +1,6 @@ | |||
| black>=21.12b0 | |||
| bump2version>=1.0 | |||
There was a problem hiding this comment.
| bump2version>=1.0 |
src/atlas4py/CMakeLists.txt
Outdated
| find_package(ecbuild) | ||
| if(NOT ecbuild_FOUND) | ||
| FetchContent_Declare( | ||
| if(build_atlas) |
There was a problem hiding this comment.
The changes in this if-branch are fine with me, but my tendency is rather to simplify than to extend here. In case one want's to build atlas the motivation is likely: I want to use or develop atlas4py, don't bother me with installing atlas, just set me up. Being able to say build atlas for me, but please use this eckit version I provide appears a rather narrow use case that can easily be handled by just building everything manually.
if(build_atlas)
# Build ecbuild
set ( ecbuild_SOURCE_DIR ${CMAKE_BINARY_DIR}/_deps/ecbuild )
message( STATUS "Downloading ecbuild version \"${ATLAS4PY_ECBUILD_VERSION}\" to ${ecbuild_SOURCE_DIR}" )
FetchContent_Populate(
ecbuild
GIT_REPOSITORY https://github.com/ecmwf/ecbuild.git
GIT_TAG ${ATLAS4PY_ECBUILD_VERSION}
SOURCE_DIR ${ecbuild_SOURCE_DIR}
QUIET
)
set( ecbuild_ROOT ${ecbuild_SOURCE_DIR} CACHE INTERNAL "Found ecbuild" )
# Build eckit
message( STATUS "Downloading and building eckit version \"${ATLAS4PY_ECKIT_VERSION}\"" )
# Disable unused features for faster compilation
set(ECKIT_ENABLE_TESTS OFF)
set(ECKIT_ENABLE_DOCS OFF)
set(ECKIT_ENABLE_PKGCONFIG OFF)
set(ECKIT_ENABLE_ECKIT_GEO OFF)
set(ECKIT_ENABLE_ECKIT_SQL OFF)
set(ECKIT_ENABLE_ECKIT_CMD OFF)
FetchContent_Declare(
eckit
GIT_REPOSITORY https://github.com/ecmwf/eckit.git
GIT_TAG ${ATLAS4PY_ECKIT_VERSION}
)
FetchContent_MakeAvailable(eckit)
set( eckit_ROOT ${eckit_BINARY_DIR} CACHE INTERNAL "Found eckit" )
# build atlas
message( STATUS "Downloading and building atlas version \"${ATLAS4PY_ATLAS_VERSION}\"" )
# Disable unused features for faster compilation
set(ATLAS_ENABLE_TESTS OFF)
set(ATLAS_ENABLE_DOCS OFF)
set(ATLAS_ENABLE_PKGCONFIG OFF)
set(ECKIT_ENABLE_ECKIT_GEO OFF)
set(ECKIT_ENABLE_ECKIT_SQL OFF)
set(ATLAS_ENABLE_ECKIT_CMD OFF)
FetchContent_Declare(
atlas
GIT_REPOSITORY https://github.com/ecmwf/atlas.git
GIT_TAG ${ATLAS4PY_ATLAS_VERSION}
)
FetchContent_MakeAvailable(atlas)
| PYBIND11_MODULE( _atlas4py, m ) { | ||
| m.def("_initialise", atlasInitialise) | ||
| .def("_finalise", atlas::finalise); | ||
| m.attr("version") = atlas::Library::instance().version(); |
There was a problem hiding this comment.
| m.attr("version") = atlas::Library::instance().version(); | |
| m.attr("atlas_version") = atlas::Library::instance().version(); |
or just removing it altogether?
| m.attr("version") = atlas::Library::instance().version(); |
| else() | ||
| set(rpath_origin_install_libdir "$ORIGIN/${CMAKE_INSTALL_LIBDIR}") | ||
| endif() | ||
| set( CMAKE_INSTALL_RPATH "${rpath_origin_install_libdir}" ) # A semicolon-separated list specifying the RPATH to use in installed targets |
There was a problem hiding this comment.
Can you add a motivation for the 3 additional options here?
| atlas::initialise(argc, argv); | ||
| } | ||
|
|
||
| py::object toPyObject( eckit::Configuration const& v ); |
There was a problem hiding this comment.
If you want a textbook implementation you could also use py::cast and a type caster for eckit::Configuration.
I have made time available this year to work on our plan to extend atlas4py and hand over maintenance.
As a first step I tried the new build-system, and noticed that I needed to tweak the installation a little bit.