a9183756-gh / Arduino-CMake-Toolchain

CMake toolchain for all Arduino compatible boards
MIT License
135 stars 40 forks source link

_library_search_process excludes valid libraries when there isn't a matching header. #19

Closed raphael-bmec-co closed 4 years ago

raphael-bmec-co commented 4 years ago

In Arduino-CMake-Toolchain-release-1.1-dev\Arduino\System\BoardBuildTargets.cmake at the end of the function function(_library_search_process lib search_paths_var search_suffixes_var return_var) is this final check:

    # Although we got the match, let us search for the required header within the folder
    file(GLOB_RECURSE lib_header_path "${matched_lib_path}/${lib}.h*")
    if (NOT lib_header_path)
        set ("${return_var}" "${lib}-NOTFOUND" PARENT_SCOPE)
        return()
    endif()

I am not sure if this applied to other libraries. But the ESP32 BLE Arduino library has some caveats:

  1. The library folder name is BLE. So the call has to be target_link_arduino_libraries(arc003-2 AUTO_PUBLIC PRIVATE BLE) even though that isn't the library name.
  2. The library name is 'ESP32 BLE Arduino'. The mismatch here does not seem to be an issue (probably because BLE is still in the name).
  3. There is no BLE.h or BLE.cpp in the src as the library is comprised of other files. This is the core issue that prevents this library from being found.

Possible fixes are to:

  1. Modify the final check in _library_search_process to maybe include a flag to skip that check or remove it.
  2. Add BLE.h or BLE.cpp to the library (not ideal).

For me, I have commented out the check for the time being.

Looking forward to your thoughts.

a9183756-gh commented 4 years ago

Thanks for your analysis and raising a separate issue for this.

I can see that the library search cannot not find "ESP32 BLE Arduino", and whereas Arduino IDE is able to find it.

In this case,

As the library folder name does not match with the include name (as required by the specification), the search fails. Arduino-IDE implements the search differently through pre-processing, and hence is able to succeed.

Temporary workaround: (1) Rename ESP32_BLE_Arduino folder to BLEDevice (Search for ESP32_BLE_Arduino in your home or documents folder). (2) Use "BLEDevice" in target_arduino_link_libraries.

Resolution for this current issue: Although this library does not seem to follow the specification, it seems to leave some info in library.properties, that could be used to fix the issue.

Permanent resolution: Implement the search logic similar to that of Arduino IDE.

Final check in _library_search_process was added for another issue. I will analyze how to skip this check if needed.

raphael-bmec-co commented 4 years ago

Thank you.

I have uncommented the final check and implemented the workaround you have proposed.

simoleone commented 4 years ago

I think I found the same issue with Bluefruit52Lib when including the file bluefruit.h. Similarly, the library.properties file specifies includes=bluefruit.h.

Edit: It seems like the spec for libraries also has some info about how the include files are determined from library.properties. A comma-separated list in includes, or if not present, any .h file at the top-level in src/.

a9183756-gh commented 4 years ago

Yes, I saw that libraries spec as well. Although it doesn't specify the usage of includes field in the context of library search, it makes sense for the toolchain to use this field to solve this issue (Although, I believe, Arduino IDE may work even without this field). The current implementation in the toolchain tried to avoid reading all the library.properties files, and the specification seemed to favour this by specifying the algorithm in terms of just folder names. Currently implemented algorithm thus takes in include name and easily arrives at the best matching library folder. However, the algorithm required to fix the issue is to take in include name and arrive at the best library folder that provides the include, which means

  1. Scan all libraries.properties, and maintain a list of libraries and the corresponding includes, which will be additionally used for finding the library match.
  2. Allow target_link_arduino_libraries to take library name as well, in addition to include name.
  3. If user installs/removes any libraries (e.g. based on compilation error), re-index the libraries,
  4. Avoid linking with unnecessary libraries during automatic linking (Due to the difference in the way Arduino IDE and the toolchain implements automatic linking).

Let us attempt at least (1) in the current development version, for this issue #19. We will attempt a better approach that matches with Arduino IDE in the next release.

a9183756-gh commented 4 years ago

EDIT: With another fix (Make library target names consistent), this fix should work now correctly. Please verify.

Fix for this issue is available with the change #22. Although all the required features are in place, the algorithm is still not exactly the same as Arduino IDE, and hence some difference is expected. Notably, if a library provides \<some-include>.h, and if the library folder name does not match with \<some-include> and if \<some-include>.h is not listed in the includes field of the library.properties, then automatic detection of such a library is explicitly disabled to avoid the error mentioned in point (4) in the above comment. It is still possible to link with such a library explicitly using the library name (instead of the include name \<some-include>) in target_link_arduino_libraries.

a9183756-gh commented 4 years ago

Fixed in release-1.1-dev. Closing the issue.