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

ld flags used in offline build are incorrect #198

Closed
SeanBryan51 opened this issue Jan 10, 2024 · 0 comments · Fixed by #199
Closed

ld flags used in offline build are incorrect #198

SeanBryan51 opened this issue Jan 10, 2024 · 0 comments · Fixed by #199
Assignees
@SeanBryan51

Comments

@SeanBryan51
Copy link
Collaborator

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. Note: if linking against a static library then -lnetcdf as well as other libraries may be required - see 1.7 Compiling and Linking with the NetCDF Library).

    CABLE/src/offline/build3.sh

    Line 23 in 1c8a2cb

    export LD='-lnetcdf -lnetcdff'

  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).

    CABLE/src/offline/build3.sh

    Lines 22 to 23 in 1c8a2cb

    export LDFLAGS='-L'$NCDIR' -O0'
    export LD='-lnetcdf -lnetcdff'

    Having the flags split across two variables allows for the flags to be passed to the compiler in an inconsistent way. For example, the flags in LDFLAGS (and LD for the serial case) are specified before the source files:

    CABLE/src/offline/Makefile

    Lines 163 to 169 in 1c8a2cb

    .PHONY: serial
    serial: cable_driver.F90 $(OBJS)
    $(FC) $(SUPPRESS_FLAGS) $(CFLAGS) $(LDFLAGS) $(LD) -o $(PROG_SERIAL) $^ $(CINC)
    .PHONY: mpi
    mpi: cable_mpidrv.F90 cable_mpicommon.o cable_mpimaster.o cable_mpiworker.o pop_mpi.o $(OBJS)
    $(FC) $(SUPPRESS_FLAGS) $(CFLAGS) $(LDFLAGS) -o $(PROG_MPI) $^ $(CINC) $(LD)

    ld flags should generally be specified after the source files (see this SO post for justification).

A future proof solution would be to not hard code linker flags in the build system and instead use a tool such as pkg-config to get the correct flags for a given dependency (see here as an example).
@SeanBryan51 SeanBryan51 mentioned this issue Jan 10, 2024
Closed
4 tasks
@SeanBryan51 SeanBryan51 self-assigned this Jan 21, 2024
SeanBryan51 added a commit that referenced this issue Jan 22, 2024
@SeanBryan51
Fix LDFLAGS definition and usage
f2861b4 
`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
@SeanBryan51 SeanBryan51 mentioned this issue Jan 22, 2024
Merged
@SeanBryan51 SeanBryan51 linked a pull request Jan 22, 2024 that will close this issue
Fix LDFLAGS definition and usage #199
Merged
@SeanBryan51 SeanBryan51 mentioned this issue Jan 22, 2024
Merged
2 tasks
SeanBryan51 added a commit that referenced this issue Jan 23, 2024
@SeanBryan51
Fix LDFLAGS definition and usage
a13c268 
`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
SeanBryan51 added a commit that referenced this issue Jan 24, 2024
@SeanBryan51
Fix LDFLAGS definition and usage
56954e3 
`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
SeanBryan51 added a commit that referenced this issue Jan 25, 2024
@SeanBryan51
Fix LDFLAGS definition and usage
34d89ce 
`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
SeanBryan51 added a commit that referenced this issue Jan 31, 2024
@SeanBryan51
Fix LDFLAGS definition and usage
379a9cd 
`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
SeanBryan51 added a commit 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 May 17, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@SeanBryan51 SeanBryan51

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix LDFLAGS definition and usage CABLE-LSM/CABLE
1 participant
@SeanBryan51