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

depends: build zeromq with CMake #29723

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Mar 25, 2024

Based on #30078.

This picks up a change, which is a switch to building zeromq with CMake. It includes a patch with a couple changes I've upstreamed:

and another Windows-related change:

I also noticed the CMake Windows version autodetction was broken with mingw-w64 (zeromq/libzmq#4669), so we set the version explicitly.

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 25, 2024

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
Concept ACK hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

@hebasto
Copy link
Member

hebasto commented Mar 25, 2024

Concept ACK.

@hebasto
Copy link
Member

hebasto commented Mar 25, 2024

This picks up a change, which is a switch to building zeromq with CMake.

From zeromq/libzmq#4667 (comment):

Please note that CMake-generated libzmq.pc file is also broken as its "Libs.private" section contains only -lstdc++ when cross-compiling for Windows.

@fanquake
Copy link
Member Author

From zeromq/libzmq#4667 (comment):

Can you elaborate / suggest something concrete? As far as I can see, the cross-compiling build of Windows for this branch currently works fine, we already link again -lws2_32 from our own build (so that shouldn't cause build failures), and we currently patch the -lstdc++ out of Libs.private, because as far as we are concerned, it's a bug.

@fanquake
Copy link
Member Author

Given both changes have landed, I've reordered the commits, and undrafted. Will followup with the Windows issues.

@fanquake
Copy link
Member Author

Will followup with the Windows issues.

IPC build issue should be fixed in zeromq/libzmq#4672

@fanquake
Copy link
Member Author

IPC build issue should be fixed in zeromq/libzmq#4672

This was resolved using a different change. Have pulled in that patch, rebased and updated the PR description.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

I've tested 796a271 on Ubuntu 23.10 using a patch from #29960.

There are a few differences between Autotools and CMake builds:

  1. In CMake, the resulted archive lacks object files from the following sources:
gssapi_client.cpp
gssapi_mechanism_base.cpp
gssapi_server.cpp
vmci_address.cpp
vmci_connecter.cpp
vmci_listener.cpp
vmci.cpp
  1. CMake build is effectively compiled with -O3 flag.

  2. CMake adds -DZMQ_CUSTOM_PLATFORM_HPP.

@DrahtBot
Copy link
Contributor

/ci_container_base/depends/work/build/x86_64-w64-mingw32/zeromq/4.3.5-fa67b8336a3/src/ipc_listener.cpp:22:2: error: #error On Windows, IPC does not work with POLLER=select, use POLLER=epoll instead, or disable IPC transport
   22 | #error On Windows, IPC does not work with POLLER=select, use POLLER=epoll instead, or disable IPC transport
      |  ^~~~~

@fanquake
Copy link
Member Author

Fixed the Windows build error, but drafted while based on #30078.

@fanquake fanquake marked this pull request as draft May 13, 2024 08:49
fanquake and others added 3 commits May 13, 2024 20:01
Some of these have been upstreamed. Some, i.e changing minimum versions
are unlikely to be accepted, so have not and exist here to suppress
output.
Currently one Windows related fixup:
* The CMake WIN32_WINNT autodetection is broken, and must be set
  manually. We may want to set is explicitly in any case, but the
  brokenness should also be fixed upstream

Co-authored-by: fanquake <fanquake@gmail.com>
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.

None yet

4 participants