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

Subfolders can no longer be included #5186

Closed
Zeron opened this Issue Jul 30, 2016 · 15 comments

Comments

8 participants
@Zeron
Copy link

Zeron commented Jul 30, 2016

up to version 1.6.9; if you had .h files in a subfolder of a sketch, you could #include "subfolder/file.h" and it would compile.

In 1.6.10 this function has been removed.

@bperrybap

This comment has been minimized.

Copy link

bperrybap commented Jul 30, 2016

This is a serious issue that hast the potential to break many projects & sketches.
IMO, this is serious enough to kill/recall the 1.6.10 release.
The issue is that the 1.6.10 IDE is not copying in the entire sketch directory. The IDE/builder must copy everything (all files, all subdirectories & files recursively) down under tmp if it is not going to build in place and it does this on older IDEs.

@luc-github

This comment has been minimized.

Copy link

luc-github commented Jul 30, 2016

it is not same as #5176 ?

@bperrybap

This comment has been minimized.

Copy link

bperrybap commented Jul 30, 2016

Yes, and it is a serious issue. I commented further here:
arduino/arduino-builder#148

@bperrybap

This comment has been minimized.

Copy link

bperrybap commented Jul 30, 2016

Couldn't this be resolved by simply adding the original sketch directory to the include path?
That way any sketch that attempts to include a file from a sub directory using quotes as in the example in the original post gcc would find it since gcc will look on the search path if it can't find it in/under the local directory.
That way there is no additional copying that has to be done but includes in sub directories can sill be found.

@Chris--A

This comment has been minimized.

Copy link
Contributor

Chris--A commented Jul 31, 2016

@bperrybap

I think this is a good idea. Although, does the IDE share all settings globally between IDE instances.
As in changing boards effects all instances, so I'm guessing modifying the command line options is also going to be global.

@bperrybap

This comment has been minimized.

Copy link

bperrybap commented Jul 31, 2016

If the IDE is not going to copy the full sketch directory, then the IDE should tell the compiler to look in the original sketch directory for includes by adding it to the include path.
It is a simple fix that preserves backward compatibility and more than likely was an oversight by whoever changed the code to no longer copy the full sketch directory down into the tmp working area.

@Chris--A
IDE instances doesn't really come into play. The include path is already unique to each build. This solution is just adding one more include directory to the already unique include path that is created by each IDE build window when building a sketch.
The include path is a compiler option used to tell the compiler where to look for indludes. If the IDE were to add the original sketch directory to the include path then that tells the compiler to also look for includes in the original sketch directory as well as the tmp working directory where the sketch code is being compiled.
The IDE already tells the compiler where to look for the core and library header files and by default the compiler will look in the directory being compiled. The change in 1.6.10 is that the IDE no longer copies the full sketch directory to the temporary build area (which is wrong in my opinion). But if the IDE were to tell the compiler to also look in the original sketch directory for includes, then at least the issue here (which is being reported by multiple people) would be resolved, as the compiler would be able to locate the header files in the sub directories of the original sketch directory.

From a broader perspective,
I've never understood why the IDE needed to make a copy of the original sketch directory.
To me, it always seemed like unnecessary work and load on the host to make a copy of the sketch directory rather than simply use it in place but place the working files and objects down in the tmp directory.
It does not make copies of other files & directories needed for the build like library directories, and the core directory.
So why do it for the sketch?

@cmaglie

This comment has been minimized.

Copy link
Member

cmaglie commented Aug 1, 2016

It does not make copies of other files & directories needed for the build like library directories, and the core directory. So why do it for the sketch?

becuase you can have unsaved changes.

cmaglie added a commit to arduino/arduino-builder that referenced this issue Aug 1, 2016

Added test for arduino/Arduino#5186
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

matthijskooijman commented Aug 1, 2016

The change to limit recursive compilation to the src/ folder was intentionally backward-incompatible, since the original addition of recursive compilation was not considered well enough and caused hard-to-fix issues. I believe I intentionally also limited file recursive copying (and thus including) to the src folder for consistency, but now I'm not so sure this was the best approach.

I've never understood why the IDE needed to make a copy of the original sketch directory.

This is because the .ino files need to be preprocessed to:

  • Concatenate all .ino files
  • Add #include <Arduino.h> in an appropriate place
  • Add forward declarations for functions.

Since writing such a temporary file to the original sketch directory isn't proper (and sometimes impossible when it is read-only), it must be written to some temporary folder. Since the .ino files can include other files in the sketch directory, these have to be copied too.

An alternative approach to copying these other files would be adding the original sketch directory to the include path (possibly using -iquote instead of -I to maintain the distinction between "" and <> includes) seems like a good idea to prevent copying all other files into the build directory. I remember considering this option and rejecting it, though I can't quite recall the problem with it.

A related issue is that when the IDE has unsaved files, another copy of the sketch is made, which gets copies of all files, and inclues the unsaved changes too. For this case, it seems elegant to create a directory with only the files with unsaved changes and use the include path to also access the unchanged files, but this runs into an issue where an unchanged file includes a changed file (since -iquote includes a directory on the search path after the current directory, I could not find any way to insert it before).

The latter issue does not apply when putting just the pre-processed .ino files in the build directory, since that preprocessed file is never included itself. So, are there other corner cases not covered by using the include path (preferably using -iquote)?

cmaglie added a commit to arduino/arduino-builder that referenced this issue Aug 1, 2016

Added test for arduino/Arduino#5186
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>

cmaglie added a commit to arduino/arduino-builder that referenced this issue Aug 1, 2016

Copy the whole sketch into tmp folder
See arduino/Arduino#5186

Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>

cmaglie added a commit to arduino/arduino-builder that referenced this issue Aug 1, 2016

Copy the whole sketch into tmp folder
See arduino/Arduino#5186
Partially revert #148

Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>

cmaglie added a commit to arduino/arduino-builder that referenced this issue Aug 1, 2016

Sketch is fully copied in tmp when building
Should fix: arduino/Arduino#5186

Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

matthijskooijman commented Aug 1, 2016

I'll also look more closely to the rationale for using a src/ folder in the first place, since I read some comment suggesting this is not entirely clear (the comment is about libraries, but the below applies to libraries as well as sketches).

If you want to use subfolders, an obvious approach is to simply recursively list all files in the sketch or library directory, and compile them. This has one big problem: All files will be used as source files, with a filter on file extension being the only thing that prevents a file from being included in the build. In effect, the entire directory is (recursively) marked as containing source files. Now, if different things are to be included (such as examples, documentation, etc.), this is effectively impossible, since all subdirectories will be scanned for source files. This does not directly cause a problem if these other things use different filename extensions, but once your example includes a .c file, things will break.

So, essentially you need a way to let the build environment distinguish between "source files" and "other things". Solutions will probably either:

  • Use an opt-in list of subdirectories that contain source files.
  • Use an opt-out list of directories that do not contain source files.

Both of these can again be implemented in two ways:

  • Using an implicit (defined by the tool) opt-in or opt-out list. This is what is now used for sketches and recursive libraries, with the opt-in list containing only the src/ folder.
  • Using an explicit (defined by the code author) opt-in or opt-out list. This would be the case if there would have been a list of recursive folders defined in e.g. library.properties.

Together, these form four options (implicit opt-in, implicit opt-out, explicit opt-in, explicit opt-out). There might of course be other options, but for now I'll limit the discussion to these four.

Using the explicit options give the code author the most flexibility about how to arrange his source files. However, it also has some downsides:

  • Different libraries and sketches will use different layouts, making it a bit harder to find out how things will be compiled.
  • Sketches do not currently have any metadata, making this only applicable to libraries currently.
  • Third party-tools would become more complicated, as they are forced to read the metadata files to figure out what directories to compile.

For these reasons, I think the implicit option is better in an environment like Arduino. The implicit opt-in option seems the most flexible to me: Putting all source files in a src/ subdirectory seems acceptable, whereas the implicit opt-out method requires the build tool to define a list of "other" things and force authors to use those (.e.g. examples, documentation, other seems acceptable, but then some author prefers docs or doc instead).

Hence, this is why I think using the implicit opt-in method, i.e. only recursively compiling files inside the src/ folder is the best approach here.

Note that this rationale does apply to compilation of source files, but does not really apply to inclusion of header files, which is really the subject of this issue. The distinction here is that header files are explicitely named through #include directives (typically with relative paths that name the subdirectory to look in, if any), whereas compilation requires the building tool to enumerate the source files to use.

When we decided to limit recursive compilation to the src/ folder (which, given the above, I believe is a sound plan, though the changelog should mention this way more explicitly), I also thought it would be sane to also limit inclusion to the same folder, to prevent user surprise (why can I include a file from here, but not put a source file there?). Judging from the responses, this actually results in more user surprise (why can't I include this file using this explicit #include statement), so I think revisiting this part of the change makes sense.

cmaglie added a commit to arduino/arduino-builder that referenced this issue Aug 1, 2016

Sketch is fully copied in tmp when building
Should fix: arduino/Arduino#5186

Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>

matthijskooijman added a commit to matthijskooijman/arduino-builder that referenced this issue Aug 1, 2016

Added test for arduino/Arduino#5186
Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>

matthijskooijman added a commit to matthijskooijman/arduino-builder that referenced this issue Aug 1, 2016

Sketch is fully copied in tmp when building
Should fix: arduino/Arduino#5186

Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>

matthijskooijman added a commit to matthijskooijman/arduino-builder that referenced this issue Aug 1, 2016

Test sketch that includes from subfolder
There was already a test for including from inside the `src/` subfolder,
but including from other subdirectories should also be supported (even
when source files inside those directories are not compiled). This adds
a test for this. See also arduino/Arduino#5186.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

cmaglie added a commit to cmaglie/arduino-builder that referenced this issue Aug 1, 2016

Test sketch that includes from subfolder
There was already a test for including from inside the `src/` subfolder,
but including from other subdirectories should also be supported (even
when source files inside those directories are not compiled). This adds
a test for this. See also arduino/Arduino#5186.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
@bperrybap

This comment has been minimized.

Copy link

bperrybap commented Aug 1, 2016

@matthijskooijman
I don't use the IDE editor so I failed to consider and appreciate some of the complexities when using it. Thank your for reminding of that.

The focus seemed to be on what directories are used to recursively look in for compiling things or to automatically include as part of the build. I don't really have any strong feelings on this and restricting the directories used for automatically compiled sources seems reasonable, even if it is to single directory named 'src'.

However, I have seen some interesting embedded projects on sites like make/makerzine/instructables, that have sub directories in their sketch directory for libraries and utility code.
It makes for a nice all in one arduino project that has everything needed to build the project in a single sketch directory.
Keep in mind that those types of projects will break.

However, what seemed unreasonable was the restriction of not being able to use sub directories for organizing include files.
It sounds like you may be having a change of mind of this as well.

In C++ since you can have function definition (code) in the header file.
In fact if using a templated class, the code must be in the header file.
And if using many header files like this, having the ability to organize header files in sub directories can be a useful thing.

Restoring the ability of a sketch to use #include "subdir/file" would restore a consistency with library code.

Can't the include issue be solved by simply adding the original sketch directory to the include path?

matthijskooijman added a commit to cmaglie/arduino-builder that referenced this issue Aug 1, 2016

Do not limit includes to the `src` sketch subdirectory
Commit 405a24a (Limit recursive sketch compilation to the `src`
directory) modified the sketch loading to only recursively load files
from the `src` subdirectory of a sketch, in order to only compile source
files in that subdirectory. As a side effect, this also prevents source
and header files in other subdirectories from being copied into the
build directory, and hence from being included. This side effect was not
entirely unanticipated, but its implications were underestimated.

This commit undoes these side effects: source and header files are again
loaded (and thus copied) recursively in all subdirectories of a sketch.
Compilation is still limited to the src subdirectory (there already was
code to achieve this). This commit also modifies include detection to be
limited to the src subdirectory (which was previously implicit, since it
can only look at files that were copied into the build directory).

With this commit, the the meaning of "loading" a file becomes a bit
fuzzy in the code, but this is intended to be fixed in a future
refactor.

A test will be added in the next commit.

This fixes arduino/Arduino#5186.

Signed-off-by: Cristian Maglie <c.maglie@arduino.cc>
Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>

matthijskooijman added a commit to cmaglie/arduino-builder that referenced this issue Aug 1, 2016

Test sketch that includes from subfolder
There was already a test for including from inside the `src/` subfolder,
but including from other subdirectories should also be supported (even
when source files inside those directories are not compiled). This adds
a test for this. See also arduino/Arduino#5186.

Signed-off-by: Matthijs Kooijman <matthijs@stdin.nl>
@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

matthijskooijman commented Aug 1, 2016

Restoring the ability of a sketch to use #include "subdir/file" would restore a consistency with library code.

Note that this is still possible: Just put the subdirectories in the src subdirectory. That's of course not entirely convenient, and also not entirely expected, so it's not a real solution.

The current proposal is to re-instate the copying of source and header files in subdirectories as it was before, but still limit the compilation of source files to the src/ folder. This should resolve this issue, while not re-introducing the issues caused by recursive compilation of all sketch subdirectories. This seems to be the least risky way to fix this on the short term. On the (slightly) longer term, we'll see if using the include path works (which also means that only the preprocessed .ino files need to be copied to the build path, nothing else), but that needs a bit more thought and testing.

@matthijskooijman

This comment has been minimized.

Copy link
Collaborator

matthijskooijman commented Aug 1, 2016

Btw, see arduino/arduino-builder#172 for the implementation of that proposal.

@bperrybap

This comment has been minimized.

Copy link

bperrybap commented Aug 1, 2016

Seems like a very sensible approach.
For the longer term, I'd suggest keeping it simple since things like opt in/out lists, however that is implemented, particularly if added to .properties adds complexity and will have impacts on 3rd party tools that build sketches like plugins for other IDEs like eclipse, MSFT IDE tools, tools that use makefiles etc...

Any changes that the IDE makes relating to how sketches are built can have ripple effects in all those other tools causing their toolsets to break or put lots of work on those tool maintainers.

@cmaglie

This comment has been minimized.

Copy link
Member

cmaglie commented Aug 3, 2016

The next hourly-build should have fixed this issue.

@cmaglie cmaglie added this to the Release 1.6.11 milestone Aug 3, 2016

@cmaglie cmaglie changed the title Subfolders no longer compiled Subfolders can no longer be included Aug 5, 2016

@i-make-robots

This comment has been minimized.

Copy link

i-make-robots commented Aug 5, 2016

yes, sorry, #5211 is duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.