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: add optional dynamic linking to boost::log #1663

Closed
wants to merge 1 commit into from

Conversation

aduskett
Copy link
Contributor

@aduskett aduskett commented Sep 9, 2024

When building boost as a dynamic library, hundreds of undefined reference to boost::log` errors occur during linking. As the default behavior currently is to link against boost::log statically, it is best to add a new option that explicitly enables dynamic linking to boost::log. This new option is called MENDER_USE_DYNAMIC_BOOST_LOG.

Changelog: None
Ticket: None

@mender-test-bot
Copy link

@aduskett, Let me know if you want to start the integration pipeline by mentioning me and the command "start pipeline".


my commands and options

You can trigger a pipeline on multiple prs with:

  • mentioning me and start pipeline --pr mender/127 --pr mender-connect/255

You can start a fast pipeline, disabling full integration tests with:

  • mentioning me and start pipeline --fast

You can trigger GitHub->GitLab branch sync with:

  • mentioning me and sync

You can cherry pick to a given branch or branches with:

  • mentioning me and:
 cherry-pick to:
 * 1.0.x
 * 2.0.x

When building boost as a dynamic library, during linking, hundreds of
`undefined reference to `boost::log` errors occure. As the default behavior
currently is to statically link against boost::log, it is best to add a new
option that explicitly enables dynamically linking to boost::log. This new
option is called MENDER_USE_DYNAMIC_BOOST_LOG.

Signed-off-by: Adam Duskett <aduskett@gmail.com>
@aduskett
Copy link
Contributor Author

aduskett commented Sep 9, 2024

@mender-test-bot start pipeline --pr mender/127 --pr mender-connect/255

@mender-test-bot
Copy link

Hello 😺 I created a pipeline for you here: Pipeline-1445846749

Build Configuration Matrix

Key Value
AUDITLOGS_REV master
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
CREATE_ARTIFACT_WORKER_REV master
DEPLOYMENTS_ENTERPRISE_REV master
DEPLOYMENTS_REV master
DEVICEAUTH_ENTERPRISE_REV master
DEVICEAUTH_REV master
DEVICECONFIG_REV master
DEVICECONNECT_REV master
DEVICEMONITOR_REV master
GENERATE_DELTA_WORKER_REV master
GUI_REV master
INTEGRATION_REV master
INVENTORY_ENTERPRISE_REV master
INVENTORY_REV master
IOT_MANAGER_REV master
MENDER_ARTIFACT_REV master
MENDER_BINARY_DELTA_REV master
MENDER_CLI_REV master
MENDER_CONFIGURE_MODULE_REV master
MENDER_CONNECT_REV pull/255/head
MENDER_CONVERT_REV master
MENDER_GATEWAY_REV master
MENDER_REV pull/1663/head
MENDER_SETUP_REV master
MENDER_SNAPSHOT_REV master
MONITOR_CLIENT_REV master
RUN_BACKEND_INTEGRATION_TESTS true
RUN_INTEGRATION_TESTS true
TENANTADM_REV master
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
USERADM_ENTERPRISE_REV master
USERADM_REV master
WORKFLOWS_ENTERPRISE_REV master
WORKFLOWS_REV master

@aduskett
Copy link
Contributor Author

I would like to see what failed, but the pipeline URL goes to a 404 :(

@lluiscampos
Copy link
Contributor

@mender-test-bot start pipeline

@mender-test-bot
Copy link

Hello 😺 I created a pipeline for you here: Pipeline-1447273009

Build Configuration Matrix

Key Value
AUDITLOGS_REV master
BUILD_BEAGLEBONEBLACK true
BUILD_CLIENT true
BUILD_QEMUX86_64_BIOS_GRUB true
BUILD_QEMUX86_64_BIOS_GRUB_GPT true
BUILD_QEMUX86_64_UEFI_GRUB true
BUILD_VEXPRESS_QEMU true
BUILD_VEXPRESS_QEMU_FLASH true
BUILD_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
CREATE_ARTIFACT_WORKER_REV master
DEPLOYMENTS_ENTERPRISE_REV master
DEPLOYMENTS_REV master
DEVICEAUTH_ENTERPRISE_REV master
DEVICEAUTH_REV master
DEVICECONFIG_REV master
DEVICECONNECT_REV master
DEVICEMONITOR_REV master
GENERATE_DELTA_WORKER_REV master
GUI_REV master
INTEGRATION_REV master
INVENTORY_ENTERPRISE_REV master
INVENTORY_REV master
IOT_MANAGER_REV master
MENDER_ARTIFACT_REV master
MENDER_BINARY_DELTA_REV master
MENDER_CLI_REV master
MENDER_CONFIGURE_MODULE_REV master
MENDER_CONNECT_REV master
MENDER_CONVERT_REV master
MENDER_GATEWAY_REV master
MENDER_REV pull/1663/head
MENDER_SETUP_REV master
MENDER_SNAPSHOT_REV master
MONITOR_CLIENT_REV master
RUN_BACKEND_INTEGRATION_TESTS true
RUN_INTEGRATION_TESTS true
TENANTADM_REV master
TEST_QEMUX86_64_BIOS_GRUB true
TEST_QEMUX86_64_BIOS_GRUB_GPT true
TEST_QEMUX86_64_UEFI_GRUB true
TEST_VEXPRESS_QEMU true
TEST_VEXPRESS_QEMU_FLASH true
TEST_VEXPRESS_QEMU_UBOOT_UEFI_GRUB true
USERADM_ENTERPRISE_REV master
USERADM_REV master
WORKFLOWS_ENTERPRISE_REV master
WORKFLOWS_REV master

@lluiscampos
Copy link
Contributor

I would like to see what failed,

You copied the example start, pointing the bot to old or non-existing PR for mender and mender-connect. I started it for you.

but the pipeline URL goes to a 404 :(

Yeah, this pipeline is internal to the Mender team, sorry.

@lluiscampos lluiscampos requested review from kacf and danielskinstad and removed request for kacf September 10, 2024 10:20
@aduskett
Copy link
Contributor Author

I would like to see what failed,

You copied the example start, pointing the bot to old or non-existing PR for mender and mender-connect. I started it for you.

but the pipeline URL goes to a 404 :(

Yeah, this pipeline is internal to the Mender team, sorry.

Thank you so much!

@danielskinstad
Copy link
Contributor

Hey 😃 Thank you for your contribution!

I have some comments/questions.

As far as I know, and can see from the pre-existing code, it defaults to dynamically linking boost, not statically (unless you set MENDER_DOWNLOAD_BOOST).

  • ldd of mender-auth after a clean build: libboost_log.so.1.74.0 => /lib/x86_64-linux-gnu/libboost_log.so.1.74.0

Secondly, I don't get any undefined reference errors when linking boost, and I don't see it any pipelines either. Might it be your setup? Does the same happen with a completely, unmodified, fresh build of mender (and following the build guide in the README)?

@kacf
Copy link
Member

kacf commented Sep 11, 2024

Indeed, Boost is already linked dynamically. The proposed fix was removed earlier for this reason. It must be something in the environment.

@aduskett
Copy link
Contributor Author

What this looks like is Buildroot not setting BOOST_LOG_DYN_LINK or BOOST_ALL_DYN_LINKS as found in boosts config.hpp:238

#   if defined(BOOST_LOG_DYN_LINK) || defined(BOOST_ALL_DYN_LINKS)
#       define BOOST_LOG_DLL
#   endif

Which causes the above issue. Both Fedora and Debian do indeed define BOOST_LOG_DYN_LINK which is why the autobuilders aren't having the problem. Now, as to how to set BOOST_LOG_DYN_LINK is the issue.

@kacf
Copy link
Member

kacf commented Sep 11, 2024

@aduskett: Can you elaborate on how Debian and Fedora define this? I cannot find this on my system, nor on a fresh one, in fact, it's quite clearly not turned on (in Debian):

/usr/include/boost/config/user.hpp:90:// #define BOOST_ALL_DYN_LINK

(BOOST_LOG_DYN_LINK doesn't have a define anywhere, as far as I can see)

There are also some other issues with the proposal:

  • The PR doesn't appear to have any effect on my system when turning it off. This is probably because the absence of definition is having the "default" effect, which appears to be to link dynamically anyway, on my system. But this means the toggle does not work correctly.
  • add_definitions has been superseded by other variants, see here, and should anyway be lowercase to match the rest of the code.

I'm wondering if it isn't better to just do this in autobuild with direct flags instead of having an option (which has some pitfalls, as we have seen). Something like this, for example:

cmake -D CMAKE_CXX_FLAGS=-DBOOST_LOG_DYN_LINK -D CMAKE_EXE_LINKER_FLAGS=-DBOOST_LOG_DYN_LINK .

Would that work for autobuild?

@aduskett
Copy link
Contributor Author

Hey! So I * did * figure out the problem with buildroot, which is from commit 4349746dac6f55b0b0faca21a7e609fb67d1471b

Author: Fabrice Fontaine <fontaine.fabrice@gmail.com>
Date:   Thu Apr 18 11:13:50 2019 +0200

    package/boost: don't install boost cmake files
    
    Since version 1.70.0, boost installs its own cmake files which are
    utterly broken so don't install them otherwise all cmake packages using
    boost (host-thrift, domoticz, i2pd ...) will fail because
    BOOST_INCLUDE_DIRS will be empty
    
    Fixes:
     - http://autobuild.buildroot.org/results/4ada26bab5c1e6504c7d8c92672326ced1d336df
    
    Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
    Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Five years ago! The fix is to remove --no-cmake-config from boost.mk in buildroot and let boost configure the .cmake file before installing them. Sorry about the junk merge request!

@aduskett aduskett closed this Sep 11, 2024
@kacf
Copy link
Member

kacf commented Sep 11, 2024

No worries, great that you figured it out!

arnout pushed a commit to buildroot/buildroot that referenced this pull request Sep 15, 2024
This reverts commit 4349746
which states:
    commit 4349746
    Author: Fabrice Fontaine <fontaine.fabrice@gmail.com>
    Date:   Thu Apr 18 11:13:50 2019 +0200

    package/boost: don't install boost cmake files

    Since version 1.70.0, boost installs its own cmake files which are
    utterly broken so don't install them otherwise all cmake packages using
    boost (host-thrift, domoticz, i2pd ...) will fail because
    BOOST_INCLUDE_DIRS will be empty

    Fixes:
     - http://autobuild.buildroot.org/results/4ada26bab5c1e6504c7d8c92672326ced1d336df

    Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
    Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

However, this has since been fixed upstream in the five years this change
has been in Buildroot. Furthermore, having --no-cmake-config breaks packages
expecting to dynamically link to boost::log with a multitude of `undefined
reference to boost::log` errors.
See: mendersoftware/mender#1663 for one such example.

As the --no-cmake-config is no longer needed because the original problem has
long since been fixed upstream, it is safe to revert the change.

Tested with the following packages as a smoke test:

  - host-thrift
  - domoticz
  - i2pd
  - libcpprestsdk
  - log4cxx
  - mpd
  - libcamera-apps

Signed-off-by: Adam Duskett <adam.duskett@amarulasolutions.com>
Signed-off-by: Arnout Vandecappelle <arnout@mind.be>
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.

5 participants