[cmake-developers] FindBacktrace.cmake

Alexander Neundorf neundorf at kde.org
Mon Jul 22 16:07:29 EDT 2013


On Wednesday 03 July 2013, Vadim Zhukov wrote:
> 2013/7/3 Brad King <brad.king at kitware.com>:
> > On 7/3/2013 11:09 AM, Vadim Zhukov wrote:
> >> I'm an OpenBSD developer, working mostly on porting software. Most of
> >> my work is related to CMake-based land, including modern KDE and some
> >> other Qt4-based stuff.
> > 
> > Great, thanks for coming to us.
> > 
> >> Here is a module I constructed a few months ago. It's helpful for
> >> programs that want to use GNU-style backtrace(3) routine, which could
> >> be found in different libraries and be declared in different headers
> >> across OSes outta there.
> > 
> > Thanks for working on this.  It looks similar to FindThreads in that
> > the results could be builtin to the system libraries or in one of
> > several other vendor-specific libraries.  Don't model the module after
> > FindThreads though as that is one of the oldest modules and does not
> > fully use modern conventions.
> > 
> > The BACKTRACE_INCLUDE_DIR and BACKTRACE_LIBRARY variables should
> > not be documented as results that consuming projects should use
> > directly.  They are inputs to the module that end users edit in
> > the cache.  The results go in BACKTRACE_INCLUDE_DIRS and
> > BACKTRACE_LIBRARIES.  Look at how FindProtobuf.cmake separates
> > the list of variables in its documentation, for example.
> 
> Thank you, that was exactly info I was looking for!
> 
> > The line
> > 
> >  set(BACKTRACE_LIBRARIES ${CMAKE_REQUIRED_LIBRARIES})
> > 
> > looks a bit strange to me.  The CMAKE_REQUIRED_LIBRARIES variable
> > is an input to the check_* APIs and should not be depended upon
> > or used by a find module.  When calling check_symbol_exists you
> > should set CMAKE_REQUIRED_* to known values not depending on the
> > calling context.  In the case that some non-standard/non-system
> > library is needed to provide the symbol it should be set by the
> > user through the BACKTRACE_LIBRARY code path.
> 
> Yes, I was not sure if this should be done this way either. Other variants
> are:
> 
> 1. find_library("c") - not as stupid as it looks from the first time,
> because C++ compiler could be called with -nostdlib, for example (and
> this was the reason for inclusion of CMAKE_REQUIRED_LIBRARIES
> initially);
> 
> 2. Just make sure BACKTRACE_LIBRARIES is empty: if the developer wants
> to shoot himself in the foot with -nostdlib, then he's responsible for
> the all consequences; and if he wants to avoid -lc, well, he'll
> successfully avoid it.
> 
> I'd go with either way. For this try I choosed (2).
> 
> > Otherwise, the module looks like a great start!
> 
> Thank you for review. I'm sending updated module now. Aside tweaks
> based on your input, I realized that there should be a way for the
> user to specify BACKTRACE_HEADER, too. I hope that I did not
> overcomplicate things.


A few more style-comments:

please use empty else() and endif() clauses. Most of the cmake modules have 
been converted to this style recently.

The variables should not use "BACKTRACE" as prefix, but "Backtrace", so they 
match the name of the find-module exactly.


Why do you make BACKTRACE_HEADER INTERNAL ? I think INTERNAL variables are 
completely hidden, i.e. not accessible, e.g. in cmake-gui. 

You use message() without the "STATUS" keyword. I think it should be used.

Can you switch to the new-style FPHSA() ?
This would be
find_package_handle_standard_args(Backtrace
                       REQUIRED_VARS ${_BACKTRACE_STD_ARGS} )

Thanks
Alex



More information about the cmake-developers mailing list