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

Bump Boost shipped for Windows to v1.87 #10278

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

Bump Boost shipped for Windows to v1.87 #10278

wants to merge 5 commits into from

Conversation

Al2Klimov
Copy link
Member

No description provided.

@Al2Klimov Al2Klimov added area/windows Windows agent and plugins area/ci CI/CD labels Jan 2, 2025
@Al2Klimov Al2Klimov added this to the 2.15.0 milestone Jan 2, 2025
@cla-bot cla-bot bot added the cla/signed label Jan 2, 2025
@Al2Klimov
Copy link
Member Author

9fw3vb

@Al2Klimov Al2Klimov self-assigned this Jan 8, 2025
@Al2Klimov Al2Klimov marked this pull request as draft January 8, 2025 09:15
@Al2Klimov Al2Klimov added the consider backporting Should be considered for inclusion in a bugfix release label Jan 8, 2025
@Al2Klimov
Copy link
Member Author

@julianbrost @yhabteab @oxzi In your own interest, please pin Boost to < v1.87 for now.

boost::asio::spawn() doesn't take boost::coroutines::attributes anymore.

@Al2Klimov
Copy link
Member Author

... which is pretty annoying, seems that it works only with fibers now: boostorg/asio@df973a8#diff-4f46281483e5218aa919801a401dcd5b4c19704daa16f036e52da35a2860109dL1320

Definitely nothing for v2.14.4!

@julianbrost
Copy link
Contributor

@julianbrost @yhabteab @oxzi In your own interest, please pin Boost to < v1.87 for now.

That's only a very short-term solution. This will turn into a bigger problem as soon as any distro starts shipping that version. So this needs a proper solution in any case.

@Al2Klimov
Copy link
Member Author

That's only a very short-term solution.

Exactly. A bridge technology.

I mean, aren't you happy that I'm urging NOT urging yet another PR of mine into the current milestone?

This will turn into a bigger problem as soon as any distro starts shipping that version.

Yet, both Ubuntu 25 and Fedora 42 have Boost 1.83. OpenBSD 7.7 has v1.84 yet, on NixOS pinning is easiest of all I guess and MacOS isn't even officially supported.

So this needs a proper solution in any case.

Sure. A long-term solution.

@julianbrost
Copy link
Contributor

I don't say this must go into 2.14.4, I'm just saying that we shouldn't wait until "oh fuck, Fedora 44 is here and we can't build packages".

@Al2Klimov Al2Klimov force-pushed the boost187 branch 2 times, most recently from 0fb35e6 to 31e2cce Compare January 10, 2025 11:39
@Al2Klimov Al2Klimov marked this pull request as ready for review January 10, 2025 11:39
@Al2Klimov Al2Klimov removed their assignment Jan 10, 2025
@Al2Klimov Al2Klimov marked this pull request as draft January 10, 2025 14:11
@Al2Klimov
Copy link
Member Author

@Al2Klimov
Copy link
Member Author

we shouldn't wait until "oh fuck, Fedora 44 is here and we can't build packages".

A theoretical option is still doing the AL2-move, i.e building Icinga-boost 1.86 packages for that one or two $OS. (It's funny because normally that's been done due to Boost being too OLD, not too NEW.😅) Sound radical, but:

  1. can still be undone for any future Icinga version
  2. other OS have even older Boost versions
  3. Fedora has the shortest per-version support lifetime
  4. even the current PR state I'd rather not backport, if not necessary (or you insist), until a .0 and a .1 version of Icinga 2 with this PR

Note on the test failure

Unmodified

(WHERE master = 1065d3b)

.../icinga2/test/base-io-engine.cpp:38: error: in "base_io_engine/timeout_run": check called == 1 has failed [0 != 1]

This just says us the Timeout callback, [&called] { ++called; }, hasn't been called.

Has the coroutine even been started?

--- a/test/base-io-engine.cpp
+++ b/test/base-io-engine.cpp
@@ -20,2 +20,3 @@ BOOST_AUTO_TEST_CASE(timeout_run)
        boost::asio::spawn(strand, [&](boost::asio::yield_context yc) {
+               BOOST_CHECK_EQUAL(called, 1337);
                boost::asio::deadline_timer timer (io);

Now the test fails exactly like above, not with 1337, so the answer is no.

We need to go deeper...

--- a/test/base-io-engine.cpp
+++ b/test/base-io-engine.cpp
@@ -2,40 +2,46 @@

 #include "base/io-engine.hpp"
 #include "base/utility.hpp"
 #include <boost/asio.hpp>
 #include <boost/date_time/posix_time/posix_time.hpp>
 #include <BoostTestTargetConfig.h>
+#include <memory>
 #include <thread>

 using namespace icinga;

 BOOST_AUTO_TEST_SUITE(base_io_engine)

 BOOST_AUTO_TEST_CASE(timeout_run)
 {
        boost::asio::io_context io;
        boost::asio::io_context::strand strand (io);
        int called = 0;
+       auto workGuard (std::make_unique<decltype(boost::asio::make_work_guard(io))>(boost::asio::make_work_guard(io)));

        boost::asio::spawn(strand, [&](boost::asio::yield_context yc) {
+               BOOST_CHECK_EQUAL(called, 1337);
                boost::asio::deadline_timer timer (io);

                Timeout timeout (strand, boost::posix_time::millisec(300), [&called] { ++called; });
                BOOST_CHECK_EQUAL(called, 0);

                timer.expires_from_now(boost::posix_time::millisec(200));
                timer.async_wait(yc);
                BOOST_CHECK_EQUAL(called, 0);

                timer.expires_from_now(boost::posix_time::millisec(200));
                timer.async_wait(yc);
+               workGuard = nullptr;
        });

        std::thread eventLoop ([&io] { io.run(); });
+       BOOST_CHECK_EQUAL(called, 23);
        io.run();
+       BOOST_CHECK_EQUAL(called, 42);
        eventLoop.join();

        BOOST_CHECK_EQUAL(called, 1);
 }

 BOOST_AUTO_TEST_CASE(timeout_cancelled)

This one was my previous printf-debugger and it hangs at 0 != 23. I.e run() awaits workGuard, but the coroutine never stops that.

Al2Klimov and others added 4 commits January 10, 2025 16:35
not just boost::coroutines::detail::forced_unwind.

This is needed because as of Boost 1.87, boost::asio::spawn() uses Fiber, not Coroutine v1.
boostorg/asio@df973a85ed69f021

This is safe because every actual exception shall inherit from std::exception. Except forced_unwind and its Fiber equivalent, so that `catch(const std::exception&)` doesn't catch them and only them.
@Al2Klimov Al2Klimov marked this pull request as ready for review January 10, 2025 15:41
boost::coroutines::attributes(GetCoroutineStackSize()) // Set a pre-defined stack size.
#endif // BOOST_VERSION >= 108700
Copy link
Member Author

Choose a reason for hiding this comment

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

For the record, this affects only Windows, currently. (Remember! WE control what we ship there.)

boost::coroutines::attributes(GetCoroutineStackSize()) // Set a pre-defined stack size.
#endif // BOOST_VERSION >= 108000
Copy link
Member Author

Choose a reason for hiding this comment

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

This also affects:

  • Fedora 39+ (all)
  • Ubuntu 24.04+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci CI/CD area/windows Windows agent and plugins cla/signed consider backporting Should be considered for inclusion in a bugfix release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants