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

CMAKE_RUNTIME_OUTPUT_DIRECTORY Windows compatibility #1159

Open
yihuajack opened this issue Jul 30, 2024 · 5 comments
Open

CMAKE_RUNTIME_OUTPUT_DIRECTORY Windows compatibility #1159

yihuajack opened this issue Jul 30, 2024 · 5 comments

Comments

@yihuajack
Copy link

In CMakeLists.txt,

set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib)
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)

which will put soci_core and soci_ DLLs under ${CMAKE_BINARY_DIR}/bin. However, on Windows, the executable is just under ${CMAKE_BINARY_DIR} rather than ${CMAKE_BINARY_DIR}/bin. If putting DLLs under ${CMAKE_BINARY_DIR} then the executable would fail to find the soci DLLs.

@vadz
Copy link
Member

vadz commented Aug 20, 2024

Sorry, which executable do you mean? I.e. what exactly is the problem and when does it happen?

@yihuajack
Copy link
Author

yihuajack commented Aug 21, 2024

I mean the executable of the program that uses the soci library, which is expected to be located under ${CMAKE_BINARY_DIR} on Windows. The problem is that if adding soci as a thirdparty library for a program on Windows, the program will not find any dlls under ${CMAKE_BINARY_DIR}/bin but ${CMAKE_BINARY_DIR}, so that there will be a dll not found error. One way to solve this is to manually copy soci's dlls from ${CMAKE_BINARY_DIR}/bin to ${CMAKE_BINARY_DIR}, but under some circumstances it is hard to do this copy for compatibility issues. I'm not sure about what's the role of the three settings in soci's CMakeLists.txt (if removing these settings or changes the settings to ${CMAKE_BINARY_DIR} then no problem exists).

@vadz
Copy link
Member

vadz commented Aug 21, 2024

Unfortunately I'm not sure about them neither. There is the #1118 (WIP) which aims to streamline all this, but I don't know if it addresses this.

Does your top-level project define CMAKE_RUNTIME_OUTPUT_DIRECTORY? If it does, we could skip setting it if it's already set which should be totally safe, I think.

@Krzmbrzl
Copy link
Contributor

Krzmbrzl commented Aug 31, 2024

I have built applications on top of #1118 and I don't see any issues even on Windows.

The issue here is likely in the order in which the top-level project includes SOCI. If you first include SOCI, then the runtime output property propagates to your top-level project such that all your target inherit the same property (unless you set it explicitly to something else).
However, any targets declared before including SOCI will not have this property and also won't get it once SOCI is included as the setting merely acts as a default for the given target's property.

Since the output directory property merely serves a cosmetic issue, I would vote for just not explicitly setting these things in SOCI. Then cmake's default will do something that just works on all platforms.
And if any given user has special desires as to where the build artifacts should end up, they can easily set these variables when calling cmake from the command line. That will then also ensure that the result is consistent, avoiding issues such as the one presented here.


@vadz as a side-note: feel free to ping me in cmake-related issues. Especially due to my work with the cmake refactoring I a) know a lot about SOCI's existing cmake implementation and b) have an interest in getting to know all issues that people face with the current setup to make sure the new one fixes it :)

Krzmbrzl added a commit to Krzmbrzl/soci that referenced this issue Aug 31, 2024
This can lead to inconsistencies when another project embeds SOCI which
especially on Windows are likely to lead to issues when SOCI is built as
a DLL.

Fixes SOCI#1159
Krzmbrzl added a commit to Krzmbrzl/soci that referenced this issue Sep 1, 2024
It turned out that on Windows this is actually necessary in order for
DLLs to be usable.

In contrast to what has been removed in
a2faea6, this commit adds a guard that
will ensure that when CMAKE_RUNTIME_DIRECTORY (which is the crucial one
in order for DLLs to work) is already set, we don't overwrite it. This
means that we should stick to whatever has already been configured,
avoiding any inconsistencies.
Additionally, we now emit a warning if SOCI is embedded by a project
that doesn't set these variables as that can also lead to
inconsistencies as then the order of when exactly SOCI is embedded into
the top-level project matters.

Relates to SOCI#1159
@yihuajack
Copy link
Author

Thanks for your instruction. Your PR #1118 can fix this issue.

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