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

GDAL should not override POSITION_INDEPENDENT_CODE #5649

Closed
mmomtchev opened this issue Apr 23, 2022 · 10 comments
Closed

GDAL should not override POSITION_INDEPENDENT_CODE #5649

mmomtchev opened this issue Apr 23, 2022 · 10 comments

Comments

@mmomtchev
Copy link
Contributor

Expected behavior and actual behavior.

Building GDAL as a static library and position independent code is currently very impractical

Every GDAL subdirectory includes a manual override:

set_property(TARGET something PROPERTY POSITION_INDEPENDENT_CODE ${BUILD_SHARED_LIBS})

POSITION_INDEPENDENT_CODE should probably be a global value that can be adjusted by the user. My suggestion is that it be an option with its default value being ${BUILD_SHARED_LIBS}

For example, when GDAL is being built as a Node.js addon it is built as a monolithic shared object - this means linking of the Node.js bindings application with a static GDAL build that must be position independent to be loaded into Node.js as a shared library.

GDAL version and provenance

GDAL 3.5.0-git

@mmomtchev
Copy link
Contributor Author

Is there a reason for having a separate override in each directory?

@dg0yt
Copy link
Contributor

dg0yt commented Apr 24, 2022

POSITION_INDEPENDENT_CODE should probably be a global value that can be adjusted by the user. My suggestion is that it be an option with its default value being ${BUILD_SHARED_LIBS}

There is already CMAKE_POSITION_INDEPENDENT_CODE. But it may make sense to read the documentation.
(And there are toolchains that pass compiler flags directly.)

@mmomtchev
Copy link
Contributor Author

Well, if there are toolchains that pass compiler flags directly, then nothing can be done for these toolchains.
CMAKE_POSITION_INDEPENDENT_CODE is not enough, there must be an option that can be set from the outside.

@dg0yt
Copy link
Contributor

dg0yt commented Apr 25, 2022

CMAKE_POSITION_INDEPENDENT_CODE is a CMake variable, so it can be set from the user.

But it may make sense to read the documentation.

Here: https://cmake.org/cmake/help/v3.23/variable/CMAKE_POSITION_INDEPENDENT_CODE.html

@mmomtchev
Copy link
Contributor Author

mmomtchev commented Apr 25, 2022

I have added an option that sets CMAKE_POSITION_INDEPENDENT_CODE because using an option has advantages when including the project via add_directory. I can't understand what you imply, maybe you can simply say it?

@rouault
Copy link
Member

rouault commented Apr 25, 2022

I believe this PR is fine. We need to explicitly override CMake default settings because we use OBJECT libraries, that would otherwise be built with settings for static linking even if we incorporate them in a shared libraries (things could perhaps be simpler if we use features that were added in CMake 3.12 or 3.13 regarding linking an OBJECT library, but for now that's probably the best solution)

@dg0yt
Copy link
Contributor

dg0yt commented Apr 26, 2022

I have added an option that sets CMAKE_POSITION_INDEPENDENT_CODE because using an option has advantages when including the project via add_directory.

I think that CMAKE_POSITION_INDEPENDENT_CODE is the user option, and adding a project-specific option to control a standard option is a source of confusion.
I don't see how this relates to add_directory. In fact, projects inventing own options for build-wide settings seem to add a complication to this use case.

CMake documentation also shows that the use of this variable for executables somewhat changed with CMake versions/policies, so it has not only ON/OFF but also "unset".
Any observed insufficiencies could be handled by reporting inconsistencies between BUILD_SHARED_LIBS and CMAKE_POSITION_INDEPENDENT_CODE, or by using a derived, normal variable that fills the gap when CMAKE_POSITION_INDEPENDENT_CODE is unset.

@rouault
Copy link
Member

rouault commented Apr 27, 2022

CMake documentation also shows that the use of this variable for executables somewhat changed with CMake versions/policies, so it has not only ON/OFF but also "unset".

I indeed see in the doc that there might be potential issues with some linkers if enabling CMAKE_POSITION_INDEPENDENT_CODE for executables. @mmomtchev would you be willing to rework your PR to keep the various set_target_properties(), but perhaps use a GDAL_OBJECT_LIBRARIES_POSITION_INDEPENDENT_CODE variable, initialized with BUILD_SHARED_LIBS value ?

@mmomtchev
Copy link
Contributor Author

@rouault done

@rouault
Copy link
Member

rouault commented May 2, 2022

fixed per #5650

@rouault rouault closed this as completed May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants