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

Issue #3635 Added the sketch folder to the path #3717

Closed
wants to merge 1 commit into from
Closed

Issue #3635 Added the sketch folder to the path #3717

wants to merge 1 commit into from

Conversation

ricklon
Copy link
Contributor

@ricklon ricklon commented Aug 22, 2015

Issue #3635 Added the sketch folder to the include path.

includeFolders.add(prefs.getFile("build.core.path"));
    includeFolders.add(sketch.getFolder());
    if (prefs.getFile("build.variant.path") != null)

Issue #3635 Added the sketch folder to the include path.
```
includeFolders.add(prefs.getFile("build.core.path"));
    includeFolders.add(sketch.getFolder());
    if (prefs.getFile("build.variant.path") != null)
```
@NicoHood
Copy link
Contributor

Just to understand this correct:
This way you can open a new tab in the IDE, name it libSetting.h and include it within the library. This way one can easily change the config without modifying the source? Is this correct?

If yes I'd definitely agree with the PR, havent tested it though. Can anyone trigger the bot to build this?

Is there also a way to ignore the include if the config file is not inside the sketch folder? Like a default config? Or at least a proper error message when the file is not included?

@ricklon
Copy link
Contributor Author

ricklon commented Aug 23, 2015

Thanks!

You can use an #if defined() to detect the external header from the
library. If not detected then set a default. Is possible.

On Sat, Aug 22, 2015, 11:56 PM Nico notifications@github.com wrote:

Just to understand this correct:
This way you can open a new tab in the IDE, name it libSetting.h and
include it within the library. This way one can easily change the config
without modifying the source? Is this correct?

If yes I'd definitely agree with the PR, havent tested it though. Can
anyone trigger the bot to build this?

Is there also a way to ignore the include if the config file is not inside
the sketch folder? Like a default config? Or at least a proper error
message when the file is not included?


Reply to this email directly or view it on GitHub
#3717 (comment).

@ffissore ffissore self-assigned this Aug 24, 2015
@NicoHood
Copy link
Contributor

But how can you detect a header? You need to include it before to detect it. But before the compiler will complain about a missing header. So this wont work here, the header needs to be there. What we need would be an order of priority here. so it first looks in the sketch and then inside the library. And also that a doubled name wont cause weird problems.

@matthijskooijman
Copy link
Collaborator

@ArduinoBot build this please

@ricklon
Copy link
Contributor Author

ricklon commented Aug 24, 2015

It doesn't fail silently. So it can be figured out. I have had it in MPIDE
for 4 years without problem. Library designers have come to depend on it.

--Rick

On Mon, Aug 24, 2015, 12:12 PM Matthijs Kooijman notifications@github.com
wrote:

@ArduinoBot https://github.com/ArduinoBot build this please


Reply to this email directly or view it on GitHub
#3717 (comment).

@NicoHood
Copy link
Contributor

Meaning if you use the external settings option it is required to have it there.
Still a nice addition, I'll report my testings after the build finished.

@NicoHood
Copy link
Contributor

I want to note that this only works if the sketch is saved. Normally you can compile the sketch without saving.

From the .ino sketch the .test file is always seen the most up to date (without saving) and the library will only recognize changes if the sketch is saved.

Since this is bad for testing purposes (i normally dont save any debug stage) there needs to be further testing for this PR.

Otherwise the function works as expected. But as said, if the file does not exists an inclusion error will pop up:

In file included from /sketchbook/libraries/HID-Project/src/HID-Project.h:34:0,
                 from sketch_aug24b.ino:13:
/sketchbook/libraries/HID-Project/src/Consumer.h:27:18: fatal error: test.h: No such file or directory
 #include "test.h"
                  ^
compilation terminated.
Error compiling.

(this error/behavior has nothing to do with my alinkage PR since this build here has not included this fix. So the error is related to this PR only)

@matthijskooijman
Copy link
Collaborator

I actually think that means that the temporary directory where all sketch files are preprocessed or copied to needs to be in the include path instead (the copying of extra files is a recent addition, I believe).

@NicoHood
Copy link
Contributor

includeFolders.add(sketchBuildFolder);

Works for me without saving. Blathijs tip seemed correct.

@ricklon
Copy link
Contributor Author

ricklon commented Aug 25, 2015

Good find. Should I update the pull request?

On Tue, Aug 25, 2015, 11:57 AM Nico notifications@github.com wrote:

includeFolders.add(sketchBuildFolder);

Works for me without saving. Blathijs tip seemed correct.


Reply to this email directly or view it on GitHub
#3717 (comment).

@NicoHood
Copy link
Contributor

yes please. Use git add and then git commit --amend to edit the commit. git push --force to upload it again (in case you dont know how to do this ;) )

@matthijskooijman can you build this again then? :D

@ricklon
Copy link
Contributor Author

ricklon commented Aug 25, 2015

I've tested this in three configurations on Mac OS X:

I had a library that included the following file:
HTTPServerConfig.h

I tested each configuration:

  1. Newly creating the file.
  2. Then renaming the file "HTTPServerConfig111.h".
  3. Rename to the original file.
  4. Then deleting the file.
  5. Original modification: works in all cases Iv'e created
  List<File> includeFolders = new ArrayList<File>();
    includeFolders.add(prefs.getFile("build.core.path"));
    includeFolders.add(sketch.getFolder());
    //includeFolders.add(sketchBuildFolder);

  1. Suggested modification: works but still finds the deleted file cached.
  List<File> includeFolders = new ArrayList<File>();
    includeFolders.add(prefs.getFile("build.core.path"));
    //includeFolders.add(sketch.getFolder());
    includeFolders.add(sketchBuildFolder);
  1. Combined modification: works but still finds the deleted file cached.
  List<File> includeFolders = new ArrayList<File>();
    includeFolders.add(prefs.getFile("build.core.path"));
    includeFolders.add(sketch.getFolder());
    includeFolders.add(sketchBuildFolder);

Renaming seams to work properly.

--Rick

On Tue, Aug 25, 2015 at 9:01 AM, Nico notifications@github.com wrote:

yes please. Use git add and then git commit --amend to edit the commit.
git push --force to upload it again (in case you dont know how to do this
;) )

@matthijskooijman https://github.com/matthijskooijman can you build
this again? :D


Reply to this email directly or view it on GitHub
#3717 (comment).

Co-founder
Fair Use Building and Research (FUBAR) Labs
http://fubarlabs.org

@ricklon
Copy link
Contributor Author

ricklon commented Aug 26, 2015

I wrote a really quick test for the include library. I can modify it as
needed.
https://github.com/ricklon/libinclude

--Rick

On Tue, Aug 25, 2015 at 2:38 PM, Rick Anderson rick.rickanderson@gmail.com
wrote:

I've tested this in three configurations on Mac OS X:

I had a library that included the following file:
HTTPServerConfig.h

I tested each configuration:

  1. Newly creating the file.
  2. Then renaming the file "HTTPServerConfig111.h".
  3. Rename to the original file.
  4. Then deleting the file.
  5. Original modification: works in all cases Iv'e created
  List<File> includeFolders = new ArrayList<File>();
    includeFolders.add(prefs.getFile("build.core.path"));
    includeFolders.add(sketch.getFolder());
    //includeFolders.add(sketchBuildFolder);

  1. Suggested modification: works but still finds the deleted file cached.
  List<File> includeFolders = new ArrayList<File>();
    includeFolders.add(prefs.getFile("build.core.path"));
    //includeFolders.add(sketch.getFolder());
    includeFolders.add(sketchBuildFolder);
  1. Combined modification: works but still finds the deleted file cached.
  List<File> includeFolders = new ArrayList<File>();
    includeFolders.add(prefs.getFile("build.core.path"));
    includeFolders.add(sketch.getFolder());
    includeFolders.add(sketchBuildFolder);

Renaming seams to work properly.

--Rick

On Tue, Aug 25, 2015 at 9:01 AM, Nico notifications@github.com wrote:

yes please. Use git add and then git commit --amend to edit the commit.
git push --force to upload it again (in case you dont know how to do this
;) )

@matthijskooijman https://github.com/matthijskooijman can you build
this again? :D


Reply to this email directly or view it on GitHub
#3717 (comment).

Co-founder
Fair Use Building and Research (FUBAR) Labs
http://fubarlabs.org

Co-founder
Fair Use Building and Research (FUBAR) Labs
http://fubarlabs.org

@NicoHood
Copy link
Contributor

NicoHood commented Sep 1, 2015

I've now tried your example.

The weird thing is, that my patch (2nd version) first worked and now seems to always throw a compiler error about the missing header file. I dont know why, its the same example I am using (yours). It worked 5min ago...

However the problem still is that version 1 is not working correct unless you save the sketch. So this is not of any use for me.

The best thing would be that it includes the user sketch preferred (if exists) and then tries to use the default config.

Another option would be to globally try to include a file named "UserConfig.h" if it exists so any library can use this if desired, and if not the Arduino IDE somehow adds a wrapper for this (an empty file). This way you have a single file for all library settings which can be used for many libraries and always works the same. What about that?

I havent tried version 3 as I dont think this is correct. I am not happy with any of these 3 solutions, sadly. I really like to see this feature (as I also need it right now for my libraries).

@ricklon
Copy link
Contributor Author

ricklon commented Sep 1, 2015

So the issue is when creating a new sketch depending on the library then
compile without saving there is a failure?

I'm sure I can find something for it.

On Tue, Sep 1, 2015 at 11:04 AM, Nico notifications@github.com wrote:

I've now tried your example.

The difference is that you include the header from the library (which I
did not do) but which makes totally more sense.

The weird thing is, that my patch (2nd version) first worked and now seems
to always throw a compiler error about the missing header file. I dont know
why, its the same example I am using (yours). It worked 5min ago...

However the problem still is that version 1 is not working correct unless
you save the sketch. So this is not of any use for me.

The best thing would be that it includes the user sketch preferred (if
exists) and then tries to use the default config.

Another option would be to globally try to include a file named
"UserConfig.h" if it exists so any library can use this if desired, and if
not the Arduino IDE somehow adds a wrapper for this (an empty file). This
way you have a single file for all library settings which can be used for
many libraries and always works the same. What about that?

I havent tried version 3 as I dont think this is correct. I am not happy
with any of these 3 solutions, sadly. I really like to see this feature (as
I also need it right now for my libraries).


Reply to this email directly or view it on GitHub
#3717 (comment).

Co-founder
Fair Use Building and Research (FUBAR) Labs
http://fubarlabs.org

@NicoHood
Copy link
Contributor

NicoHood commented Sep 1, 2015

add

#ifdef TESTCONFIG
#error
#endif

to your sketch. comment out th definition in the .h file. It wont be updated. That what I commented days ago already. Now my patch (2nd version) doesnt seem to work at all. It must be something trivial I think? --> yep somehow my change wasnt in the git anymore, there was no patch applied. no wonder that it didnt work.

So well version2 still works better. If both path (lib and sketch) have the same header file, the libraries header is preferred. This way we cannot create a default setting. Otherwise that'd be perfect.

@NicoHood
Copy link
Contributor

NicoHood commented Sep 1, 2015

Have a look at this:
#3757

So it seems that version2 is correct and the file deletion is a general IDE bug.
I've added a feature to fall back to a default library config, if no user config exists.

@ricklon
Copy link
Contributor Author

ricklon commented Sep 1, 2015

Good find. I can amend the pull request with option 2. Then start on
fixing that other bug.

On Tue, Sep 1, 2015, 12:55 PM Nico notifications@github.com wrote:

Have a look at this:
#3757 #3757

So it seems that version2 is correct and the file deletion is a general
IDE bug.
I've added a feature to fall back to a default library config, if no user
config exists.


Reply to this email directly or view it on GitHub
#3717 (comment).

@ricklon
Copy link
Contributor Author

ricklon commented Sep 1, 2015

Oh. Hang on you took care of it. How about we can with your updated
issue. Good?

On Tue, Sep 1, 2015, 1:00 PM Rick Anderson rick.rickanderson@gmail.com
wrote:

Good find. I can amend the pull request with option 2. Then start on
fixing that other bug.

On Tue, Sep 1, 2015, 12:55 PM Nico notifications@github.com wrote:

Have a look at this:
#3757 #3757

So it seems that version2 is correct and the file deletion is a general
IDE bug.
I've added a feature to fall back to a default library config, if no user
config exists.


Reply to this email directly or view it on GitHub
#3717 (comment).

@NicoHood
Copy link
Contributor

NicoHood commented Sep 1, 2015

Sure we can discuss it there any further. the bot is started to build so you can simply download the PR soon.

Fixing the deletion issue would be a different issue which we could open. I just wanted some feedback from someone who knows the building system better than me, I might be wrong.

@ricklon
Copy link
Contributor Author

ricklon commented Sep 1, 2015

Can go.

On Tue, Sep 1, 2015, 1:01 PM Rick Anderson rick.rickanderson@gmail.com
wrote:

Oh. Hang on you took care of it. How about we can with your updated
issue. Good?

On Tue, Sep 1, 2015, 1:00 PM Rick Anderson rick.rickanderson@gmail.com
wrote:

Good find. I can amend the pull request with option 2. Then start on
fixing that other bug.

On Tue, Sep 1, 2015, 12:55 PM Nico notifications@github.com wrote:

Have a look at this:
#3757 #3757

So it seems that version2 is correct and the file deletion is a general
IDE bug.
I've added a feature to fall back to a default library config, if no
user config exists.


Reply to this email directly or view it on GitHub
#3717 (comment).

@NicoHood
Copy link
Contributor

NicoHood commented Sep 2, 2015

What do you mean?

@ricklon
Copy link
Contributor Author

ricklon commented Sep 2, 2015

The new issue. You posted. Does it supersede mine?

On Wed, Sep 2, 2015, 2:26 AM Nico notifications@github.com wrote:

What do you mean?


Reply to this email directly or view it on GitHub
#3717 (comment).

@NicoHood
Copy link
Contributor

NicoHood commented Sep 3, 2015

Yeah it basically fixes the saving bug and adds a new Library.Setting.h option. I just opened a new PR to test this feature and build the IDE with that patch also, so we can compare.

@facchinm
Copy link
Member

facchinm commented Feb 8, 2017

Superseded by arduino/arduino-builder#15

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.

6 participants