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

Fix build with boost ASIO 1.87. Support boost 1.66+ #9628

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vtnerd
Copy link
Contributor

@vtnerd vtnerd commented Dec 17, 2024

This should compile with Boost 1.73-1.87. I have only tested on Boost 1.74 and Boost 1.87 (primarily unit tests). I will mark the places where I know changes need to be made for older boost versions. I will clear up all such errors by manually downloading Boost 1.66 and testing.

One problem I have with supporting Boost 1.66-1.69 is that most types require an io_context object, whereas Boost 1.70+ have relaxed this requirement to an executor object. I will show an example of where it matters; it will be easy to break older Boost versions unintentionally.

EDIT:
This is currently marked as a draft, because the depends build will fail until #9162 gets merged (which is also marked as draft).

contrib/epee/include/net/abstract_tcp_server2.inl Outdated Show resolved Hide resolved
contrib/epee/src/net_ssl.cpp Outdated Show resolved Hide resolved
contrib/epee/src/net_helper.cpp Outdated Show resolved Hide resolved
@tobtoht
Copy link
Contributor

tobtoht commented Dec 17, 2024

(This needs #9162 to pass depends CI. I will undraft that PR after the next round of merges.)

@vtnerd vtnerd force-pushed the fix/boost_1.87_squashed branch from 8c98c45 to e88e61d Compare December 18, 2024 02:51
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 18, 2024

The latest force push should work with Boost 1.66-1.87. Something funky happened with the tests, so I may push a dummy change later to see what happens.

@tobtoht
Copy link
Contributor

tobtoht commented Dec 18, 2024

https://github.com/tobtoht/monero/actions/runs/12390979989/job/34587144842#step:5:1281

Looks like boost::asio::io_service is also used in src/device_trezor/trezor.

@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 20, 2024

Fore pushed some changes:

  • Reverted changes to query code, they are basically same style with new interface
  • Switched to io_context_t as @jeffro256 suggested
  • Fixed Trezor build.

@vtnerd vtnerd force-pushed the fix/boost_1.87_squashed branch 2 times, most recently from 1c62d08 to da62d98 Compare December 20, 2024 19:22
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 20, 2024

Force pushed twice with updates:

  • Added a missing include, but it doesn't matter as the depends builds is using Boost 1.64 which won't work
  • Reverted some whitespace changes that were made when making the patch.

@vtnerd vtnerd force-pushed the fix/boost_1.87_squashed branch from da62d98 to a0ce1e9 Compare December 20, 2024 19:38
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 20, 2024

Force pushed an include fix.

@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 20, 2024

This patch looks ready. The depends CI will all fail until #9162 gets merged. I will leave as draft for now for that reason.

Copy link
Collaborator

@selsta selsta left a comment

Choose a reason for hiding this comment

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

README needs an update to indicate the new minimum version

@0xFFFC0000
Copy link
Collaborator

Once this is ready for review, please remove Draft status.

@vtnerd vtnerd force-pushed the fix/boost_1.87_squashed branch 2 times, most recently from 625f370 to 0452910 Compare December 25, 2024 23:17
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 25, 2024

Force pushed:

Likely this will get removed as draft, assuming some of the tests cycle green.

@vtnerd vtnerd force-pushed the fix/boost_1.87_squashed branch from 0452910 to ad7eaaa Compare December 25, 2024 23:45
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 25, 2024

README needs an update to indicate the new minimum version

Forgot to update this in the last force push, so another quick push with this change.

@vtnerd vtnerd marked this pull request as ready for review December 26, 2024 14:19
@vtnerd
Copy link
Contributor Author

vtnerd commented Dec 26, 2024

Now ready for review.

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.

5 participants