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.

Sign up for GitHub

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

Jump to bottom

Improve CMake code #309

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Improve CMake code #309

wants to merge 14 commits into from

Conversation

DerDakon
Copy link
Contributor

This should make things more bullet proof and portable.

@DerDakon
Copy link
Contributor Author

Ping?

@bk138
Copy link
Member

bk138 commented Jun 17, 2019 via email

@DerDakon
Copy link
Contributor Author

Looks like you are back, so ping again ;)

bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated
@@ -640,7 +640,7 @@ function(get_link_libraries OUT TARGET)
set(RESULT "")
get_target_property(LIBRARIES ${TARGET} INTERFACE_LINK_LIBRARIES)
foreach(LIB ${LIBRARIES})
if("${LIB}" MATCHES ".*NOTFOUND.*")
if(NOT LIB)
Copy link
Member

Choose a reason for hiding this comment

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

I reckon NOTFOUND is different to unset. Not sure though. Do you have a link to the docs at hand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are different, but both false. See https://cmake.org/cmake/help/v3.15/command/if.html, section "if()".

bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated
@@ -223,11 +223,9 @@ endif()
if(LIBVNCSERVER_HAVE_SYS_UIO_H)
if(WITH_GCRYPT AND LIBGCRYPT_LIBRARIES)
message(STATUS "Building crypto with Libgcrypt")
set(CRYPTO_LIBRARIES ${LIBGCRYPT_LIBRARIES})
Copy link
Member

Choose a reason for hiding this comment

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

Bettter be verbose than to rely on side effects. wontmerge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is added a few lines below to ADDITIONAL_LIBRARIES.

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, nothing wrong here and the logic is much better especially see my comment on line 220

bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt Show resolved Hide resolved
bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt
@@ -621,30 +621,26 @@ endif()
#
# this gets the libraries needed by TARGET in "-libx -liby ..." form
#
function(get_link_libraries OUT TARGET)
Copy link
Member

Choose a reason for hiding this comment

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

need to check this one

Copy link
Contributor

Choose a reason for hiding this comment

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

need to check this one

This is fine for me, ADDITIONAL_LIBS correctly contains all the required libs, and i makes the code easier to read.

bk138
bk138 reviewed Jun 30, 2019
View reviewed changes
CMakeLists.txt
@@ -110,6 +110,7 @@ endif()

Copy link
Member

Choose a reason for hiding this comment

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

Here I need some more info on the purpose of this commit :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, the GitHub webinterface wont tell me what you added this comment to, so I can't tell you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bk138 this is fine, is just coding style. The endif(expression_name) has been deprecated a long time ago.

bk138
bk138 requested changes Jun 30, 2019
View reviewed changes
Copy link
Member

@bk138 bk138 left a comment •
edited

Choose a reason for hiding this comment

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

Cherry-picked 2, some are wontmerge, some needinfo, 1 needs a minor addition :-)
Thanks for the contribution!

@DerDakon DerDakon force-pushed the cmake-cleanups branch 3 times, most recently from 87a400f to a792655 Compare July 2, 2019 18:47
@DerDakon
Copy link
Contributor Author

What's missing now?

@bk138
Copy link
Member

bk138 commented Aug 27, 2019

Mainly time on my side, on parental leave for the time being. I've not forgotten this though :-)

@ledocc ledocc mentioned this pull request Sep 17, 2019
Open
@DerDakon DerDakon requested a review from bk138 June 19, 2020 16:20
@bk138
Copy link
Member

bk138 commented Apr 28, 2022

Big change to the build system to make it more modular, which is good, downside is that the big change increases the probability of regressions. In-depth testing needed, i.e. quite some time investment.

antenore
antenore suggested changes Apr 29, 2022
View reviewed changes
Copy link
Contributor

@antenore antenore left a comment

Choose a reason for hiding this comment

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

@DerDakon Please rebase to master as there are some conflicts and some CMake additions to be integrated in your branch

@bk138 Beside the requested change, to me this is good to go

CMakeLists.txt
@@ -110,6 +110,7 @@ endif()

Copy link
Contributor

Choose a reason for hiding this comment

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

@bk138 this is fine, is just coding style. The endif(expression_name) has been deprecated a long time ago.

CMakeLists.txt Outdated
@@ -223,11 +223,9 @@ endif()
if(LIBVNCSERVER_HAVE_SYS_UIO_H)
if(WITH_GCRYPT AND LIBGCRYPT_LIBRARIES)
message(STATUS "Building crypto with Libgcrypt")
set(CRYPTO_LIBRARIES ${LIBGCRYPT_LIBRARIES})
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, nothing wrong here and the logic is much better especially see my comment on line 220

CMakeLists.txt
if(PNG_FOUND)
set(LIBVNCSERVER_HAVE_LIBPNG 1)
else()
unset(PNG_LIBRARIES) # would otherwise contain -NOTFOUND, confusing target_link_libraries()
endif(PNG_FOUND)
if(NOT OPENSSL_FOUND)
unset(OPENSSL_LIBRARIES) # would otherwise contain -NOTFOUND, confusing target_link_libraries()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is indeed unneeded, when OPENSSL_FOUND is NOT FOUND, OPENSSL_LIBRARIES is already unset.

So the change absolutely make sense to me.

CMakeLists.txt
@@ -621,30 +621,26 @@ endif()
#
# this gets the libraries needed by TARGET in "-libx -liby ..." form
#
function(get_link_libraries OUT TARGET)
Copy link
Contributor

Choose a reason for hiding this comment

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

need to check this one

This is fine for me, ADDITIONAL_LIBS correctly contains all the required libs, and i makes the code easier to read.

CMakeLists.txt Outdated
${CMAKE_CURRENT_BINARY_DIR}/libvncclient.pc
DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig
)
if(NOT WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

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

Good spot this one, on Windows would (silently?) fail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antenore antenore antenore requested changes

@bk138 bk138 Awaiting requested review from bk138

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DerDakon @bk138 @antenore