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

Additional include paths libs #502

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

facchinm
Copy link
Member

@facchinm facchinm commented Nov 29, 2019

Fixes #501

@facchinm facchinm requested a review from cmaglie December 2, 2019 10:57
@masci masci added this to the 0.8.0 milestone Dec 31, 2019
@masci masci modified the milestones: 0.8.0, 0.9.0 Feb 14, 2020
@rsora rsora modified the milestones: 0.9.0, 0.10.0 Feb 21, 2020
@oclyke
Copy link

oclyke commented Mar 10, 2020

Thanks @facchinm!

I have done some initial testing. There seems to be a problem in the 'Detecting Libraries' build step.

Procedure:

  1. Replace binary arduino-builder in my installment with the one you provided (windows)
  2. Write a sample library that needs to access the API of an "immutable" submodule (attached as .zip - called aipt_lib for 'additional include path testing')

The library.properties file includes the line:

additional_include_paths=src/ossdl/include

This is because I expect the builder to create an include in the tool like "-I{path_to_lib}/src/ossdl/include" so that the tools can find the header file of the submodule
aipt_lib.zip

  1. Write a test sketch
#include "aipt.h"

void setup() {
  // put your setup code here, to run once:

  Serial.begin(115200);
  delay(500);

  testFun1();         // make sure standard Arduino library features working
  printOSSDLName();   // test functionality from the 'submodule'

  Serial.print("Done");
}

void loop() {
  // put your main code here, to run repeatedly:

}
  1. Try to compile (Board: Uno)

Results

  • Running as-is fails when detecting libraries used. I don't see the additional include path being used in the build lines that are used to detect libraries, so causing errors and aborting.
  • If you fix the include problem (comment out #include "ossdl.h" and uncomment #include "ossdl/include/ossdl.h" in 'aipt.h') then compilation continues which reveals that the correct include path is being used later on in the build process. E.g.
    "-IC:\\Users\\owen.lyke\\Documents\\Arduino\\libraries\\aipt_lib\\src" "- 
    IC:\\Users\\owen.lyke\\Documents\\Arduino\\libraries\\aipt_lib\\src\\ossdl\\include"
    

More thoughts

How does library detection work? I think there lies the cause of the problem

  1. Run preprocessor on the main sketch - record what includes there were. Does not seem to require knowledge of any libraries (good...)
  2. Run preprocessor on main sketch again but this time add a -I flag for the library's src folder. this step seems to be missing -I flags for the additional include paths I think that at this time the library has already been loaded so the additional include paths could be included here too
  3. Run preprocessor on library source files. This step does include -I flags for the additional include paths

A small nit - I notice that changing library.properties does not signal a change of the library to the builder and so a cached version of the library may be used, however if the additional_include_paths field is changed that does affect compilation. I'm not sure what can be done about this. In the worst-case scenario I'm sure people (library developers) can just deal with it and re-save their files after changing that field.

@facchinm facchinm force-pushed the additional_include_paths_libs branch from a11f866 to 6058360 Compare March 11, 2020 10:22
@facchinm
Copy link
Member Author

@oclyke
first of all, thanks for taking the time to test it.
Something changed in the cache code from the time I first wrote the patch, so the build was not behaving as intended. I pushed an extra commit 4853add that fixes the preprocessor phase.
Said that, I'd really love if people writing libraries could just use real relative includes (without hoping that the buildsystem fixes all the -I for them 🙂 )

@oclyke
Copy link

oclyke commented Mar 11, 2020

@facchinm
Sweet - if I have time I will try to build the arduino-builder executable (new process for me) and test it out. I am very excited about this potential addition to Arduino :)

I agree that authors of original libraries should write proper relative includes. But even if every project did that this feature would still be a valuable addition to Arduino -- why? Because now, with a very simple mechanism (git submodules), we can 'symbolically' include and build off of other open-source projects. All the while fighting code duplication, easing maintenance, and more strongly supporting other open-source projects.

See, I would not expect the author of an original library to use the additional_include_paths feature - they should write includes correctly for their library. But when they include another work as a sub-folder with its own root then they will be very glad to have this ability.

@SciLor
Copy link

SciLor commented Jul 28, 2020

Could you provide another build to test?

@ubidefeo
Copy link

@SciLor
since this commit hasn't been merged yet (pending review),
you could checkout the additional_include_paths_libs branch of the https://github.com/facchinm/arduino-cli fork
Instructions to build can be found here, and it's not that much of a hassle
https://arduino.github.io/arduino-cli/dev/CONTRIBUTING/#building-the-source-code

We will end up merging this PR once we get to review it, it shouldn't take long
u.

@SciLor
Copy link

SciLor commented Jul 28, 2020

@ubidefeo
I tried to build it myself, downloaded go/task but building doesn't seem to work. There seems something missing in the Prerequisites.

C:\temp\arduino-cli>task build
"tr": executable file not found in $PATH
"grep": executable file not found in $PATH
io: read/write on closed pipe
task: Command "echo `go list ./... | grep -v legacy | tr '\n' ' '`" in taskvars file failed: exit status 1

@per1234
Copy link
Contributor

per1234 commented Jul 28, 2020

Hi @SciLor. These errors are caused by the use of shell commands that aren't available when using Windows cmd or PowerShell. Fortunately, there is a nice tool called "Git BASH" that is included with Git for Windows. If you have Git installed, you already have it on your computer. If not, you can download it from the official Git website:
https://git-scm.com/download/win

If you then start C:\Program Files\Git\git-bash.exe and run task build you should have a successful build of Arduino CLI.

@SciLor
Copy link

SciLor commented Jul 28, 2020

@per1234 thanks for the hint. Works as charm.
task build will just build the arduino-cli.exe for me. How to build the arduino-builder.exe?

@per1234
Copy link
Contributor

per1234 commented Jul 28, 2020

@SciLor there's no need for arduino-builder.exe. Just use the arduino-cli.exe executable that was generated via task build directly from the command line:
https://arduino.github.io/arduino-cli/latest/commands/arduino-cli/

The additional_include_paths field of library.properties works just as well with Arduino CLI and it will allow you fully test the functionality of this PR.

@SciLor
Copy link

SciLor commented Jul 28, 2020

@per1234 I can't use arduino-cli as I am using energia (arduino fork) that is still using the arduino-builder.

@cmaglie cmaglie removed this from the 0.12.0 milestone Sep 14, 2020
@cmaglie cmaglie added this to the 0.14.0 milestone Sep 14, 2020
@jan-chvojka
Copy link

Any progress on this? I updated Arduino and have wondered why is my project not compiling anymore. It didn't kept my additional libraries I put to the installation folder. So I thought I will keep the libraries in different folder. What was my surprise as a developer to find out that there is no way to add additional paths...

@ubidefeo
Copy link

@jan-chvojka

I updated Arduino

do you mean the arduino-cli?

It didn't kept my additional libraries I put to the installation folder.

which is your installation folder?

out that there is no way to add additional paths...

you can add additional libraries in arduino-cli using the --libraries flag but we currently found a bug and are trying to address it

@per1234 per1234 added the type: enhancement Proposed improvement label Feb 3, 2021
@github-actions github-actions bot closed this Mar 30, 2021
@per1234 per1234 reopened this Mar 30, 2021
@silvanocerza silvanocerza removed this from the 0.14.0 milestone May 11, 2021
@per1234 per1234 added the topic: code Related to content of the project itself label Jan 15, 2022
@arielscarpinelli
Copy link

Is there anything we could do to accelerate merging this one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Libraries to Extend Include Path