Skip to content

Conversation

ismaell
Copy link
Contributor

@ismaell ismaell commented Aug 22, 2021

The Q_OS_UNIX macro is inappropriate for detection because many UNIX-like
platforms may lack backtrace support in the libc. E.g.: Darwin / Mac OS X,
Musl libc, OpenBSD, OpenIndiana.

@jonaski
Copy link
Contributor

jonaski commented Aug 22, 2021

I think it's better to use: find_package(Backtrace)

@ismaell
Copy link
Contributor Author

ismaell commented Aug 22, 2021

@jonaski Thanks, I'll try that.

@ismaell
Copy link
Contributor Author

ismaell commented Aug 22, 2021

@jonaski is that enough?

#include <QtGlobal>
#ifdef Q_OS_UNIX
#ifdef HAVE_BACKTRACE
#include <execinfo.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

execinfo.h also may not be correct. FindBacktrace recommends generating a file that includes ${Backtrace_HEADER}
https://cmake.org/cmake/help/v3.0/module/FindBacktrace.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's why I was asking... I went for the trivial change...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Solved. I think.

The Q_OS_UNIX macro is inappropriate because many UNIX-like platforms may
lack backtrace support in the libc. E.g.: Darwin / Mac OS X, Musl libc,
OpenBSD, OpenIndiana.
)

find_package(Backtrace)
configure_file(core/conf_backtrace.h.in conf_backtrace.h)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should only do this if the package was found. Otherwise, Backtrace_HEADER won't be defined.

Copy link
Contributor 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? the header file needs to be produced, always...

Copy link
Contributor Author

@ismaell ismaell Aug 27, 2021

Choose a reason for hiding this comment

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

also, the include is guarded by an ifdef...

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah. I didn't notice that you had moved the ifdef.

@hatstand hatstand merged commit 628ff65 into clementine-player:master Aug 31, 2021
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