[JOSS Review]: Consider making build installable

[matthewfeickert]

With regards to openjournals/joss-reviews#7525 (comment), a suggested revision:

The current code seems to make strong assumptions about the user being on a Ubuntu release, which should not be required as system level packages can be usefully recommended but build requirements should be scoped to the individual software level, not their distributions for different flavors of Linux. This was brought up in Issue #37 and addressed in https://github.com/prashjha/PeriDEM/blob/8f73cbd12fbf8d483bb46670661f9f2641d03c37/tools/README.md.

That is all helpful, but the following Pixi manifest (in the top level of the repository)

[workspace]
channels = ["conda-forge"]
name = "PeriDEM"
platforms = ["linux-64"]
version = "0.1.0"

# Currently can't install, only build?
# c.f. https://github.com/prashjha/PeriDEM/blob/8f73cbd12fbf8d483bb46670661f9f2641d03c37/README.md?plain=1#L264
[tasks.build]
cmd = """
rm -rf build && \
cmake   -DEnable_Documentation=OFF \
        -DEnable_Tests=ON \
        -DEnable_High_Load_Tests=OFF \
        -DDisable_Docker_MPI_Tests=ON \
        -DVTK_DIR="$CONDA_PREFIX" \
        -DMETIS_DIR="$CONDA_PREFIX" \
        -DCMAKE_BUILD_TYPE=Release \
        -DCMAKE_INSTALL_PREFIX="$CONDA_PREFIX" \
        -S . \
        -B build && \
cmake build -LH && \
cmake --build build --clean-first --parallel "$(nproc --ignore=2)" && \
ctest --verbose --test-dir build/
"""

[dependencies]
cxx-compiler = ">=1.11.0,<2"
cmake = "<4"
make = ">=4.4.1,<5"
vtk = ">=9.5.2,<10"
yaml-cpp = ">=0.8.0,<0.9"
metis = ">=5.2.1,<6"
openmpi = ">=5.0.8,<6"
gmsh = ">=4.13.1,<5"  # for tests

is sufficient (given the current listed dependencies) to build and test the library and makes no Linux distribution assumptions beyond the computing platform

$ pixi run build
...
[ 98%] Built target TestPeriDEM
[100%] Linking CXX executable bin/PeriDEM
[100%] Built target PeriDEM
...
100% tests passed, 0 tests failed out of 16

so while I think your suggestions are good, I don't necessarily know if you want to tie yourself to particular Linux distributions or package managers

PeriDEM/tools/README.md

Line 6 in 8f73cbd

* recommend to install using `apt-get` or `brew`

You could probably free yourself up to just report what you do and make it explicit what you expect users to install for themselves.

It would be a boon in general though if the code were installable to arbitrary user choice of directory tree, which is seems it currently is not

PeriDEM/README.md

Lines 264 to 267 in 8f73cbd

	> :exclamation: As of now, we can only build the library and not install it.
	This means you will have to build the package in the `build` directory, and
	use the `build/bin/PeriDEM` executable. We plan to provide the method to `install`
	the library in the future.

That would be great to see.

Also, using

PeriDEM/CMakeLists.txt

Line 9 in 8f73cbd

cmake_minimum_required(VERSION 3.10)

is problematic as it forces the exact CMake policy regardless of the CMake version installed, and disallows using features from newer CMake releases if PeriDEM was going to be built as part of another project. CMake <3.10 have already having support dropped in CMake v4.3, and so it is possible that in the very near future users would be required to downgrade their CMake installation to even be able to build PeriDEM.

As mentioned in the cmake_minimum_required docs, defining a <policy_max>

$ git diff -- CMakeLists.txt
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 44031a98..53e00b8a 100755
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -6,7 +6,7 @@
 # Distributed under the Boost Software License, Version 1.0. (See accompanying
 # file LICENSE)
 
-cmake_minimum_required(VERSION 3.10)
+cmake_minimum_required(VERSION 3.10...4.3)
 
 #----------------------------------#
 #<<<<<<<<<< Project name >>>>>>>>>>#

allows for this to be avoided and incrementally tested, and pick up warnings in advance (e.g. the ones that are present here).

[github-actions]

Hey # thanks for reporting an issue! We will try to get in touch with you soon.

[matthewfeickert]

As mentioned in #37 (comment), it would

be nice to be able to install headers and libraries

This should be relatively straightforward to add by following the CMake docs tutorial: https://cmake.org/cmake/help/latest/guide/tutorial/Installation%20Commands%20and%20Concepts.html.

I would also recommend my colleague @henryiii's excellent (open source web) book: An Introduction to Modern CMake. Specifically, the Exporting and Installing chapter.

[prashjha]

Hi @matthewfeickert, I am working through this now. Thank you for taking the time to provide these suggestions.

[prashjha]

Hi @matthewfeickert, thank you for the excellent suggestion to use Pixi.

I tried it here, and the code works cleanly on Mac. I will verify that this also works on Ubuntu, and then work on the install option.