ObKo / stm32-cmake

CMake for stm32 developing.
MIT License
1.2k stars 339 forks source link

stm32: common: Fix '_stm32_generate_file' #314

Closed crvux closed 1 year ago

crvux commented 1 year ago

After cmake 3.10 the target property OUTPUT_NAME is not set by default. This commit adds a check to see if the target property has a value.

atsju commented 1 year ago

We require CMake 3.16 minimum to use the repository (see https://github.com/ObKo/stm32-cmake/search?q=cmake_minimum_required)

knowing this, do we need to check both cases ? If yes, what makes OUTPUT_NAMEgenerated or not (for my information only) ?

crvux commented 1 year ago

knowing this, do we need to check both cases ?

OUTPUT_NAME sets the base name for output ELF file. This might be useful, for example, when we want append git hash to our ELF name. In cmake 3.10 and older, the OUTPUT_NAME property defaults to the logical target name (https://cmake.org/cmake/help/v3.10/prop_tgt/OUTPUT_NAME.html) and we can easily use value of this property to determine output ELF name. In newer cmake versions this property is not set by default (https://cmake.org/cmake/help/v3.16/prop_tgt/OUTPUT_NAME.html) and we need to check if the value is set. My mistake is that I didn't notice the version of cmake we're using. So there is no need to check both cases.

atsju commented 1 year ago

My mistake is that I didn't notice the version of cmake we're using. So there is no need to check both cases.

Many things changed arround Cmake 3.16 It would be really complex to maintain full compatibility on many projects so here we decided to just force 3.16 minimum. Moreover, there is no reason to maintain soo old backcompatibility. If a project using our repo is active, it will easily be able to upgrade. And if it's inactive, it can use older version of our repo with any cmake version. Only thing that may be needed when using a too recent version of cmake with old project is to set some properties to mimic older cmake versions.

TLDR: it's up to cmake to maintain backcompatibility. It's reasonable to force people to use cmake>=3.16

crvux commented 1 year ago

The commit fixes the function for cmake>=3.16. Is there anything that needs to be changed to merge ? :)

atsju commented 1 year ago

@Hish15

Hish15 commented 1 year ago

I don't get your issue @crvux. What problem did you had before the change you propose ?

If cmake decide not to set it by default we most probably don't want to force it. Keep in mind, this project is some kind of wrapper around cmake to deal with stm32 MCUs. User are free to change any default on their project.

crvux commented 1 year ago

What problem did you had before the change you propose ?

When using 'stm32_generate_binary_file' the name of the generated binary file will be 'TARGET_OUTPUT_NAME-NOTFOUND.bin'. This is due to the reason described in the my post above. Proposal fixes this issue.