Skip to content

Add option to disable the server or client library - #673

Merged
bk138 merged 2 commits into
LibVNC:masterfrom
learn-more:optional_libraries
Sep 19, 2025
Merged

bk138 merged 2 commits into
LibVNC:masterfrom
learn-more:optional_libraries

Conversation

@learn-more

Copy link
Copy Markdown
Contributor

Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
Comment thread CMakeLists.txt
@bk138

bk138 commented Sep 17, 2025 •

Copy link
Copy Markdown
Member

Looks good code wise, there seem to be some edge cases though:

  • cmake .. -DWITH_LIBVNCSERVER=OFF
    • wstest.c:(.text+0x105): undefined reference to `rfbLog'
    • cargstest.c:(.text+0x84): undefined reference to `rfbGetScreen'
    • ...and some more, but only the tests
  • cmake .. -DWITH_LIBVNCCLIENT=OFF
    • /usr/bin/ld: CMakeFiles/test_encodingstest.dir/test/encodingstest.c.o: warning: relocation against rfbClientLog' in read-only section .text' (they might link against both client and server...)
    • only this test affected AFAICT

@bk138 bk138 self-assigned this Sep 17, 2025
@learn-more

Copy link
Copy Markdown
Contributor Author

Looks good code wise, there seem to be some edge cases though:

* cmake .. -DWITH_LIBVNCSERVER=OFF
  
  * wstest.c:(.text+0x105): undefined reference to `rfbLog'
  * cargstest.c:(.text+0x84): undefined reference to `rfbGetScreen'
  * ...and some more, but only the tests

* cmake .. -DWITH_LIBVNCCLIENT=OFF
  
  * /usr/bin/ld: CMakeFiles/test_encodingstest.dir/test/encodingstest.c.o: warning: relocation against `rfbClientLog' in read-only section `.text' (they might link against both client and server...)
  * only this test affected AFAICT

Thanks, I will fix those.

Do you also want me to have a go at integrating this in the CI in this PR?

@bk138

bk138 commented Sep 17, 2025

Copy link
Copy Markdown
Member

Do you also want me to have a go at integrating this in the CI in this PR?

yes, that's probably a good idea :-)

@learn-more
learn-more force-pushed the optional_libraries branch 2 times, most recently from 079caf7 to 76f3ba6 Compare September 17, 2025 18:20
@learn-more

Copy link
Copy Markdown
Contributor Author

Do you also want me to have a go at integrating this in the CI in this PR?

yes, that's probably a good idea :-)

I hope you don't mind, I applied the include feature of the matrix a bit creatively to have readable job names.

Comment thread CMakeLists.txt
Comment thread .github/workflows/ci.yml Outdated
Comment thread CMakeLists.txt
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml

@bk138 bk138 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

almost there! the last install statement with the .pc files needs adapting, too!

@bk138 bk138 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aaand: the set(INSTALL_HEADER_FILES) too.

basically: rfb.h only for server, rfbclient.h only for client.

@learn-more

learn-more commented Sep 18, 2025 •

Copy link
Copy Markdown
Contributor Author

almost there! the last install statement with the .pc files needs adapting, too!

Aaand: the set(INSTALL_HEADER_FILES) too.

basically: rfb.h only for server, rfbclient.h only for client.

Both of these should be addressed now.

Thanks for your patience!


The includetest does not seem to like the optional header deployment.
I'll add 2 targets, one for the client and one for the server if that's okay with you?

@bk138
bk138 merged commit 50023af into LibVNC:master Sep 19, 2025
28 checks passed
@learn-more
learn-more deleted the optional_libraries branch September 19, 2025 11:30
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

Successfully merging this pull request may close these issues.

2 participants