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 inherits CompilerWrapper (#20) #23

Open
wants to merge 4 commits into
base: bom_master
Choose a base branch
from

Conversation

lukehoffmann
Copy link
Collaborator

Changes Linker to inherit CompilerWrapper as in Issue #20

@@ -29,9 +29,9 @@ def test_run(self, tool_box):
config.artefact_store[ArtefactSet.OBJECT_FILES] = \
{'foo': {'foo.o', 'bar.o'}}

with mock.patch('os.getenv', return_value='-L/foo1/lib -L/foo2/lib'):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was this test previously meant to mock LDFLAGS or FFLAGS?

Copy link
Owner

Choose a reason for hiding this comment

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

Leave this for now. I need a decision from the Fab devs if we can abandon support for environment variables (Fab should be free of that, any application ... like the Fab build system can decide to support this and provide the appropriate flags based on environment variables).

@@ -32,9 +32,9 @@ def test_run(tool_box):
config.artefact_store[ArtefactSet.OBJECT_FILES] = \
{None: {'foo.o', 'bar.o'}}

with mock.patch('os.getenv', return_value='-L/foo1/lib -L/foo2/lib'):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Was this test previously meant to mock LDFLAGS or FFLAGS?

Copy link
Owner

Choose a reason for hiding this comment

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

Same, imho this should all go.

@lukehoffmann
Copy link
Collaborator Author

This was my work on issue #20 before I saw the new comment about reusing flags with mpi wrappers.

Have a look and see if it would be useful in its current form.

@lukehoffmann lukehoffmann changed the title Linker as compiler wrapper (#20) Linker inherits CompilerWrapper (#20) Sep 25, 2024
@hiker
Copy link
Owner

hiker commented Sep 30, 2024

Seems to go in the right direction. But we need to support nested linker - e.g. an intel mpif90 wrapper would need to take a linker (which is now a CompilerWrapper) - linker-ifort as parameter. Then any flags from linker-ifort need to he used in the intel-mpif90 wrapper. I am not sure about the best way to implement this, here some tests with using isinstance:

joerg@Joerg-Surface:~/work/fab/source/fab/tools$ git diff ./linker.py
diff --git a/source/fab/tools/linker.py b/source/fab/tools/linker.py
index 1256ff3..7c60066 100644
--- a/source/fab/tools/linker.py
+++ b/source/fab/tools/linker.py
@@ -55,6 +55,8 @@ class Linker(CompilerWrapper):
         try:
             return self._lib_flags[lib]
         except KeyError:
+            if isinstance(self.compiler, Linker):
+                return self._compiler.get_lib_flags(lib)
             raise RuntimeError(f"Unknown library name: '{lib}'")
 
     def add_lib_flags(self, lib: str, flags: List[str],
@@ -112,6 +114,8 @@ class Linker(CompilerWrapper):
         :returns: the stdout of the link command
         '''
         # Don't need to add compiler's flags, they are added by CompilerWrapper.
+        if isinstance(self.compiler, Linker):
+            print("RECURSIVE")
         params: List[Union[str, Path]] = []
         if openmp:
             params.append(self._compiler.openmp_flag)

So the first modification (to get_lib_flags) should work just fine - if we don't have a defintion (which could overwrite the base linker), and the 'compiler' is a linker, we call it to get the definition.

The second modification needs a bit more work. For now I'd guess that any pre- and post-flags from the linker wrapper should wrap the linker ... that sounds convoluted. Assume linker1 adds pre1 and post1, and linker 2 adds pre2 and post2. If linker2 - Linker(linker1), then we should get:

pre2  pre1  <libraryflags> post1 post2

Is that enough to get you started?

@hiker
Copy link
Owner

hiker commented Oct 23, 2024

Luke will not have time to finish this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants