Skip to content

Update compile options to enable static linking to work#380

Merged
davidchisnall merged 4 commits intognustep:masterfrom
ingresso-pete:master
Apr 18, 2026
Merged

Update compile options to enable static linking to work#380
davidchisnall merged 4 commits intognustep:masterfrom
ingresso-pete:master

Conversation

@ingresso-pete
Copy link
Copy Markdown

This makes the static options the same as the dynamic, which was causing the linking to fail. As this requires robin-map, and this is installed under /usr/local on BSD and similar systems, then we also add /usr/local/include to the header search path if it exists.

All the tests pass, and this works perfectly with my test code too.

Copy link
Copy Markdown
Member

@davidchisnall davidchisnall left a comment

Choose a reason for hiding this comment

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

Please can you also add static to at least some of the test matrix so that we don't break it in the future?

Comment thread CMakeLists.txt Outdated
Comment on lines +158 to +160
if (IS_DIRECTORY /usr/local/include)
include_directories("/usr/local/include")
endif()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure why this is needed, we should be fetching the robin map if it isn't installed, or finding it otherwise. I don't like accidentally depending on ambient things if we can avoid it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes, it puzzled me, because even though it includes the header, you dont need to link the robin library to use the result. will take a look at this.

will also look at the tests, [ I assumed that they would run automatically if I built it static ]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

[ oh, the code is all in the header.... ]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

For the tests, if you build the static library, it now compiles a static version of each of the non-legacy tests, and statically links the whole thing. Those get run alongside the shared library tests, and they all pass.

[ I am not great at CMake, so I have to admit I used Claude for the actual changes to make this happen. I understand what it has done, and it seems reasonable to me, but you may want to eyeball that last commit a bit more closely. ]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, CMake is a bit weird, ‘link library’ really means ‘import everything this target exposes’, which includes adding include paths it exports.

This makes the static options the same as the dynamic, which was
causing the linking to fail. As this requires robin-map, and this
is installed under /usr/local on BSD and similar systems, then we
also add /usr/local/include to the header search path if it exists .
This rmeoves the clunky insretion of /usr/local/include, which was my
prevuous fix. It nwo works the same way for static as it does for dynamic.
When we are bulding the static library, then a second version
of each of the non-legacy tests is generated, linked against
it. These are run together with the shared library versions.
…table

The previous commit created tests which linked the static library, but
were stil themselves dynamic executables. To properly test that we
can use the static library in a statically linked executable we want
the tests themselevs to be linekd this way. Hence this.
@davidchisnall davidchisnall merged commit ecef7f1 into gnustep:master Apr 18, 2026
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants