Updating BoxLib with the ability to build again.#2813
Updating BoxLib with the ability to build again.#2813tgamblin merged 4 commits intospack:developfrom jrood-nrel:update_boxlib
Conversation
|
|
||
| homepage = "https://ccse.lbl.gov/BoxLib/" | ||
| url = "https://ccse.lbl.gov/pub/Downloads/BoxLib.git" | ||
| base_url = "https://github.com/BoxLib-Codes/BoxLib.git" |
There was a problem hiding this comment.
Is base_url used for anything?
There was a problem hiding this comment.
Nope, just was looking at another package as an example.
| '-DENABLE_FBASELIB=ON', | ||
| '-DCMAKE_C_COMPILER=%s' % spec['mpi'].prefix.bin + "/mpicc", | ||
| '-DCMAKE_CXX_COMPILER=%s' % spec['mpi'].prefix.bin + "/mpic++", | ||
| '-DCMAKE_Fortran_COMPILER=%s' % spec['mpi'].prefix.bin + "/mpif90" |
There was a problem hiding this comment.
Can you actually use spec['mpi'].mpicc, etc.? You can see where they are defined (and what they are named) in the setup_dependent_package method of any MPI package.
There was a problem hiding this comment.
Sure, another case of me looking at the first package I can think of (SAMRAI) as an example as to how to specify the MPI compiler.
|
|
||
| homepage = "https://ccse.lbl.gov/BoxLib/" | ||
| url = "https://ccse.lbl.gov/pub/Downloads/BoxLib.git" | ||
| url = "https://github.com/BoxLib-Codes/BoxLib.git" |
There was a problem hiding this comment.
I think it would be good to use an actual download tarball for the URL. There seem to be plenty for this repository. The latest is:
https://github.com/BoxLib-Codes/BoxLib/archive/16.12.2.tar.gz
| '-DCMAKE_C_COMPILER=%s' % spec['mpi'].mpicc, | ||
| '-DCMAKE_CXX_COMPILER=%s' % spec['mpi'].mpicxx, | ||
| '-DCMAKE_Fortran_COMPILER=%s' % spec['mpi'].mpifc | ||
| ]) |
There was a problem hiding this comment.
Instead of doing:
options = []
options.extend([
...
])why not do:
options = [
...
]or even:
return [
...
]There was a problem hiding this comment.
I personally feel that options.extend() has become a typical convention in the package files. It's a minor thing, but I think it sets the stage for someone in the future to add or ignore CMake configure commands based on variants for example. Where a return would go away immediately if any logic were to be implemented. I would also disagree with setting options directly to the configure arguments, because the way it is currently written will appear more uniform in the future if logic was added for customization options, and the first setting of options will be the odd man out.
3 participants
Boxlib was out-of-date and the git url was broken. I have taken the liberty of updating it so that it at least can build something. Having to choose whether it builds the 2D or 3D version or to build both will take some future work.