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: fix with_stacktrace_backtrace for Macos & MinGW + bump dependencies #5577

Merged
merged 9 commits into from
May 23, 2021

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented May 20, 2021

Specify library name and version: lib/1.0

closes #5556
closes #5571


  • 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.

dl is already added if Linux of FreeBSD to stacktrace_backtrace component
prince-chrismc
prince-chrismc previously approved these changes May 20, 2021
@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

It still fails with MinGW on Windows out of the box because default value of addr2line_location option is /usr/bin/addr2line

@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

Interesting, now we can see that with_stacktrace_backtrace doesn't work as is on Macos.

@madebr
Copy link
Contributor

madebr commented May 20, 2021

Looks like this error got triggered before at #5420 (comment) but I didn't look into it because it went away.

This is the culprit: https://github.com/boostorg/stacktrace/blob/b37e0d8e9f9613813534b6765559bdec693f34d5/include/boost/stacktrace/detail/libbacktrace_impls.hpp#L95-L101

The failed logs contain:

/Users/jenkins/w/BuildSingleReference@2/.conan/data/b2/4.5.0/_/_/package/46f53f156846659bf39ad6675fa0ee8156e859fe/lib
boost/1.72.0: WARN: Build folder is dirty, removing it: /Users/jenkins/w/BuildSingleReference@2/.conan/data/boost/1.72.0/_/_/build/f05b37ef109e11cd86bbb8d64d51015777db9870
WARN: replace_in_file didn't find pattern '/* thread_local */' in '/Users/jenkins/w/BuildSingleReference@2/.conan/data/boost/1.72.0/_/_/source/source_subfolder/boost/stacktrace/detail/libbacktrace_impls.hpp' file.
WARN: replace_in_file didn't find pattern '/* static __thread */' in '/Users/jenkins/w/BuildSingleReference@2/.conan/data/boost/1.72.0/_/_/source/source_subfolder/boost/stacktrace/detail/libbacktrace_impls.hpp' file.

It looks like my fix at the top of the build method does not apply/work on older boost releases...

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

Yes, tools.replace_in_file with strict=False is bad bad bad ;)

@madebr
Copy link
Contributor

madebr commented May 20, 2021

No wait, the patch is ok, but is currently only applied for clang.
It should be expanded to apple-clang. I don't know for what versions though.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

Oh I see, there are 2 successive patches, it's not obvious to understand what happen. The first 2 ones seem to be useless, these patterns don't exist.

@madebr
Copy link
Contributor

madebr commented May 20, 2021

Don't know whether it would make it easier to understand, but a regex could do it in one line 😄
conan-io/conan#7007

tools.replace_regex_in_file(r"(thread_local|static __thread)", r"/*\1*/")

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

I mean, I didn't understand why there was first a patch to replace /* thread_local */ by thread_local with strict=False, then another one to do the opposite based on a condition.
But I get it, boost recipe does not copy source code, so any conditional patch is a mess.

@madebr
Copy link
Contributor

madebr commented May 20, 2021

I mean, I didn't understand why there was first a patch to replace /* thread_local */ by thread_local with strict=False, then another one to do the opposite based on a condition.
But I get it, boost recipe does not copy source code, so any conditional patch is a mess.

That was one of the comments on my earlier review.
Perhaps add a comment/FIXME. jgsogo commented to maybe remove no_copy_source = True in a future pr.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

There should be a way to patch those lines in libbacktrace_impls.hpp with macros.

@madebr
Copy link
Contributor

madebr commented May 20, 2021

There is BOOST_NO_CXX11_THREAD_LOCAL (https://github.com/boostorg/stacktrace/blob/b37e0d8e9f9613813534b6765559bdec693f34d5/include/boost/stacktrace/detail/libbacktrace_impls.hpp#L95-L101).
Maybe my testing was incorrect and defining that macro suffices?

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 20, 2021

Maybe this in source()?

        tools.replace_in_file(os.path.join(self.source_folder, self._source_subfolder, "boost", "stacktrace", "detail", "libbacktrace_impls.hpp"),
                              "#   ifndef BOOST_NO_CXX11_THREAD_LOCAL",
                              "#   if defined(__APPLE__) || (defined(__clang__) && __clang_major__ < 6)\n#   elif !defined(BOOST_NO_CXX11_THREAD_LOCAL)")

@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 21, 2021

off-topic: any reason why installation is done in build()?

@madebr
Copy link
Contributor

madebr commented May 21, 2021

I didn't investigate but I think because configuration + build + install is tightly coupled in boost.build (b2).
You need to do b2 <args> to build and b2 <args> install to install.
b2 will re-run all tests in package, unless it stores state.

It would be nice to have the boost recipe behaving normal.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 21, 2021

Seems to be properly split in https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-boost/PKGBUILD, I'll try to split those steps.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 21, 2021

But first fix this issue in test_package on macOS:

/Users/jenkins/w/BuildSingleReference@2/.conan/data/boost/1.70.0/_/_/package/9b6ef57289b8c95bc978159b9aa7bfb003dfb749/include/boost/stacktrace/detail/collect_unwind.ipp:33:2: error: "Boost.Stacktrace requires `_Unwind_Backtrace` function. Define `_GNU_SOURCE` macro or `BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED` if _Unwind_Backtrace is available without `_GNU_SOURCE`."
#error "Boost.Stacktrace requires `_Unwind_Backtrace` function. Define `_GNU_SOURCE` macro or `BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED` if _Unwind_Backtrace is available without `_GNU_SOURCE`."
 ^

@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented May 21, 2021

Well, waiting for #5575. I'm surprised to see that CI uses --dry-build option, which is the reason why coherency of build requirements of dependencies is tested.

@SpaceIm SpaceIm closed this May 22, 2021
@SpaceIm SpaceIm reopened this May 22, 2021
@conan-center-bot

This comment has been minimized.

@conan-center-bot
Copy link
Collaborator

All green in build 8 (a310eb3362dddb89220eab8b1c48eed3176fa6eb):

  • boost/1.70.0@:
    All packages built successfully! (All logs)

  • boost/1.69.0@:
    All packages built successfully! (All logs)

  • boost/1.71.0@:
    All packages built successfully! (All logs)

  • boost/1.72.0@:
    All packages built successfully! (All logs)

  • boost/1.73.0@:
    All packages built successfully! (All logs)

  • boost/1.74.0@:
    All packages built successfully! (All logs)

  • boost/1.75.0@:
    All packages built successfully! (All logs)

  • boost/1.76.0@:
    All packages built successfully! (All logs)

Copy link
Contributor

@madebr madebr left a comment

Choose a reason for hiding this comment

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

The logs look fine.
Thanks for fixing this up.

@garethsb
Copy link
Contributor

Thanks 👍

@SSE4 SSE4 requested a review from uilianries May 23, 2021 13:27
@conan-center-bot conan-center-bot merged commit f4df50d into conan-io:master May 23, 2021
@SpaceIm SpaceIm deleted the fix/boost-stacktrace branch May 23, 2021 13:47
madebr pushed a commit to madebr/conan-center-index that referenced this pull request Jun 21, 2021
…W + bump dependencies

* bump zstd

* no os.rename

* allow to build on Macos out of the box

don't add dependency to libunwind

* remove duplicated dl system lib

dl is already added if Linux of FreeBSD to stacktrace_backtrace component

* no stacktrace_addr2line on Windows

it requires POSIX system: https://www.boost.org/doc/libs/develop/doc/html/stacktrace/configuration_and_build.html

* completly drop stacktrace_backtrace on Windows

stacktrace_backtrace doesn't seem to be built with MinGW, even if requested with backtrace lib.

* fix system libs for stacktrace components on Windows

see https://www.boost.org/doc/libs/develop/doc/html/stacktrace/configuration_and_build.html

* temporary hack for stacktrace with apple-clang

* add interface definition for stacktrace on Macos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants