CMake: Fix libdir in pkg-config file#214
CMake: Fix libdir in pkg-config file#214akien-mga wants to merge 1 commit intoKhronosGroup:masterfrom
Conversation
| list(APPEND PRIVATE_LIBS_LIST "-l${LIB}") | ||
| endforeach() | ||
| list(REMOVE_DUPLICATES PRIVATE_LIBS_LIST) | ||
| list(JOIN PRIVATE_LIBS_LIST " " PRIVATE_LIBS) |
There was a problem hiding this comment.
I'm not a CMake expert, so if there's a cleverer way to prevent adding duplicates to a string without having to use a list and a REMOVE_DUPLICATES filter, that could be used it.
Or maybe iterating over both CMAKE_CXX_IMPLICIT_LINK_LIBRARIES and PLATFORM_LIBS is overkill, I did not investigate further.
lenny-lunarg
left a comment
There was a problem hiding this comment.
This looks good to me. Stripping duplicates seems like a good idea, even if it wasn't already causing trouble.
|
Let me just add one more comment, can I request that you rebase this so we can hopefully get a CI pass on Linux? Windows CI is still failing, but Linux CI is working on the last code in master. I just want to double check that this can pass a build on the CI machine (because these failures are caused because master was failing when you opened the PR). |
The previous code assumed that `CMAKE_INSTALL_LIBDIR` is a relative
path, which it is not guaranteed to be, so we could end up with things
like this:
```
libdir=${exec_prefix}//usr/lib64
```
`CMAKE_INSTALL_FULL_LIBDIR` is guaranteed to be an absolute path.
We do lose the `${exec_prefix}` variable, the full path is now
hardcoded.
While at it, I made the include path configurable similarly, and fixed
`Libs.private` which would get duplicate entries:
```
Libs.private: -lstdc++ -lm -lgcc_s -lgcc -lc -lgcc_s -lgcc
```
f6b624e to
e4b7c54
Compare
|
I don't know why I didn't think to just rebase this myself when I requested the rebase for CI. Ijust did that now. Assuming it passes I'll merge. |
|
Sorry I had forgotten about this, I would have rebased otherwise :) |
|
No worries. I would have done it sooner, too if I had been thinking about this. |
|
I'm sorry I didn't reply to this at the time, but the reason I didn't merge this earlier was that the change was failing CI. The reason for this list that the |
The previous code assumed that
CMAKE_INSTALL_LIBDIRis a relativepath, which it is not guaranteed to be, so we could end up with things
like this:
CMAKE_INSTALL_FULL_LIBDIRis guaranteed to be an absolute path.We do lose the
${exec_prefix}variable, the full path is nowhardcoded.
While at it, I made the include path configurable similarly, and fixed
Libs.privatewhich would get duplicate entries: