-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Fix LDFLAGS definition and usage #199
Fix LDFLAGS definition and usage #199
Conversation
a13c268
to
56954e3
Compare
1e6d325
to
aaebf2a
Compare
@harshula Is |
Hi, Good question! My understanding is that it is a default package on GNU/Linux distros. |
This is more of a question around the CMake implementation (#200) but it is still relevant. If @harshula I see this was done for libaccessom2: ACCESS-NRI/libaccessom2@3311b58. Micael's CMake implementation for mom6 also uses |
With Spack, it's as simple as, |
It also looks like the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am approving but I need to check the portability of pkg_config
. We have at least one active developer in Europe who does not use Gadi. And I have no idea what's available on their system.
Depending on what comes out of the enquiry, further adaptation might be required.
Should the prepend_path
be guarded by a "if gadi" condition? Or do we only worry about that for the CMake version.
It is implied that anything under |
`ld` flags are currently specified incorrectly. For example: 1. `-lnetcdf` is not required since it is a dependency of the netCDF Fortran library - only `-lnetcdff` is needed. 2. `ld` flags are being split across two variables: `LDFLAGS` and `LD` when there should only be one (`LD` typically refers to the path to the `ld` executable). Having the flags split across two variables allows for the flags to be passed to the compiler in an inconsistent way. See #198 for more details. This change removes the `LD` variable and any hard-coded linker flags or include flags in the Makefile or build script. Linker flags and include flags are replaced with a query to `pkg-config` so that the build system is insulated from changes to library versions and dependencies. Fixes #198
af1f126
to
379a9cd
Compare
On Rocky Linux:
On RHEL clones, it's safe to conclude that pkg-config will be installed by default alongside well-known Development Tools. |
Aahh, the diff view in the review didn't show the |
Since I hadn't seen the almost entirety of the build script is within a |
@ccarouge it will be relevant for CMake as find_package(PkgConfig REQUIRED) |
In preparation for building CABLE with [spack](https://spack.io/), @harshula, @ccarouge and I have found areas where the current build system for CABLE offline can be improved (see #188 for more details). This change adds a [CMake](https://cmake.org/) based build system to address these issues: - #190 Although this problem can be solved in the Makefile by specifying the relative paths to each source file, it requires updating all the Makefile Fortran module dependency rules which is a pain. CMake automatically handles Fortran module dependencies out of the box so doing this with CMake is much easier. - #191 When invoking `cmake`, custom options, compilers and compiler flags can be configurable by setting the appropriate variables via the [`-D` flag](https://cmake.org/cmake/help/latest/manual/cmake.1.html#cmdoption-cmake-D), for example: `cmake -DCMAKE_Fortran_COMPILER="mpif90" -DCMAKE_Fortran_FLAGS="-O2 -fp-model precise" -S . -B build`. These can be used for doing MPI only builds and enabling specific debug flags. - #192 As mentioned above, CMake automatically handles Fortran module dependencies out of the box. Parallelised builds can be done by specifying the `-j` flag to `cmake`. The following pull requests should be merged before this pull request so that we can demonstrate binary equivalence between the executables compiled by both Makefile and CMake builds. This ensures no unwanted changes are introduced by the transition. - [x] #197 - [x] #199 Edit: the above PR's have been merged - the serial and MPI binaries built by CMake (in release mode) are equivalent to the binaries built by the Makefile based build system in the main branch. Fixes #188, #190, #191, #192 <!-- readthedocs-preview cable start --> ---- 📚 Documentation preview 📚: https://cable--200.org.readthedocs.build/en/200/ <!-- readthedocs-preview cable end -->
ld
flags are currently specified incorrectly. For example:-lnetcdf
is not required since it is a dependency of the netCDF Fortran library - only-lnetcdff
is needed.ld
flags are being split across two variables:LDFLAGS
andLD
when there should only be one (LD
typically refers to the path to theld
executable). Having the flags split across two variables allows for the flags to be passed to the compiler in an inconsistent way.See #198 for more details.
This change removes the
LD
variable and any hard-coded linker flags or include flags in the Makefile or build script. Linker flags and include flags are replaced with a query topkg-config
so that the correct flags are used. Usingpkg-config
also insulates the build system from changes to library versions and dependencies.This PR should ideally be done after #197 is merged so that we can verify compiled object files have not been altered as a result of this change.Compiled object files are identical between this branch and the main branch. However, the executables differ between the two branches due to changes in the compiler arguments at the linking step.
Regression tests using benchcab* (bitwise comparison of model output via nccmp) show that model output is bitwise identical between the current branch and the main branch for serial and MPI model runs.
*executables were built manually for the spatial case (see this related issue: CABLE-LSM/benchcab#244).
Fixes #198
📚 Documentation preview 📚: https://cable--199.org.readthedocs.build/en/199/