Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add catalyst and adios #214

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Add catalyst and adios #214

wants to merge 17 commits into from

Conversation

julienfausty
Copy link

@julienfausty julienfausty commented Jan 24, 2023 •
edited
Loading

This pull request comes in support of this one in GEOSX: GEOS-DEV/GEOS#2250

It performs the following operations
Add new dependencies into thirdPartyLibs repository:

  • catalyst itself version 2.0.0-rc3
  • adios/catalyst implementation on commit af3b35f
  • adios2 v2.8.3
  • update conduit from 0.8.2 to 0.8.6
  • update VTK to 49361a2 for mangling capabilities

@julienfausty julienfausty mentioned this pull request Jan 24, 2023
Closed
7 tasks
CharlesGueunet
CharlesGueunet reviewed Jan 24, 2023
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt
@@ -214,7 +215,7 @@ list(APPEND build_list hdf5 )
# Conduit
################################
set(CONDUIT_DIR "${CMAKE_INSTALL_PREFIX}/conduit")
set(CONDUIT_URL "${TPL_MIRROR_DIR}/conduit-0.8.2.tar.gz")
set(CONDUIT_URL "${TPL_MIRROR_DIR}/conduit-0.8.6.zip")
Copy link

Choose a reason for hiding this comment

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

this is not the Catalyst version, is this wanted ?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I don't think we should tether geosx's conduitversion to catalyst's. This is just an update of the conduit package to get some things compiling

@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch from b0a01ba to 7a00253 Compare March 6, 2023 08:44
@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch 2 times, most recently from acf1e38 to 84f47f5 Compare March 14, 2023 08:31
@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch 2 times, most recently from 0fdc45f to 47f28b0 Compare April 5, 2023 12:57
@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch from 47f28b0 to a716a79 Compare April 26, 2023 07:34
@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch from a716a79 to 4a96262 Compare July 20, 2023 07:52
@CharlesGueunet
VTK: update to master and add mangling
ff57a33 
Mangling VTK uses an inline namespace to change the VTK symbols in the
binary so as to allow multiple VTK binaries to coexist
@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch from 4a96262 to ff57a33 Compare July 20, 2023 08:27
GCC13: patch and fix errors and warnings
05e2192 
* downgrade fmt 10.0.0 -> 9.1.0
* patch raja for stdexcept and cstdlib
@julienfausty julienfausty force-pushed the add-catalyst-and-adios branch from 26b0f75 to 6e1dad0 Compare September 7, 2023 13:37
CharlesGueunet
CharlesGueunet approved these changes Sep 14, 2023
View reviewed changes
Copy link

@CharlesGueunet CharlesGueunet left a comment

Choose a reason for hiding this comment

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

LGTM after small questions / remarks

CMakeLists.txt
@@ -1078,7 +1085,8 @@ endif()
################################
if( ENABLE_VTK )
set( VTK_DIR "${CMAKE_INSTALL_PREFIX}/vtk" )
set( VTK_URL "${TPL_MIRROR_DIR}/VTK-9.2.6.tar.gz" )
set( VTK_URL "${TPL_MIRROR_DIR}/VTK-59d89108f4.tar.gz" )
Copy link

Choose a reason for hiding this comment

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

you do not need to change, but a think a nightly (with a date as a version) is slightly more readable.
Can you comment that this is between 9.2.6 and 9.3.0 ?

Copy link
Author

Choose a reason for hiding this comment

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

yep, I was not aware of the nightly VTK downloads, I will use that and update the repo

Copy link
Author

Choose a reason for hiding this comment

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

see below

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated
################################
# FMT
################################
set(FMT_DIR "${CMAKE_INSTALL_PREFIX}/fmt")
set(FMT_URL "${TPL_MIRROR_DIR}/fmt-10.0.0.tar.gz")
set(FMT_URL "${TPL_MIRROR_DIR}/fmt-9.1.0.zip")
Copy link

Choose a reason for hiding this comment

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

why a downgrade ?

Copy link
Author

Choose a reason for hiding this comment

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

was a compiler issue with gcc14

CMakeLists.txt
BUILD_COMMAND ${TPL_BUILD_COMMAND}
INSTALL_COMMAND "${TPL_INSTALL_COMMAND}"
PATCH_COMMAND patch -p1 < ${TPL_MIRROR_DIR}/VTK-ABI.patch
Copy link

Choose a reason for hiding this comment

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

why is this a patch ? seems like it should be updated upstream

Copy link
Author

Choose a reason for hiding this comment

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

This has been updated upstream, but to avoid wasting time with the merge I made this patch and then forgot about it. I will update VTK with a nightly build and take out the patch

Copy link
Author

Choose a reason for hiding this comment

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

the 9.3.0-rc1 version has a broken ABI, let's wait for the release before making the changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharlesGueunet CharlesGueunet CharlesGueunet approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@julienfausty @CharlesGueunet @sframba