Fixed sketch compile with broken symlink

[cmaglie]

Please check if the PR fulfills these requirements

See how to contribute

• The PR has no duplicates (please search among the Pull Requests
  before creating one)
• The PR follows
  our contributing guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)
• UPGRADING.md has been updated with a migration guide (for breaking changes)
• configuration.schema.json updated if new parameters are added.

What kind of change does this PR introduce?

Fix sketch compilation when there is a broken symlink.

What is the current behavior?

• if a file, not part of the sketch, is a broken symlink an error is reported:

$ ls -l 
total 4
-rw-rw-r-- 1 cmaglie cmaglie 33 Jan 11 16:34 Brokensymlink.ino
lrwxrwxrwx 1 cmaglie cmaglie  6 Jan 11 16:51 other -> broken
$ arduino-cli compile -b arduino:avr:uno 
Can't open sketch: stat /home/cmaglie/Workspace/sketchbook-cores-beta/Brokensymlink/other: no such file or directory
$

What is the new behavior?

• if a source file, part of the sketch, is a broken symlink an error is reported:

$ ls -l
total 4
-rw-rw-r-- 1 cmaglie cmaglie 33 Jan 11 16:34 Brokensymlink.ino
lrwxrwxrwx 1 cmaglie cmaglie  6 Jan 11 16:51 other.ino -> broken
$ arduino-cli compile -b arduino:avr:uno 
Can't open sketch: reading sketch files: stat /home/cmaglie/Brokensymlink/other.ino: no such file or directory
$ 

• if a file, not part of the sketch, is a broken symlink no errors are reported:

$ ls -l
total 4
-rw-rw-r-- 1 cmaglie cmaglie 33 Jan 11 16:34 Brokensymlink.ino
lrwxrwxrwx 1 cmaglie cmaglie  6 Jan 11 16:51 other -> broken
$ arduino-cli compile -b arduino:avr:uno 
Sketch uses 444 bytes (1%) of program storage space. Maximum is 32256 bytes.
Global variables use 9 bytes (0%) of dynamic memory, leaving 2039 bytes for local variables. Maximum is 2048 bytes.

Used platform Version Path
arduino:avr   1.8.6   /home/cmaglie/.arduino15/packages/arduino/hardware/avr/1.8.6
$ 

Does this PR introduce a breaking change, and is titled accordingly?

Some sketches may start failing to build, but this is not a breaking change, I see it as a long-standing bug fix

Other information

Fix #1765
Supersedes #1438

[matthijskooijman]

Also see #1438 for an older approach for this problem, which would ignore stat errors from broken symlinks, and only raise an error if a broken symlink was actually opened (e.g. for copying into the build directory). That is less broad than the fix in this PR, which IIUC omits all broken symlinks from the list of sketch files (so e.g. would silently omit a broken myfile.c symlink from compilation) which probably works better in more cases, but could also end up making things fail in unexpected ways silently.

Note that the approach in the other PR ended up not solving the actual problem I was having at the time (ignoring a broken compilation_commands.json symlink), since .json files are now also sketch files that will be copied, so I ended up adding an extra explicit ignore for compilation_commands.json there. For me this problem is now solved differently (using a .clangd file), but maybe there is still some merit in the general approach of that PR. I can imagine it would be useful to use that approach (ignore stat errors from broken symlinks, do not ignore all open errors (which is another improvement by that PR, also for other issues like missing permissions - didn't check master if that is still a problem there) and maybe ignore open errors for broken symlinks but print a warning?

[cmaglie]

That is less broad than the fix in this PR, which IIUC omits all broken symlinks from the list of sketch files (so e.g. would silently omit a broken myfile.c symlink from compilation) which probably works better in more cases, but could also end up making things fail in unexpected ways silently.

This PR is not trying to omit a broken symlink to myfile.c from compilation. BTW I've added a specific test to be sure:

$ ls -l internal/integrationtest/compile_4/testdata/ValidSketchWithNonInoBrokenSketchFileSymlink
total 4
-rw-rw-r-- 1 cmaglie cmaglie 31 Jan 12 12:41 ValidSketchWithNonInoBrokenSketchFileSymlink.ino
lrwxrwxrwx 1 cmaglie cmaglie  6 Jan 12 12:41 other_file.c -> broken
$ arduino-cli compile -b arduino:avr:uno internal/integrationtest/compile_4/testdata/ValidSketchWithNonInoBrokenSketchFileSymlink
Can't open sketch: reading sketch files: stat /home/cmaglie/Workspace/arduino-cli/internal/integrationtest/compile_4/testdata/ValidSketchWithNonInoBrokenSketchFileSymlink/other_file.c: no such file or directory
$

Besides that, looking at the integration tests, it seems quite hard to get this thing right (luckily we have a ton of tests...).

The tricky part is that when you recurse subdirs you have to decide if you should follow a symlink or not, but you don't know in advance if a broken symlink points to a directory or a file, for example:

MySketch/Sketch.ino
MySketch/Other.ino -> broken

in this case, the link points to a sketch file, and it should error on compile. BTW when we do the ReadDirRecursive we don't know if the link points to a file or a dir, so we cannot just ignore it. I've approached this with a bit of heuristic, if the name of the symlink has one of the accepted file extensions and it is a broken link, it must be accepted as part of the sketch file list. BTW thinking on this a bit more the symlink existence check should be moved inside the ReadDirRecursive, so it will not error if the symlink is broken (assuming it is a directory), but just pass the symlink path in the result list (as it was a file) for further processing. I'll make this change and see how it goes.

Another tricky issue is that when we ignore errors on broken symlinks, we may be affected by infinite symlink loop recursion because previously we stopped at the first "path too long" error, but now we don't! (well it's not exactly infinite recursion, but an exponentially long recursion). So I had to implement a better loop recursion detection: arduino/go-paths-helper#29

Finally, there is another test case that is failing now:

--- FAIL: TestCompile (30.26s)
    --- PASS: TestCompile/WithoutFqbn (0.01s)
    --- PASS: TestCompile/ErrorMessage (0.05s)
    --- PASS: TestCompile/WithSimpleSketch (0.88s)
    --- PASS: TestCompile/WithCachePurgeNeeded (0.60s)
    --- PASS: TestCompile/OutputFlagDefaultPath (0.30s)
    --- FAIL: TestCompile/WithSketchWithSymlinkSelfloop (0.31s)

this one I hadn't the time to check yet...

[matthijskooijman]

This PR is not trying to omit a broken symlink to myfile.c from compilation. BTW I've added a specific test to be sure:

Ah, I see. I only looked quickly before and missed the "Fail if a sketch file is a broken symlink" which changes the check to indeed not omit broken symlinks with supported extensions.

if the name of the symlink has one of the accepted file extensions and it is a broken link, it must be accepted as part of the sketch file list.

That sound reasonable, yes.

Another tricky issue is that when we ignore errors on broken symlinks, we may be affected by infinite symlink loop recursion because previously we stopped at the first "path too long" error, but now we don't!

Hm, I do not see how this follows from the "omit some broken symlinks from the file list" change in this PR, but maybe you mean that it results from some earlier change in go-paths-helper? Though I can't quite see which one.

Also, did you happen to have a look at arduino/go-paths-helper#13? It fixes some (broken) symlink related stuff (and also adds an Lstat() method just like you did recently ;-p). I haven't had a closer look now, though, I'm out of time :-)

[cmaglie]

Another tricky issue is that when we ignore errors on broken symlinks, we may be affected by infinite symlink loop recursion because previously we stopped at the first "path too long" error, but now we don't!

Hm, I do not see how this follows from the "omit some broken symlinks from the file list" change in this PR, but maybe you mean that it results from some earlier change in go-paths-helper? Though I can't quite see which one.

Yes, it's very tricky, it has surprised me too, consider this folder:

dir1 -> ..
dir2 -> ..

basically both points to the current dir, so you can write any path like dir1/dri2/dir1/dir1/dir2/dir1/... with any sequence of dir1 and dir2. Previously this stopped at the first long path error path too long: dir1/dir1/dir1/dir1/dir1/..., it wasn't ideal, but it eventually terminated quickly after a reasonable number of iterations. But if we skip "invalid" symlinks, we are doing a backtracking on all possible paths:

dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/..../dir1/dir1/dir1/dir1/dir1/dir1/ -> path too long
dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/..../dir1/dir1/dir1/dir1/dir1/dir2/ -> path too long
dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/..../dir1/dir1/dir1/dir1/dir2/dir1/ -> path too long
dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/..../dir1/dir1/dir1/dir1/dir2/dir1/ -> path too long
dir1/dir1/dir1/dir1/dir1/dir1/dir1/dir1/..../dir1/dir1/dir1/dir2/dir1/dir1/ -> path too long
.....

we may catch the exact path too long error and quit the recursion only on that but this may be complicated to do cross-OS, so I opted to make a better loop detection.

Also, did you happen to have a look at arduino/go-paths-helper#13? It fixes some (broken) symlink related stuff (and also adds an Lstat() method just like you did recently ;-p). I haven't had a closer look now, though, I'm out of time :-)

It's going to be fully implemented with the other incoming PRs on go-paths-helper... :-)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8115da1) 68.82% compared to head (e9dc764) 68.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2497      +/-   ##
==========================================
+ Coverage   68.82%   68.92%   +0.10%     
==========================================
  Files         204      204              
  Lines       20457    20451       -6     
==========================================
+ Hits        14079    14096      +17     
+ Misses       5221     5206      -15     
+ Partials     1157     1149       -8     

Flag	Coverage Δ
unit	68.92% <100.00%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cmaglie]

The version of go-paths imported in go.mod is the one containing this PR: arduino/go-paths-helper#30
I'll merge and update the references once the review is done.

[alessio-perugini]

Remember to replace the go-paths-helper with the tagged version.