arduino / arduino-cli

Arduino command line tool
https://arduino.github.io/arduino-cli/latest/
GNU General Public License v3.0
4.36k stars 382 forks source link

Library detection fails if header is not at top level #1275

Closed runger1101001 closed 1 year ago

runger1101001 commented 3 years ago

Bug Report

Current behavior

If I just include #include "encoders/as5048a/MagneticSensorAS5048A.h" library detection fails and (obviously) compilation isn't possible.

If I add a "Dummy.h" to the top level (src folder) of my library, and include that in my sketch, library detection succeeds.

Expected behavior

I'd expect the library detection to succeed, no matter what header I include from the library. Or I'd expect a way to explicitly include a library (is there one? I haven't found it :-( ).

Environment

Version: 2.0.0-beta.5 Date: 2021-04-15T10:43:43.670Z (2 weeks ago) CLI Version: 0.18.1 alpha [b3cf8e19]

Additional context

ubidefeo commented 3 years ago

hi @runger1101001 where are such files located?

could you share your project or a minimal not-working setup?

matthijskooijman commented 3 years ago

I believe this is currently the intended behavior, though I could not find it explicitly documented (I looked at https://arduino.github.io/arduino-cli/latest/sketch-build-process/#dependency-resolution and at https://arduino.github.io/arduino-cli/latest/library-specification/#layout-of-folders-and-files, where the latter does document that only (all) top-level include files will be added to the sketch when selecting the library in the IDE, and that only the top-level include files are expected to be an interface to the sketch and internal headers should be in subfolders, it does not explicitly document that headers in subfolders cannot trigger inclusion of the library).

I'm not entirely sure if this behavior was carefully considered, or just a by-product of the implementation (originally, libraries did not do recursive compilation and were really expected to be flat structures). I agree that it is somewhat surprising that these subdirectory-includes cannot trigger the inclusion of a library, but once a library is included in the build, they can be included with e.g. #include <foo/bar.h> normally (which is of course because of how gcc implements -I).

So maybe this is something to consider changing.

I think this change could be made without compatibility issues, since this would change behavior only for sketches and intallation where, during dependency resolution, an include of the for foo/bar.h is found to be unresolved by gcc. In the current situation, this include cannot be resolved by the IDE, so compilation will fail. So such a change would only change behavior for sketches which currently fail to compile, which poses no compatibility problem. There might be other subtle influences that I'm missing here, though.

runger1101001 commented 3 years ago

hi @runger1101001 where are such files located?

could you share your project or a minimal not-working setup?

https://github.com/simplefoc/Ardunio-FOC-drivers

It is an adjunct library to the main SimpleFOC library (a library for driving brushless motors using field-oriented control). It contains a variety of drivers and supporting code, which has different release cycles, different quality control, etc and is not part of the "core" of SimpleFOC.

Typically, people will use only one driver from the supporting project, hence the including of only headers from subfolders. For this type of setup it makes no sense to have a top-level header that includes all the drivers, and there is no realistic way to predict which combinations users might want. So they just directly include the headers for the parts they need.

Now that I know and understand the problem, I'm actually fine with a solution where a "dummy header" from the top level is included. Its not super-elegant but it will work and is easy to explain. users can just include the dummy header to trigger the library detection, and whatever other headers they need.

I do think it should be more explicitly documented somewhere. Maybe directly in the error message that comes for unresolved dependencies?

runger1101001 commented 3 years ago

And thank you very much for getting back to me so quickly!

umbynos commented 1 year ago

The current implementation looks correct, so I'm closing