Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include library paths used by libraries in the compiler search path. #1250

Closed
wants to merge 2 commits into from

Conversation

giseburt
Copy link

This pull fixes a case where libraries that a sketch uses indirectly (other libraries is uses require them) need to be included directly in order to compile.

It is necessary to #include a library's file in your sketch in order for the IDE to add that library's path to the compiler command line. This patch searches the files that are included as well, adding dependent libraries to the search path. This removes then need to directly #include indirectly used libraries.

The upside is that this will not break any existing code. The workaround that's needed now will continue to work, but will simply become unnecessary.

An example, in the Adafruit SSD1331 OLED driver test sketch (https://github.com/adafruit/Adafruit-SSD1331-OLED-Driver-Library-for-Arduino/blob/master/examples/test/test.pde) contains this code:

#include <Adafruit_GFX.h>
#include <Adafruit_SSD1331.h>
#include <SPI.h>

With this pull, only the directly relevant library to the sketch is needed to be added, and so it could be simplified to:

#include <Adafruit_SSD1331.h>

The library itself includes those files, and so the IDE is able to locate them and add them to the include path. Of course, the old code will continue to work as well.

LibraryA.h can now include LibraryB.h, and the path for LibraryB will be added to the compile line.
@Lauszus
Copy link
Contributor

Lauszus commented Dec 5, 2013

Are you ever going to merge this? I think it just confuses users if they are present with a lot extra includes in the top of the sketch.
We have recently needed to add an include statement to every example in the USB Host shield library: felis/USB_Host_Shield_2.0@edf9682, as we started to use another SPI library for the Teensy 3. This just makes it even more annoying, as 99% of our users are using different boards.

@jantje
Copy link

jantje commented Dec 5, 2013

+1
This also takes away a big part of the need of library specification 1.5

@matthijskooijman
Copy link
Collaborator

Looking at the actual code, I have a few remarks:

  • There are two (nearly) identical pieces of code added that scan the code for #includes. I even think this code is already present somewhere (for scanning .ino files), so this would make three copies. This code should be generalized so it can be reused instead.
  • The current code scans libraries that are included from the main sketch for dependencies, but I think it does not scan those dependencies recursively for further dependencies. e.g., if the sketch include LibA, which includes LibB, which includes libC, libC will not be added to the link.

In other words, I think most of the code should be written differently. I might have a go at this if I can find some time.

As for the library spec Jantje mentioned, I think he's right saying that if this autodetection is implemented, the explicit library dependencies which were proposed for the 1.5-style libraries is no longer strictly needed. It could still be useful to specify version constraints, but the need for that is a lot less pressing. I still think we need a proper library spec for metadata and better structured libraries, but that's another matter.

In any case, I think it makes sense to implement auto detection of library dependencies, since:

  • It also works like this for sketches.
  • Manually specifying dependencies is more work and more error-prone

If we fix this issue, btw, I think we should also try to fix #636 at the same time.

@matthijskooijman
Copy link
Collaborator

Note that this pullrequest fixes #236

@jantje
Copy link

jantje commented Dec 8, 2013

I think the fix for #636 is the functionally the same as this fix. The only difference is that this is triggered at "import time" while #636 is triggered a "verify" time

@matthijskooijman
Copy link
Collaborator

Huh? You're proposing that, when you import a library through the menu, it should scan that library for any libraries it uses and add #includes for them too? That sounds like the wrong way to fix this: if the imported library changes and later requires an extra library, any sketches created before will break.

It seems to me that this autodetection should work at verify time in all cases?

Or did I misunderstand your point?

@jantje
Copy link

jantje commented Dec 8, 2013

I think there exists 2 options. I did not advocate one above the other. I assumed this was one and #636 was the other.

@giseburt
Copy link
Author

giseburt commented Dec 8, 2013

The point of this pull request was to simply add intelligence to the
include path of the compiler. I think altering the code any more than is
necessary is a bad idea.

It should follow #includes of dependent libraries, and be able to do so
recursively. That requires that the code have the includes in the first
place, however.

-Rob

On Sunday, December 8, 2013, jantje wrote:

I think there exists 2 options. I did not advocate one above the other. I
assumed this was one and #636https://github.com/arduino/Arduino/issues/636was the other.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1250#issuecomment-30091740
.

@jantje
Copy link

jantje commented Dec 8, 2013

och I get it. This one is about the ino file and #636 is about other c cpp files in the sketch folder.
Both are at compile time.
Sorry for the confusion.

@giseburt
Copy link
Author

giseburt commented Dec 8, 2013

No worries. I was just clearing up my intent.

I'm not sure without checking the code if it works now (with this pull
request, I mean), but I do believe both cases should work or be fixed to.
Any dependent libraries and the dependencies should be automatically added
to the compilers search paths.

On Sunday, December 8, 2013, jantje wrote:

och I get it. This one is about the ino file and #636https://github.com/arduino/Arduino/issues/636is about other c cpp files in the sketch folder.
Both are at compile time.
Sorry for the confusion.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1250#issuecomment-30092823
.

cmaglie added a commit to cmaglie/Arduino that referenced this pull request Dec 9, 2013
@cmaglie
Copy link
Member

cmaglie commented Dec 9, 2013

Rob, this pull request cannot be merged cleanly any more, too much changes happened upstream in the meantime.

I reworked it and I'll publish in another pull request for review.

C

@giseburt
Copy link
Author

giseburt commented Dec 9, 2013

Ok, thank you.

On Sunday, December 8, 2013, Cristian Maglie wrote:

Rob, this pull request cannot be merged cleanly any more, too much changes
happened upstream in the meantime.

I reworked it and I'll publish in another pull request for review.

C


Reply to this email directly or view it on GitHubhttps://github.com//pull/1250#issuecomment-30097449
.

@matthijskooijman
Copy link
Collaborator

I think this pullrequest can be closed in favor of #1726?

@cmaglie
Copy link
Member

cmaglie commented Jan 21, 2014

Yes, I'm closing this one.

@cmaglie cmaglie closed this Jan 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants