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

Don't modify CMAKE_RUNTIME_OUTPUT_DIRECTORY #1312

Conversation

franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Sep 12, 2022

Define and use openPMD_RUNTIME_OUTPUT_DIRECTORY instead. Instead, use the target property RUNTIME_OUTPUT_DIRECTORY rather than the global variable. Might lead to duplicates if both are defined:

> cmake . -LA 2>/dev/null | grep -i runtime
CMAKE_RUNTIME_OUTPUT_DIRECTORY:PATH=/home/franzpoeschel/singularity_build/openPMD_build/bin
openPMD_RUNTIME_OUTPUT_DIRECTORY:PATH=/home/franzpoeschel/singularity_build/openPMD_build/bin

For motivation, I'll copy Nils Schild's Slack message:

I wanted to use openPMD as a subproject in my CMake project using add_subdirectory(./third-party/openPMD-api) .
Since openPMD modifies some global CMake variables e.g. the CMAKE_RUNTIME_OUTPUT_DIRECTORY I ran into some bugs since the location of all my targets has been changed as well.
I could fix this by UNSET(CMAKE_RUNTIME_OUTPUT_DIRECTORY CACHE) after adding openPMD.
Is there a reason to change the global CMake variables in the openPMD Project? For CMAKE_RUNTIME_OUTPUT_DIRECTORY I could not find any usage in the github repo. But maybe I did not searched well enough.

TODO

  • Check if the replaced instances make sense for use in an internally-shipped source tree

Define and use openPMD_RUNTIME_OUTPUT_DIRECTORY instead
@DerNils-git
Copy link
Contributor

DerNils-git commented Sep 12, 2022

Maybe the less invasive option to solve this would be not to define global CMake variables as CACHE variables in the project.
Then the scope of CMake variables like would be limited to the directory of the subproject. CMake Variables
So instead of using e.g.

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib"
      CACHE PATH "Build directory for archives")

use

set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/lib")

I did this for the following variables in the openPMD project and could build the project afterwards without problem as a subproject of my own CMake project:

CMAKE_ARCHIVE_OUTPUT_DIRECTORY
CMAKE_LIBRARY_OUTPUT_DIRECTORY
CMAKE_RUNTIME_OUTPUT_DIRECTORY
CMAKE_INSTALL_CMAKEDIR
CMAKE_BUILD_TYPE

@franzpoeschel
Copy link
Contributor Author

Ah, I didn't know that this would restrict the scope. We might also take the chance to use target properties instead of a global, but leaving the global here might actually make things more maintainable in the long run.
I'll leave this open for now and wait for @ax3l's opinion.

@DerNils-git
Copy link
Contributor

If you remove them from CACHE they would still be global within the openPMD project but it would not change the same variables in parent projects.
Therefore this might be a more stable option for users but still easily maintainable.

@ax3l ax3l self-assigned this Sep 19, 2022
@ax3l ax3l self-requested a review September 19, 2022 14:52
@ax3l
Copy link
Member

ax3l commented Sep 19, 2022

@DerNils-git @franzpoeschel thank you so much for raising this.

After quickly checking in Kitware-driven projects like ADIOS2 and ParaView, I think @DerNils-git suggestion is what we should go with (#1313). Otherwise we have to introduce equivalent project-level control variables also for library paths and other artifacts we produce.

If the current approach still sets the variable for a downstream project in superbuilds, then this is all great 👍

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.

3 participants