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.

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

Linker flags for common libraries #19

Merged
merged 22 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d7c66c6
Add simple getter for linker library flags
lukehoffmann Aug 6, 2024
0340a27
Add getter for linker flags by library
lukehoffmann Aug 21, 2024
4cc5485
Fix formatting
lukehoffmann Aug 21, 2024
f017eed
Add optional libs argument to link function
lukehoffmann Aug 21, 2024
4f62bfd
Reorder and clean up linker tests
lukehoffmann Aug 22, 2024
1caf110
Make sure `Linker.link()` raises for unknown lib
lukehoffmann Aug 22, 2024
dd81dbe
Add missing type
lukehoffmann Aug 22, 2024
8d72ccd
Fix typing error
lukehoffmann Aug 22, 2024
b3021d6
Add 'libs' argument to link_exe function
lukehoffmann Aug 29, 2024
c9b4183
Try to add documentation for the linker libs feature
lukehoffmann Aug 29, 2024
390a2fb
Use correct list type in link_exe hint
lukehoffmann Aug 30, 2024
d8c9db1
Add silent replace option to linker.add_lib_flags
lukehoffmann Aug 30, 2024
c612aae
Merge remote-tracking branch 'origin/bom_master' into linker-lib-flags
hiker Sep 5, 2024
649b9ff
Fixed failing test triggered by executing them in specific order (too…
hiker Sep 5, 2024
b0e5842
Fixed line lengths.
hiker Sep 5, 2024
0645816
Add tests for linker LDFLAG
lukehoffmann Sep 11, 2024
728089e
Add pre- and post- lib flags to link function
lukehoffmann Sep 11, 2024
76322a1
Fix syntax in built-in lib flags
lukehoffmann Sep 23, 2024
58007e8
Remove netcdf as a built-in linker library
lukehoffmann Sep 23, 2024
7286200
Configure pre- and post-lib flags on the Linker object
lukehoffmann Sep 23, 2024
c6e3b42
Use more realistic linker lib flags
lukehoffmann Sep 23, 2024
f7e40f3
Formatting fix
lukehoffmann Sep 23, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Documentation/source/advanced_config.rst
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,14 @@ to specify 3rd party libraries at the link stage.

link_exe(state, flags=['-lm', '-lnetcdf'])

Linkers will be pre-configured with flags for common libraries. Where possible,
a library name should be used to include the required flags for linking.

.. code-block::
:linenos:

link_exe(state, libs=['netcdf'])

Path-specific flags
-------------------

Expand Down
17 changes: 12 additions & 5 deletions source/fab/steps/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"""
import logging
from string import Template
from typing import Optional
from typing import List, Optional, Union

from fab.artefacts import ArtefactSet
from fab.steps import step
Expand All @@ -33,7 +33,10 @@ def __call__(self, artefact_store):


@step
def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
def link_exe(config,
libs: Union[List[str], None] = None,
flags: Union[List[str], None] = None,
source: Optional[ArtefactsGetter] = None):
"""
Link object files into an executable for every build target.

Expand All @@ -49,8 +52,10 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
The :class:`fab.build_config.BuildConfig` object where we can read
settings such as the project workspace folder or the multiprocessing
flag.
:param libs:
A list of required library names to pass to the linker.
:param flags:
A list of flags to pass to the linker.
A list of additional flags to pass to the linker.
:param source:
An optional :class:`~fab.artefacts.ArtefactsGetter`. It defaults to the
output from compiler steps, which typically is the expected behaviour.
Expand All @@ -59,13 +64,15 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
linker = config.tool_box.get_tool(Category.LINKER, config.mpi)
logger.info(f'Linker is {linker.name}')

libs = libs or []
flags = flags or []
source_getter = source or DefaultLinkerSource()

target_objects = source_getter(config.artefact_store)
for root, objects in target_objects.items():
exe_path = config.project_workspace / f'{root}'
linker.link(objects, exe_path, openmp=config.openmp, add_libs=flags)
linker.link(objects, exe_path, openmp=config.openmp, libs=libs,
post_lib_flags=flags)
config.artefact_store.add(ArtefactSet.EXECUTABLES, exe_path)


Expand Down Expand Up @@ -115,4 +122,4 @@ def link_shared_object(config, output_fpath: str, flags=None,

objects = target_objects[None]
out_name = Template(output_fpath).substitute(output=config.build_output)
linker.link(objects, out_name, openmp=config.openmp, add_libs=flags)
linker.link(objects, out_name, openmp=config.openmp, post_lib_flags=flags)
69 changes: 64 additions & 5 deletions source/fab/tools/linker.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@

import os
from pathlib import Path
from typing import cast, List, Optional
from typing import cast, Dict, List, Optional
import warnings

from fab.tools.category import Category
from fab.tools.compiler import Compiler
Expand Down Expand Up @@ -51,6 +52,14 @@ def __init__(self, name: Optional[str] = None,
self._compiler = compiler
self.flags.extend(os.getenv("LDFLAGS", "").split())

# Maintain a set of flags for common libraries.
self._lib_flags: Dict[str, List[str]] = {}
# Include netcdf as an example, since it is reasonable portable
self.add_lib_flags(
'netcdf',
['$(nf-config --flibs)', '($nc-config --libs)']
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, but the second entry should be $(nc-config...) (i.e. switch $ and (.
But even worse: this actually does not work, because atm we don't use a shell when starting a process, we only fork ... and as a result the (shell) $(...) is not executed.
It's easy enough to add that flat (and instead of a list of parameters, we supply a string), but I better check that with Matthew. It might break something subtly (because parameters that contain a string now need quoting ... not that I think we have any spaces anywhere :) ).
So for now, just remove the whole setting of netcdf. I will add this using a proper shell script in the site-specific/default config. and discuss this with Matthew.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)

@property
def mpi(self) -> bool:
''':returns: whether the linker supports MPI or not.'''
Expand All @@ -66,16 +75,61 @@ def check_available(self) -> bool:

return super().check_available()

def get_lib_flags(self, lib: str) -> List[str]:
'''Gets the standard flags for a standard library

:param lib: the library name

:returns: a list of flags

:raises RuntimeError: if lib is not recognised
'''
try:
return self._lib_flags[lib]
except KeyError:
raise RuntimeError(f"Unknown library name: '{lib}'")

def add_lib_flags(self, lib: str, flags: List[str],
silent_replace: bool = False):
'''Add a set of flags for a standard library

:param lib: the library name
:param flags: the flags to use with the library
:param silent_replace: if set, no warning will be printed when an
existing lib is overwritten.
'''
if lib in self._lib_flags and not silent_replace:
warnings.warn(f"Replacing existing flags for library {lib}: "
f"'{self._lib_flags[lib]}' with "
f"'{flags}'.")

# Make a copy to avoid modifying the caller's list
self._lib_flags[lib] = flags[:]

def remove_lib_flags(self, lib: str):
'''Add a set of flags for a standard library

:param lib: the library name
'''
try:
del self._lib_flags[lib]
except KeyError:
pass

def link(self, input_files: List[Path], output_file: Path,
openmp: bool,
add_libs: Optional[List[str]] = None) -> str:
pre_lib_flags: Optional[List[str]] = None,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we had a misunderstanding :( I want to have the pre- and post-flags for the linker object, not the link step (since the link step ideally should be as independent from the linker and site used as possible, we now would have to specify site- and linker/compiler-specific paths here.

I'll add more details to the actual ticket #7 (it's just more convenient to write there :) )

libs: Optional[List[str]] = None,
post_lib_flags: Optional[List[str]] = None) -> str:
'''Executes the linker with the specified input files,
creating `output_file`.

:param input_files: list of input files to link.
:param output_file: output file.
:param openm: whether OpenMP is requested or not.
:param add_libs: additional linker flags.
:param pre_lib_flags: additional linker flags to use before libs.
:param libs: additional libraries to link with.
:param post_lib_flags: additional linker flags to use after libs.

:returns: the stdout of the link command
'''
Expand All @@ -88,7 +142,12 @@ def link(self, input_files: List[Path], output_file: Path,
params = []
# TODO: why are the .o files sorted? That shouldn't matter
params.extend(sorted(map(str, input_files)))
if add_libs:
params += add_libs

if pre_lib_flags:
params.extend(pre_lib_flags)
for lib in (libs or []):
params.extend(self.get_lib_flags(lib))
if post_lib_flags:
params.extend(post_lib_flags)
params.extend([self._output_flag, str(output_file)])
return self.run(params)
25 changes: 15 additions & 10 deletions tests/unit_tests/steps/test_archive_objects.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ def test_for_exes(self):

# ensure the correct artefacts were created
assert config.artefact_store[ArtefactSet.OBJECT_ARCHIVES] == {
target: set([str(config.build_output / f'{target}.a')]) for target in targets}
target: set([str(config.build_output / f'{target}.a')])
for target in targets}

def test_for_library(self):
'''As used when building an object archive or archiving before linking
Expand All @@ -65,32 +66,36 @@ def test_for_library(self):
mock_result = mock.Mock(returncode=0, return_value=123)
with mock.patch('fab.tools.tool.subprocess.run',
return_value=mock_result) as mock_run_command, \
pytest.warns(UserWarning, match="_metric_send_conn not set, cannot send metrics"):
archive_objects(config=config, output_fpath=config.build_output / 'mylib.a')
pytest.warns(UserWarning, match="_metric_send_conn not set, "
"cannot send metrics"):
archive_objects(config=config,
output_fpath=config.build_output / 'mylib.a')

# ensure the correct command line calls were made
mock_run_command.assert_called_once_with([
'ar', 'cr', str(config.build_output / 'mylib.a'), 'util1.o', 'util2.o'],
'ar', 'cr', str(config.build_output / 'mylib.a'), 'util1.o',
'util2.o'],
capture_output=True, env=None, cwd=None, check=False)

# ensure the correct artefacts were created
assert config.artefact_store[ArtefactSet.OBJECT_ARCHIVES] == {
None: set([str(config.build_output / 'mylib.a')])}

def test_incorrect_tool(self):
def test_incorrect_tool(self, mock_c_compiler):
'''Test that an incorrect archive tool is detected
'''

config = BuildConfig('proj', ToolBox(), mpi=False, openmp=False)
tool_box = config.tool_box
cc = tool_box.get_tool(Category.C_COMPILER, config.mpi)
# And set its category to C_COMPILER
cc = mock_c_compiler
# And set its category to be AR
cc._category = Category.AR
# So overwrite the C compiler with the re-categories Fortran compiler
# Now add this 'ar' tool to the tool box
tool_box.add_tool(cc)

with pytest.raises(RuntimeError) as err:
archive_objects(config=config,
output_fpath=config.build_output / 'mylib.a')
assert ("Unexpected tool 'gcc' of type '<class "
"'fab.tools.compiler.Gcc'>' instead of Ar" in str(err.value))
assert ("Unexpected tool 'mock_c_compiler' of type '<class "
"'fab.tools.compiler.CCompiler'>' instead of Ar"
in str(err.value))
8 changes: 6 additions & 2 deletions tests/unit_tests/steps/test_link.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,20 @@ def test_run(self, tool_box):
linker = Linker("mock_link", "mock_link.exe", "mock-vendor")
# Mark the linker as available to it can be added to the tool box
linker._is_available = True

# Add a custom library to the linker
linker.add_lib_flags('mylib', ['-L/my/lib', '-mylib'])
tool_box.add_tool(linker, silent_replace=True)
mock_result = mock.Mock(returncode=0, stdout="abc\ndef".encode())
with mock.patch('fab.tools.tool.subprocess.run',
return_value=mock_result) as tool_run, \
pytest.warns(UserWarning,
match="_metric_send_conn not "
"set, cannot send metrics"):
link_exe(config, flags=['-fooflag', '-barflag'])
link_exe(config, libs=['mylib'], flags=['-fooflag', '-barflag'])

tool_run.assert_called_with(
['mock_link.exe', '-L/foo1/lib', '-L/foo2/lib', 'bar.o', 'foo.o',
'-fooflag', '-barflag', '-o', 'workspace/foo'],
'-L/my/lib', '-mylib', '-fooflag', '-barflag',
'-o', 'workspace/foo'],
capture_output=True, env=None, cwd=None, check=False)
Loading