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

boost: Adds with_stacktrace_noop option #7503

Closed
wants to merge 1 commit into from
Closed

boost: Adds with_stacktrace_noop option #7503

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Oct 2, 2021

Specify library name and version: boost/all

This is to add a with_stacktrace_noop option to boost::stacktrace. Currently, the BOOST_STACKTRACE_USE_NOOP define is set always and there isn't a way to disable it without modifying downstream generated build configs.


  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2021

CLA assistant check
All committers have signed the CLA.

@conan-center-bot
Copy link
Collaborator

Sorry, the build is only launched for Access Request users. You can request access writing in this issue.

@@ -1567,7 +1569,8 @@ def filter_transform_module_libraries(names):
self.cpp_info.components["stacktrace_backtrace"].defines.append("BOOST_STACKTRACE_USE_BACKTRACE")
self.cpp_info.components["stacktrace_backtrace"].requires.append("libbacktrace::libbacktrace")

self.cpp_info.components["stacktrace_noop"].defines.append("BOOST_STACKTRACE_USE_NOOP")
Copy link
Contributor

Choose a reason for hiding this comment

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

stacktrace_noop is a header-only library.
So the component is always available (when stacktrace is enabled)

Sure, you can add an option to specifically disable it, but is it worth it?

If you push further, please also do the following:

  • in configure, do del self.options.with_stcktrace_noop if stacktrace is disabled.
  • Also do del self.info.options.with_stacktrace_noop inpackage_id` (this can be done unconditionally)

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean stacktrace_noop is a header-only library? From what I understand, the noop is an implementation detail that is activated with the BOOST_STACKTRACE_USE_NOOP define. I'm new to the conan package, so is there another way to disable the noop define? Thanks!

Copy link
Contributor

@madebr madebr Oct 2, 2021

Choose a reason for hiding this comment

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

Look in the main recipe, there is only one noop match:

self.cpp_info.components["stacktrace_noop"].defines.append("BOOST_STACKTRACE_USE_NOOP")

This component is set when stacktrace is enabled:

if not self.options.without_stacktrace:

It does not add any libs.
self.cpp_info.components["stacktrace_noop"].libs = [...] is not present.

When writing the stacktrace tests, I found out that noop is the fallback backend of stacktrace.
If no implementation can be found, then nothing (=no-op) will be done.

The only way to disable no-op, is to make sure that another backend is available and used.

Copy link
Author

Choose a reason for hiding this comment

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

I see, then shouldn't the backtrace backend (which is True by default) disable no-op?

"with_stacktrace_backtrace": True,

Copy link
Contributor

Choose a reason for hiding this comment

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

No? no-op is always available, even when the addr2line/libbacktrace backend is built.

All three boost_stacktrace backends (noop/addr2line/libbacktrace) can be available in the same pacakge, just not used in the same target when using the cmake targets.

You can switch between the backends by carefully setting the definitions and linking to the correct libraries.

Copy link
Author

Choose a reason for hiding this comment

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

Will do, thanks!

Also, you mentioned that cmake would be deprecated as a generator. Is that for conan as a whole or just boost?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean only for Conan.
conanbuiodinfo.cmake will be a thing of the past.
I'm taking about Conan 2.0, coming in the (near) future.

Copy link
Author

Choose a reason for hiding this comment

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

I see, thanks!

As an aside, what build system would you recommend outside of cmake these days?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I use cmake for all of my private and professional c/c++ projects.
All other build systems have disadvantages that are showstoppers for me.

Though, a simple Makefile might be useful for something that has nothing to do with compiling sources.

[Meson}(https://mesonbuild.com/) is used by some OSS projects.
But I find its documentation hard to read + it is opiniated.
It has a potential though.
A meson syntax with a cmake backend would be very nice..

The grandfather of build systems for OSS is of course autotools (autoconf + automake + libtool).
But that is geared mostly towards Linux + gcc. It can work with MSVC on Windows, but it's harder.

If you really don't care about portability, you can also of course just use Visual Studio projects (.snl and .vcxproj files).

So in general, I can't really recommend anything but cmake.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, appreciate your insights!

Comment on lines +85 to +91
add_executable(stacktrace_noop_exe stacktrace.cpp)
if(WITH_STACKTRACE_NOOP)
target_compile_definitions(stacktrace_noop_exe PRIVATE TEST_STACKTRACE_IMPL=4)
else()
# BOOST_STACKTRACE_USE_NOOP is not defined in this case, so we set it here only for this target
target_compile_definitions(stacktrace_noop_exe PRIVATE TEST_STACKTRACE_IMPL=4 PRIVATE BOOST_STACKTRACE_USE_NOOP)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix your formatting (space, 4chars).
Also, you always test noop. This is because noop is header-only.
So this shows why I think this noop option is non-sensical.

The only thing it does is enable/disable exposure of Boost::stacktrace_noop.

else()
# BOOST_STACKTRACE_USE_NOOP is not defined in this case, so we set it here only for this target
target_compile_definitions(stacktrace_noop_exe PRIVATE TEST_STACKTRACE_IMPL=4 PRIVATE BOOST_STACKTRACE_USE_NOOP)
endif()
target_link_libraries(stacktrace_noop_exe PRIVATE Boost::stacktrace_noop)
Copy link
Contributor

Choose a reason for hiding this comment

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

This target will not be available when stacktrace_noop is disabled.

@ghost ghost closed this Oct 3, 2021
This pull request was closed.
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.

4 participants