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

A better way of supporting legacy CABLE build systems #244

Closed
SeanBryan51 opened this issue Jan 23, 2024 · 2 comments
Closed

A better way of supporting legacy CABLE build systems #244

SeanBryan51 opened this issue Jan 23, 2024 · 2 comments
Assignees
@SeanBryan51
Labels
enhancement New feature or request priority:high High priority issues that should be included in the next release.

Comments

@SeanBryan51
Copy link
Collaborator

SeanBryan51 commented Jan 23, 2024 •
edited
Loading

We currently support building legacy CABLE versions via the build_script key, where a path to a build script can be provided for benchcab to execute at the build step. Some issues with this approach are:

  1. Scripts may require arguments or flags. It is currently not possible to specify flags to scripts, only the path to the script can be specified.
  2. Spatial simulations will require multiple executables to be built per version of CABLE (serial and mpi). To support legacy build systems, we will need a way specify multiple build commands for each executable.

This functionality will be needed as most development versions of CABLE will have different build systems, especially when there are significant developments to the CABLE build system for the main branch (e.g. CABLE-LSM/CABLE#200) .

Note, see here for a relevant discussion around this issue.
@SeanBryan51 SeanBryan51 added the enhancement New feature or request label Jan 23, 2024
@SeanBryan51 SeanBryan51 changed the title A better way of supporting legacy build systems in CABLE A better way of supporting legacy CABLE build systems Jan 23, 2024
This was referenced Jan 24, 2024
Merged
Closed
SeanBryan51 added a commit to CABLE-LSM/CABLE that referenced this issue Feb 28, 2024
@SeanBryan51
Fix LDFLAGS definition and usage (#199)
4be9a61 
`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 correct
flags are used. Using `pkg-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](https://github.com/CABLE-LSM/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

<!-- readthedocs-preview cable start -->
----
📚 Documentation preview 📚:
https://cable--199.org.readthedocs.build/en/199/

<!-- readthedocs-preview cable end -->
@SeanBryan51 SeanBryan51 mentioned this issue Mar 18, 2024
Closed
@SeanBryan51
Copy link
Collaborator Author

SeanBryan51 commented Mar 18, 2024 •
edited
Loading

We can add a new parameter that takes in a shell command (or a series of shell commands by specifying a multiline string) run from the root directory of the CABLE repository. For example:

build_run: |
  cd offline
  ./build.sh --some-flag

@SeanBryan51 SeanBryan51 added priority:medium Medium priority issues to become high priority issues after a release. priority:high High priority issues that should be included in the next release. and removed priority:medium Medium priority issues to become high priority issues after a release. labels Mar 21, 2024
@SeanBryan51 SeanBryan51 self-assigned this Mar 22, 2024
@SeanBryan51 SeanBryan51 mentioned this issue Mar 22, 2024
Closed
@SeanBryan51 SeanBryan51 linked a pull request Mar 22, 2024 that will close this issue
Add ability to run custom build commands #274
Closed
@SeanBryan51
Copy link
Collaborator Author

After discussing with @ccarouge, we have decided to add support for spack and to deprecate support for legacy CABLE build systems in benchcab. This adds the requirement that users will only be able to test benchcab on branches that can be built by spack. ACCESS-NRI will help with transitioning legacy build systems to use the latest build system compatible with spack.

Moving to spack will solve existing issues around supporting bespoke build scripts (e.g. #244, #249). This is the best way forward considering support for legacy build systems is a temporary that will be deprecated sooner or later.

Closing this issue.

@SeanBryan51 SeanBryan51 closed this as not planned Won't fix, can't repro, duplicate, stale Mar 22, 2024
@SeanBryan51 SeanBryan51 mentioned this issue Mar 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SeanBryan51 SeanBryan51

Labels
enhancement New feature or request priority:high High priority issues that should be included in the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add ability to run custom build commands CABLE-LSM/benchcab
1 participant
@SeanBryan51