raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.68k stars 917 forks source link

pico_generate_pio_header() doesn't set include path correctly for .pio.h files inside static libraries #157

Closed geurtv closed 4 months ago

geurtv commented 3 years ago

I have a static library with a CMakeLists.txt that starts something like this:

add_library(rp2-async STATIC)

set(SOURCE_FOLDER ${CMAKE_CURRENT_LIST_DIR}/source)

pico_generate_pio_header(rp2-async ${SOURCE_FOLDER}/hardware/uart/pio_uart.pio)

target_sources(rp2-async PRIVATE ${SOURCE_FOLDER}/hardware/uart/pio_uart.cpp)

Then in pio_uart.cpp I try to include the generated header file:

#include "pio_uart.pio.h"

Which doesn't compile because cmake didn't add the include path to the generated .pio.h. Change 'add_library' to 'add_executable' and it compiles just fine.

When I look at the implementation of pico_generate_pio_header() it adds include paths as PRIVATE for executables and as INTERFACE for libaries. If I change INTERFACE to PRIVATE, the source file compiles successfully. Now I don't know nearly enough about cmake to know if this is how to fix it, but I do argue that IMHO generated .pio.h files should not (ever) be part of the public interface of a library.

kilograham commented 3 years ago

We exclusively use INTERFACE libraries in the SDK (since they often depend on configuration defines set on the target executable)... that is why "INTERFACE" - everything is public since they are all compiled together.

So I would suggest you do that unless you have a particular reason for creating a static library.

That said, we should fix the cmake to do the correct thing for a static library

geurtv commented 3 years ago

Right ... to paraphrase myself: I barely know cmake. Interface library makes sense ... interface library it is.

Also, I just noticed static libraries don't mix that cleanly with pico-sdk anyway. Several SDK files are now compiled twice: once for the library and once for the executable. I guess it might be well defined which of the two .obj files the linker then uses, but it's probably not always harmless. Anyway, interface library it is.

kilograham commented 4 months ago

seems this was already closed in 1.3.0