Automatic download and build of MOAB #969

bam241 · 2024-12-04T20:25:45Z

Description

Add capability to automate pull and build MOAB at DAGMC building time

This is redondant with #964

Motivation and Context

This is part of the project from @shimwell on getting a pip install for OpenMC with DAGMC.

Changes

tweaked a little bit the findMOAB.cmake

add a make files that contains the proper ExternalProject configuration
add a CMAKE variable to enable pull and build and change the dips and link for the other DAGMC library

Behavior

Nothing changes if DDL_DEPS is not set at ON on Cmake configuration.
Otherwise pull build MOAB from MOAB repo builds it, and build the DAGMC lib against it.

Changelog file

Done

This is very much inspired by @ahnaf-tahmid-chowdhury work on PyNE cmake.

This works is sponsored by Proxima Fusion

bam241 · 2024-12-04T20:29:51Z

@ahnaf-tahmid-chowdhury @shimwell @gonuke even if this is still a draft, feel free to provide any feed back!

src/pyne/CMakeLists.txt

src/dagmc/CMakeLists.txt

bam241 · 2024-12-04T20:32:36Z

cmake/FindMOAB.cmake

+else()
+  # message(FATAL_ERROR "Could not find MOAB")


This should still return a fatal_error if ExternalProject encounters an issue

bam241 · 2024-12-04T20:34:14Z

The safeguard I added should be improve... (i.e.: have them only MOAB download is triggered...)

cmake/FindMOAB.cmake

bam241 · 2024-12-06T06:55:21Z

@gonuke this still needs some polishing here and there, but I think this is ready for a first opinion.

ahnaf-tahmid-chowdhury

Thank you very much for the update. I have a few comments for improvement.

cmake/DAGMC_macros.cmake

cmake/MOAB_PullAndMake.cmake

ahnaf-tahmid-chowdhury · 2024-12-06T12:19:53Z

cmake/MOAB_PullAndMake.cmake

+        -DBUILD_SHARED_LIBS:BOOL=ON
+        -DENABLE_HDF5:BOOL=ON
+        -DHDF5_ROOT:PATH=${HDF5_ROOT}
+        -DCMAKE_INSTALL_RPATH=${HDF5_ROOT}/lib:${MOAB_INSTALL_PREFIX}/lib


This method may work here for now, but may create trouble while working with scikit-build-core depended project. However, I am not sure here.

I have no experience about scikit-build-core, maybe we shall fix it, if the problem occurs ?

I believe MOAB master branch now has scikit-build core while version 5.5.1 does not.

Perhaps changing the moab version to master would provide a quick way of checking if this continues to work

GIT_REPOSITORY https://bitbucket.org/fathomteam/moab.git GIT_TAG ${moab_version}

Trying it now.

Just to mention @bam241 tried this with the master branch of MOAB (which has scikit build core) and reported that it works

have a doubt about what I did, I'll try one more time

Any final resolution on this?

I have tested this against MOAB master, this works.

Test on 5fe643b
checked the proper work of this against MOAB Master branch

.github/workflows/linux_build_test.yml

jon-proximafusion · 2024-12-10T12:19:18Z

Testing locally with this docker file and it compiles for me
Note this points to Bam's fork and this PR branch
Also note, this example is very minimal with no DD, Embree, Pyne

FROM ubuntu:24.04

RUN apt-get update -y
RUN apt-get install git cmake libhdf5-serial-dev g++ libeigen3-dev -y 

RUN mkdir DAGMC && \
	cd DAGMC && \
	git clone --single-branch --branch auto_moab --depth 1 https://github.com/bam241/DAGMC.git && \
	mkdir build && \
	cd build && \
	cmake ../DAGMC -DBUILD_TALLY=ON \
                   -DBUILD_STATIC_EXE=OFF \
                   -DBUILD_STATIC_LIBS=OFF \
                   -DCMAKE_INSTALL_PREFIX=/DAGMC/ \
                   -DBUILD_UWUW=OFF \
                   -DPULL_INSTALL_MOAB=ON && \
	make -j install

cmake/MOAB_PullAndMake.cmake

bam241 · 2024-12-12T11:08:30Z

Test on 5fe643b
checked the proper work of this against MOAB Master branch

jon-proximafusion · 2025-01-09T16:43:13Z

IMO this PR makes building DAGMC easier for users. Currently we ask users to compile MOAB and then compile DAGMC passing some of the info on MOAB to the DAGMC compile. I believe the current situation is overly complicated.

This PR simplifies the compile for users which I think will be appreciated and lowers the bar slightly for installing DAGMC which might help bring in a few more users.

Additionally it retains backwards compatibility so if someone does what to compile both MOAB and DAGMC in the current way then they can still do that.

The main change I would suggest is that we make the pull_install_moab variable accept a version number of MOAB and default to the current latest. This would offer greater user control over the automated download. However that could be a follow up PR if desired.

*update this also now allows users to specific the version of MOAB automatically downloaded

gonuke

A few comments trying to streamline and localize these changes.

CMakeLists.txt

cmake/FindMOAB.cmake

gonuke · 2025-01-09T17:11:58Z

cmake/FindMOAB.cmake

-if (MOAB_CMAKE_CONFIG)
+
+# First check if we are forcing the download of MOAB
+if (PULL_INSTALL_MOAB)


Have we thought about whether we put this condition up one level? That is, if we are going to PULL_INSTALL, then we don't call FindPackage at all and just do this macro instead? Then we don't need to mess with the FindMOAB file. I think that will work???

pshriwise

I'm nominally on board here and want to make the installation process easier, but I do have concerns. There are secondary dependencies to consider and how we might specify those to the direct dependencies of MOAB. For example, there's a requirement here for an HDF5_ROOT that's expected to be set prior to the CMake call (from what I can tell).

I think what's here really would make the installation process better for all, but I do want to think through these scenarios thoroughly.

src/mcnp/CMakeLists.txt

pshriwise · 2025-01-09T17:55:21Z

cmake/MOAB_PullAndMake.cmake

+        -DBUILD_SHARED_LIBS:BOOL=ON
+        -DENABLE_HDF5:BOOL=ON
+        -DHDF5_ROOT:PATH=${HDF5_ROOT}
+        -DCMAKE_INSTALL_RPATH=${HDF5_ROOT}/lib:${MOAB_INSTALL_PREFIX}/lib


Any final resolution on this?

pshriwise

Sorry, meant to comment not approve.. yet 😄

bam241 · 2025-01-09T18:14:45Z

💔

makeclean · 2025-01-10T09:20:30Z

IMO you should also add -DENABLE_FORTRAN=OFF, -DENABLE_BLASLAPCK=OFF you might need to pass an EIGEN3_DIR or something

bam241 · 2025-01-10T13:09:26Z

@gonuke, I moved this 1 level.

I added a HDF5_macor.cmake file that factories HDF5 search and variables definition.

shimwell · 2025-01-24T13:41:37Z

if PULL_INSTALL_MOAB is defined as ON, then MOAB_VERSION is set to the default "5.5.1"

I might be tempted not to allow "ON" as an option. What do others think?
Reasoning for this suggestion is that:

It makes the command more opaque, users won't know what they are getting
It adds a maintenance burden as we should keep this tag up to date with latest MOAB release

I might be tempted to use 'latest' or 'develop' as options by that is another topic.

cmake/MOAB_PullAndMake.cmake

Co-authored-by: Patrick Shriwise <[email protected]>

ahnaf-tahmid-chowdhury · 2025-01-27T15:19:46Z

cmake/MOAB_PullAndMake.cmake

+
+  INCLUDE(ExternalProject)
+  MESSAGE("HDF5_ROOT: ${HDF5_ROOT}")
+  SET(MOAB_INSTALL_PREFIX "${CMAKE_INSTALL_PREFIX}/moab")


It seems MOAB is going to be installed in a custom path, which means only DAGMC will have access to it, as it will use the rpath. This is a good approach, and we can apply the same strategy for HDF5 as well. However, I think we can use the same CMAKE_INSTALL_PREFIX for both DAGMC and MOAB instead of CMAKE_INSTALL_PREFIX}/moab, as there are no conflicts having the same file name.

I did put the MOAB installation there, because I though about scenarii where users would already have MOAB installed on it computer but would try an installation of DAGMC against a different version of MOAB.
To avoid conflict, I rather have MOAB installed in the DAGMC install dir, as it is installed through DAGMC.
Also if one would come to uninstall DAGMC by removing the install dir, it would make sense to remove MOAB at the same time.

rherrero-pf · 2025-02-03T17:07:00Z

Really like the idea behind this feature so I though I should give it a try. Happy to simplify the install process

Running this and I think the autodownloading of moab assumes that moab is available in the path /DAGMC/moab which in my case is not true.

rm -rf ~/code_development/DAGMC
mkdir ~/code_development/DAGMC
cd ~/code_development/DAGMC
git clone --single-branch --branch auto_moab --depth 1 https://github.com/bam241/DAGMC.git
mkdir build
cd build
cmake ../DAGMC -DBUILD_TALLY=ON \
                -DBUILD_STATIC_EXE=OFF \
                -DBUILD_STATIC_LIBS=OFF \
                -DCMAKE_INSTALL_PREFIX=/DAGMC/ \
                -DBUILD_UWUW=OFF \
                -DPULL_INSTALL_MOAB=ON \
                -DHDF5_ROOT=/usr/lib/x86_64-linux-gnu/hdf5
make -j install

~/code_development$ bash i.sh 
Cloning into 'DAGMC'...
remote: Enumerating objects: 453, done.
remote: Counting objects: 100% (453/453), done.
remote: Compressing objects: 100% (422/422), done.
remote: Total 453 (delta 42), reused 177 (delta 19), pack-reused 0 (from 0)
Receiving objects: 100% (453/453), 18.58 MiB | 21.03 MiB/s, done.
Resolving deltas: 100% (42/42), done.
-- The C compiler identification is GNU 11.4.0
-- The CXX compiler identification is GNU 11.4.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found Git: /usr/bin/git (found version "2.34.1") 

-- CMAKE_BUILD_TYPE not specified, defaulting to Release
-- CMAKE_BUILD_TYPE: Release

NO MOAB_VERSION specified. Defaulting to 5.5.1
MOAB_VERSION to be installed: 5.5.1
-- MOAB will be downloaded and built
HDF5_ROOT: /usr/lib/x86_64-linux-gnu/hdf5
MOAB_LIBRARY_DIRS: /DAGMC/moab/lib
CMAKE_SHARED_LIBRARY_SUFFIX: .so
CMake Error at /usr/share/cmake-3.22/Modules/ExternalProject.cmake:1754 (file):
  file problem creating directory: /DAGMC/moab
Call Stack (most recent call first):
  /usr/share/cmake-3.22/Modules/ExternalProject.cmake:3668 (_ep_set_directories)
  cmake/MOAB_PullAndMake.cmake:25 (ExternalProject_Add)
  CMakeLists.txt:58 (moab_pull_make)


CMake Error at /usr/share/cmake-3.22/Modules/ExternalProject.cmake:1756 (message):
  dir '/DAGMC/moab' does not exist after file(MAKE_DIRECTORY)
Call Stack (most recent call first):
  /usr/share/cmake-3.22/Modules/ExternalProject.cmake:3668 (_ep_set_directories)
  cmake/MOAB_PullAndMake.cmake:25 (ExternalProject_Add)
  CMakeLists.txt:58 (moab_pull_make)


-- Configuring incomplete, errors occurred!
See also "/home/rherrero/code_development/DAGMC/build/CMakeFiles/CMakeOutput.log".
make: *** No rule to make target 'install'.  Stop.

bam241 · 2025-02-03T17:12:35Z

Hello @rherrero-pf thanks for testing my working
I am wondering if the problem doesn't come from -DCMAKE_INSTALL_PREFIX=/DAGMC/
are you sure you have the right to write in the /DAGMC folder ?
folder at / are usually protected.

the error you get is: file problem creating directory: /DAGMC/moab

shimwell · 2025-02-04T10:52:04Z

Thanks @bam241

confirming this slightly adapted version works for me

sudo apt-get install libhdf5-serial-dev
sudo apt install libeigen3-dev
rm -rf ~/code_development/DAGMC
mkdir -p ~/code_development/DAGMC
cd ~/code_development/DAGMC
git clone --single-branch --branch develop --depth 1 https://github.com/svalinn/DAGMC.git
mkdir build
cd build
cmake ../DAGMC -DBUILD_TALLY=ON \
              -DBUILD_STATIC_EXE=OFF \
              -DBUILD_STATIC_LIBS=OFF \
              -DCMAKE_INSTALL_PREFIX=$HOME/code_development/DAGMC/ \
              -DBUILD_UWUW=OFF \
              -DPULL_INSTALL_MOAB=5.5.1 \
              -DHDF5_ROOT=/usr/lib/x86_64-linux-gnu/hdf5/serial \
              -DEIGEN3_INCLUDE_DIR=/usr/include/eigen3
# in my case I need to help it find Eigen3 with the line above, hopefully not everyone has to do that?
make -j
make -j install

then when installing openmc one needs to pass the DAGMC path to openmc like this

cd ~/code_development
git clone --single-branch --branch develop --depth 1 https://github.com/openmc-dev/openmc.git
cd openmc
mkdir build
cd build
cmake -DOPENMC_USE_DAGMC=ON -DDAGMC_ROOT=$HOME/code_development/DAGMC -DHDF5_PREFER_PARALLEL=OFF ..
make -j
make -j install
cd ..
python -m pip install .

*updated now that the PR is merged to point to svalinn repo and also includes full openmc instructions

gonuke

looks pretty good - a few final clarifications

gonuke · 2025-02-04T11:24:56Z

CMakeLists.txt

+  set(MOAB_VERSION ${PULL_INSTALL_MOAB})  
+
+  # Ensure MOAB_VERSION
+  if(${MOAB_VERSION})


It's not clear to me what conditions re true for this? It seems like it would evaluate to true if I DID set the version? but maybe I don't know how this CMake evaluates if(5.5.3), for example?

if(<variable>) True if given a variable that is defined to a value that is not a false constant. False otherwise, including if the variable is undefined. Note that macro arguments are not variables. ``` from https://cmake.org/cmake/help/latest/command/if.html

Is a version string evaluated as a "False" constant then? I would have assumed that a version string would evaluate as True and cause this message and default to happen??

you are right I miss understood this. I'll fix it!

@gonuke this should take care of it !

cmake/FindMOAB.cmake

doc/CHANGELOG.rst

Co-authored-by: Paul Wilson <[email protected]>

bam241 · 2025-02-04T15:05:46Z

CMakeLists.txt

+  if(NOT ${MOAB_VERSION} MATCHES "^[0-9]+\\.[0-9]+\\.[0-9]")
+    MESSAGE("MOAB_VERSION: ${MOAB_VERSION} not recognized. Defaulting to 5.5.1")
+    set(MOAB_VERSION "5.5.1")
+  endif()


This should enforce MOAB_VERSION to be shaped as X.Y.Z with X, Y, Z as numbers.

Does this work even if we have a number that takes 2 chars, e.g. 5.5.10

maybe not I'll try and fix it if not

gonuke

Suggested fix for number regex

CMakeLists.txt

Co-authored-by: Paul Wilson <[email protected]>

gonuke

Sorry, but you seem to have messed up some indentation... just want that fixed, please.

gonuke · 2025-02-05T01:47:34Z

cmake/DAGMC_macros.cmake

+    if(PULL_INSTALL_MOAB)
+    target_link_libraries(${lib_name}-shared INTERFACE ${MOAB_LIBRARY_DIRS}/libMOAB${CMAKE_SHARED_LIBRARY_SUFFIX})
+    add_dependencies(${lib_name}-shared MOAB)
+     endif()


Can you fix this indentation, please?

gonuke

LGTM - I'd be happy to have one other person take a last look... @pshriwise ?

pshriwise · 2025-02-05T20:59:00Z

Gave this another look today. Thanks @bam241!

gonuke · 2025-02-06T03:04:34Z

This caused a test to fail on merge. We have a test that checks to see if we are compatible with the develop branch of HDF5. I am not sure what changed to make this fail - whether it was the HDF5 code changing or something in this build?

bam241 · 2025-02-06T07:00:26Z

I don't see the failing test.
could you provide a link to it ?
I'll keep looking as well

bam241 · 2025-02-06T07:02:08Z

found it

gonuke · 2025-02-06T11:09:04Z

It doesn't really matter now, I don't think, since tests pass with the specific version of HDF5, but it may be a sign for the future?

bam241 · 2025-02-06T11:10:35Z

I think I am the one who broke it:

bam241#4

bam241 · 2025-02-06T11:11:05Z

I revert the change I did on the findMOAB.cmake (responsible to deal with HDF5) and it works

bam241 commented Dec 4, 2024

View reviewed changes

src/pyne/CMakeLists.txt Outdated Show resolved Hide resolved

bam241 commented Dec 4, 2024

View reviewed changes

src/dagmc/CMakeLists.txt Outdated Show resolved Hide resolved

bam241 commented Dec 4, 2024

View reviewed changes

jon-proximafusion mentioned this pull request Dec 5, 2024

updated cmake min version to 3.18 #970

Closed

shimwell reviewed Dec 5, 2024

View reviewed changes

cmake/FindMOAB.cmake Outdated Show resolved Hide resolved

shimwell reviewed Dec 5, 2024

View reviewed changes

cmake/FindMOAB.cmake Outdated Show resolved Hide resolved

bam241 force-pushed the auto_moab branch from 8761cb8 to c20a90e Compare December 6, 2024 05:31

bam241 marked this pull request as ready for review December 6, 2024 06:42

bam241 requested a review from gonuke December 6, 2024 06:54

bam241 force-pushed the auto_moab branch 2 times, most recently from d49d208 to 45feb89 Compare December 6, 2024 11:32

ahnaf-tahmid-chowdhury reviewed Dec 6, 2024

View reviewed changes

jon-proximafusion reviewed Dec 12, 2024

View reviewed changes

cmake/MOAB_PullAndMake.cmake Outdated Show resolved Hide resolved

jon-proximafusion reviewed Dec 12, 2024

View reviewed changes

cmake/MOAB_PullAndMake.cmake Outdated Show resolved Hide resolved

bam241 force-pushed the auto_moab branch from 4be94aa to 17d11d1 Compare December 12, 2024 10:58

shimwell mentioned this pull request Dec 12, 2024

Fetch DAGMC if not found in cmakelists.txt openmc-dev/openmc#3193

Draft

5 tasks

bam241 force-pushed the auto_moab branch from b86a8a6 to 6675cef Compare January 9, 2025 14:20

gonuke requested changes Jan 9, 2025

View reviewed changes

pshriwise previously approved these changes Jan 9, 2025

View reviewed changes

pshriwise reviewed Jan 9, 2025

View reviewed changes

bam241 dismissed pshriwise’s stale review via 19da17b January 10, 2025 12:02

pshriwise reviewed Jan 24, 2025

View reviewed changes

cmake/MOAB_PullAndMake.cmake Outdated Show resolved Hide resolved

pshriwise reviewed Jan 24, 2025

View reviewed changes

cmake/MOAB_PullAndMake.cmake Show resolved Hide resolved

Update MOAB_PullAndMake.cmake

4e71015

Co-authored-by: Patrick Shriwise <[email protected]>

ahnaf-tahmid-chowdhury reviewed Jan 27, 2025

View reviewed changes

shimwell mentioned this pull request Jan 28, 2025

Adding moab download if not found via fetch content #964

Closed

gonuke requested changes Feb 4, 2025

View reviewed changes

bam241 and others added 4 commits February 4, 2025 12:36

Apply suggestions from code review

e022d74

Co-authored-by: Paul Wilson <[email protected]>

fixing MOAB version

b070148

fixing regex to only accept number shaped as X.Y.Z

814933e

english

383ddb2

bam241 commented Feb 4, 2025

View reviewed changes

gonuke requested changes Feb 4, 2025

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

bam241 and others added 2 commits February 4, 2025 21:10

some improvement

4d22c95

Update CMakeLists.txt

141bb0b

Co-authored-by: Paul Wilson <[email protected]>

gonuke requested changes Feb 5, 2025

View reviewed changes

restoring indent

56e8c13

gonuke approved these changes Feb 5, 2025

View reviewed changes

pshriwise approved these changes Feb 5, 2025

View reviewed changes

gonuke merged commit 10b1451 into svalinn:develop Feb 6, 2025
20 checks passed

Automatic download and build of MOAB #969

Automatic download and build of MOAB #969

Conversation

bam241 commented Dec 4, 2024 • edited Loading

Description

Motivation and Context

Changes

Behavior

Changelog file

bam241 commented Dec 4, 2024

Choose a reason for hiding this comment

bam241 commented Dec 4, 2024 • edited Loading

bam241 commented Dec 6, 2024

ahnaf-tahmid-chowdhury 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

jon-proximafusion commented Dec 10, 2024 • edited by shimwell Loading

bam241 commented Dec 12, 2024

jon-proximafusion commented Jan 9, 2025 • edited by shimwell Loading

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshriwise left a comment

Choose a reason for hiding this comment

bam241 commented Jan 9, 2025

makeclean commented Jan 10, 2025

bam241 commented Jan 10, 2025

shimwell commented Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rherrero-pf commented Feb 3, 2025

bam241 commented Feb 3, 2025 • edited Loading

shimwell commented Feb 4, 2025 • edited Loading

gonuke 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

gonuke left a comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonuke left a comment

Choose a reason for hiding this comment

pshriwise commented Feb 5, 2025

gonuke commented Feb 6, 2025

bam241 commented Feb 6, 2025

bam241 commented Feb 6, 2025

gonuke commented Feb 6, 2025

bam241 commented Feb 6, 2025

bam241 commented Feb 6, 2025

bam241 commented Dec 4, 2024 •

edited

Loading

bam241 commented Dec 4, 2024 •

edited

Loading

jon-proximafusion commented Dec 10, 2024 •

edited by shimwell

Loading

jon-proximafusion commented Jan 9, 2025 •

edited by shimwell

Loading

shimwell commented Jan 24, 2025 •

edited

Loading

bam241 commented Feb 3, 2025 •

edited

Loading

shimwell commented Feb 4, 2025 •

edited

Loading