Linker flags for common libraries #19

lukehoffmann · 2024-08-22T06:18:55Z

Add support for libraries in the Linker class. As per issue #7:

In order to better support different paths on different sites:
The linker base class should provide a dictionary that maps strings (which are 'standard' library names) to a list of linker flags. E.g.
'xios' -> ["-L", "/bla/bla", "-l", "morebla"] 
'netcdf' -> ['$(nf-config --flibs)', '($nc-config --libs)`]`,
An application than only specifies which libraries it needs to link with (and in what order), and the linker then adds the correct flags during the linking stage.

If a library is specified that is unknown, raise an error.

There must be function to add and remove libraries (and Fab as default would likely provide none? Or Maybe just say netcdf as an example, since this is reasonable portable). Site-specific scripts will want to modify the settings (e.g.

lukehoffmann · 2024-08-22T06:37:37Z

source/fab/steps/link.py

@@ -65,7 +65,7 @@ def link_exe(config, flags=None, source: Optional[ArtefactsGetter] = None):
    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, add_flags=flags)


Should I include support for libs up at this level?

Yes please. Keep the flags parameter, while I can't think of a real good use case, you never know if an app might want/need to provide additional flags (hmm - maybe ask for linker mapping file or so ... there are enough linker options :) )

lukehoffmann · 2024-08-22T06:39:56Z

@hiker please let me know if I've misunderstood what you had in mind, or if not can you review please?

hiker · 2024-08-27T13:38:46Z

Looks perfect. Bring it up to the link_exe (and link_shared_object), and this is ready to go.

hiker · 2024-08-27T13:48:49Z

source/fab/tools/linker.py

+        except KeyError:
+            raise RuntimeError(f"Unknown library name: '{lib}'")
+
+    def add_lib_flags(self, lib: str, flags: List[str]):


Hmm - I just remembered: during the code review of the tool box, I was asked to add a silent_replace option: if there is already an entry defined that is overwritten, do a warnings.warn(...) to indicate that library settings get replaced (but if there is no entry, just add it as well).

Wasn't sure if you meant both the warning and the silent option, but I've added both since it's fairly simple

…ls then steps)

hiker · 2024-09-05T14:24:20Z

I've merged bom_masters into this and resolved the conflicts. I detected one test failure only triggered when the tests were executed in a certain order (in steps/archive) - I switched this test to use mock_c_compiler (which is always available), instead of the default in the tool box (which somehow changed when things were executed in a certain oder).

I also noticed that linesn66 and 147 are not covered by a test in test_linker:

~/work/fab/tests/unit_tests/tools$ pytest --cov-report term-missing --cov fab.tools.linker ./test_linker.py 
/home/joerg/.local/lib/python3.10/site-packages/pep8.py:110: FutureWarning: Possible nested set at position 1
  EXTRANEOUS_WHITESPACE_REGEX = re.compile(r'[[({] | []}),;:]')
============================================================================== test session starts ===============================================================================
platform linux -- Python 3.10.12, pytest-7.4.4, pluggy-1.0.0
rootdir: /home/joerg/work/fab
configfile: pyproject.toml
plugins: pylint-0.21.0, pep8-1.0.6, flakes-4.0.5, xdist-3.2.1, anyio-3.6.2, cov-4.0.0
collected 15 items                                                                                                                                                               

test_linker.py ...............                                                                                                                                             [100%]

---------- coverage: platform linux, python 3.10.12-final-0 ----------
Name                                              Stmts   Miss  Cover   Missing
-------------------------------------------------------------------------------
/home/joerg/work/fab/source/fab/tools/linker.py      59      2    97%   66, 147
-------------------------------------------------------------------------------
TOTAL                                                59      2    97%

I don't think this is an official requirement (line 66 is covered if we run all tests in unit_tests/tools, and line 147 somewhere from unit_tests/steps), but imho this is quite useful.

Now, line 66 is actually my fault I think (unless it was a side-effect of conflicts when merging??), and looking at it, this code is actually wrong (it relies on linker._compiler to exist, which atm does not have to be the case ... though de-facto it is). With #20 this will actually be fixed, so ... just ignore line 66 for now, but covering 147 looks useful

hiker · 2024-09-05T23:24:44Z

I think we need two extensions:

We need to have a list of flags that are always added before the library names (use case: all my libraries come from spack, but for whatever reasons I need to add their path using -L ... the only option to do this atm is to add this to every library, since I can't know in which order the future applications will add the library names). For this case, I can just add the paths once, and it will always be added.
Similarly, it might be good to have a list of flags to be added after the libraries. Use case: when linking xios with Fortran, you need to link explicitly with the stdc++ libraries. Since there could be more than one library that is based on c++, and a linker will link a library only once, being able to always specify this at the end is very useful

lukehoffmann · 2024-09-11T06:44:37Z

@hiker

We need to have a list of flags that are always added before the library names

Similarly, it might be good to have a list of flags to be added after the libraries

Currently the libs part of the link() function call.

Do you want to also put these pre/post flags into the link() function call, e.g.

linker.link(..., prelib_flags=[..], libs=[..], postlib_flags[..], ...)

Or should the be properties of the linker,

linker.prelib_flags = [ ... ]
linker.postlib_flags = [ ... ]
...
linker.link(..., libs=[..], ...)

Or do you want both options?

Currently we can technically do both with

linker.flags - always goes before libs (but also before the source files)
the add_flags argument - these flags always go after the libs flags

But the first is configured on the linker object, while the second is passed into link().

lukehoffmann · 2024-09-11T07:34:07Z

I decided to go with the option of adding both arguments to the link function. So these will be in addition to linker.flags. See what you think.

hiker · 2024-09-12T02:04:47Z

I decided to go with the option of adding both arguments to the link function. So these will be in addition to linker.flags. See what you think.

I admit I can't decide, and am happy with either (as long as these flags are all optional, since typically you won't need them). The only thing I did notice: flags that go before the source code (linker.flags) seems pretty useless. Any 'generic' linker flag can go anywhere (e.g. to ask for a symbol map file or so, or adding a search path), but any library must come after the .o files.

lukehoffmann · 2024-09-12T02:57:01Z

Yeah the linker.flags is just built-in behaviour of the Tool baseclass, which adds them in the run method. We will probably not use them, but I thought I'd better add a test for them since they exist

hiker · 2024-09-23T01:50:14Z

source/fab/tools/linker.py

+        # Include netcdf as an example, since it is reasonable portable
+        self.add_lib_flags(
+            'netcdf',
+            ['$(nf-config --flibs)', '($nc-config --libs)']


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.

See MetOffice#337

hiker

Back to you due to the misunderstanding of where to add the pre/post flag.

hiker · 2024-09-23T03:22:58Z

tests/unit_tests/tools/test_linker.py

+def test_linker_add_lib_flags(mock_c_compiler):
+    """Linker should provide a way to add a new set of flags for a library"""
+    linker = Linker(compiler=mock_c_compiler)
+    linker.add_lib_flags("xios", ["-L", "xios/lib", "-I", "xios/inc"])


Nit picking, but -I is not required for linking (it's the include path for compilation). Instead use "-l", "xios" together with -L...).

hiker · 2024-09-23T03:24:40Z

tests/unit_tests/tools/test_linker.py

+    linker = Linker(compiler=mock_c_compiler)
+    # netcdf is built in to the linker, since it is reasonable portable
+    result = linker.get_lib_flags("netcdf")
+    assert result == ["$(nf-config --flibs)", "($nc-config --libs)"]


That will affect quite a few tests, so I'll mention it only here: As mentioned above, remove netcdf lib. Instead modify the mock_linker in conftest.py to add some libraries there.

hiker · 2024-09-23T03:25:20Z

tests/unit_tests/tools/test_linker.py

+    # Replace them with another set of flags.
+    warn_message = 'Replacing existing flags for library netcdf'
+    with pytest.warns(UserWarning, match=warn_message):
+        linker.add_lib_flags("netcdf", ["-L", "netcdf/lib", "-I", "netcdf/inc"])


Same, replace with -l netcdf or so

hiker · 2024-09-23T03:29:20Z

source/fab/tools/linker.py

    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,


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

Bash-style substitution is not currently handled

Previously they were passed into the Linker.link() function

lukehoffmann · 2024-09-23T07:36:43Z

No problem, as I said I was weighing up both options, and just picked one option to implement for you to review. I've switched to configure the additional flags on the Linker itself now.

hiker · 2024-09-23T14:38:00Z

That looks good now, thanks. I'll port our lfric build to see if any other issue pops up :)

hiker

Looks nice, and works perfectly with our LFRic_atm-fab builds.

lukehoffmann added 6 commits August 21, 2024 14:21

Add simple getter for linker library flags

d7c66c6

Add getter for linker flags by library

0340a27

Fix formatting

4cc5485

Add optional libs argument to link function

f017eed

Reorder and clean up linker tests

4f62bfd

Make sure Linker.link() raises for unknown lib

1caf110

lukehoffmann requested a review from hiker August 22, 2024 06:18

lukehoffmann added 2 commits August 22, 2024 16:23

Add missing type

dd81dbe

Fix typing error

8d72ccd

lukehoffmann commented Aug 22, 2024

View reviewed changes

lukehoffmann added enhancement New feature or request Ready for review labels Aug 22, 2024

hiker reviewed Aug 27, 2024

View reviewed changes

lukehoffmann added 4 commits August 29, 2024 16:24

Add 'libs' argument to link_exe function

b3021d6

Try to add documentation for the linker libs feature

c9b4183

Use correct list type in link_exe hint

390a2fb

Add silent replace option to linker.add_lib_flags

d8c9db1

lukehoffmann requested a review from hiker August 30, 2024 07:41

hiker added 3 commits September 6, 2024 00:00

Merge remote-tracking branch 'origin/bom_master' into linker-lib-flags

c612aae

Fixed failing test triggered by executing them in specific order (too…

649b9ff

…ls then steps)

Fixed line lengths.

b0e5842

Add tests for linker LDFLAG

0645816

Add pre- and post- lib flags to link function

728089e

hiker reviewed Sep 23, 2024

View reviewed changes

hiker requested changes Sep 23, 2024

View reviewed changes

lukehoffmann added 5 commits September 23, 2024 15:01

Fix syntax in built-in lib flags

76322a1

Remove netcdf as a built-in linker library

58007e8

Bash-style substitution is not currently handled

Configure pre- and post-lib flags on the Linker object

7286200

Previously they were passed into the Linker.link() function

Use more realistic linker lib flags

c6e3b42

Formatting fix

f7e40f3

lukehoffmann requested a review from hiker September 23, 2024 07:34

hiker added under review The review is in progress and removed Ready for review labels Sep 23, 2024

hiker approved these changes Sep 24, 2024

View reviewed changes

hiker merged commit 282f068 into bom_master Sep 24, 2024
8 checks passed

hiker deleted the linker-lib-flags branch September 24, 2024 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linker flags for common libraries #19

Linker flags for common libraries #19

lukehoffmann commented Aug 22, 2024 •

edited

Loading

lukehoffmann Aug 22, 2024

hiker Aug 27, 2024 •

edited

Loading

lukehoffmann commented Aug 22, 2024

hiker commented Aug 27, 2024

hiker Aug 27, 2024

lukehoffmann Aug 30, 2024

hiker commented Sep 5, 2024

hiker commented Sep 5, 2024

lukehoffmann commented Sep 11, 2024 •

edited

Loading

lukehoffmann commented Sep 11, 2024

hiker commented Sep 12, 2024

lukehoffmann commented Sep 12, 2024 •

edited

Loading

hiker Sep 23, 2024

hiker Sep 23, 2024

hiker left a comment

hiker Sep 23, 2024

hiker Sep 23, 2024

hiker Sep 23, 2024

hiker Sep 23, 2024

lukehoffmann commented Sep 23, 2024

hiker commented Sep 23, 2024

hiker left a comment

Linker flags for common libraries #19

Linker flags for common libraries #19

Conversation

lukehoffmann commented Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

hiker Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

lukehoffmann commented Aug 22, 2024

hiker commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker commented Sep 5, 2024

hiker commented Sep 5, 2024

lukehoffmann commented Sep 11, 2024 • edited Loading

lukehoffmann commented Sep 11, 2024

hiker commented Sep 12, 2024

lukehoffmann commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukehoffmann commented Sep 23, 2024

hiker commented Sep 23, 2024

hiker left a comment

Choose a reason for hiding this comment

lukehoffmann commented Aug 22, 2024 •

edited

Loading

hiker Aug 27, 2024 •

edited

Loading

lukehoffmann commented Sep 11, 2024 •

edited

Loading

lukehoffmann commented Sep 12, 2024 •

edited

Loading