All committers have signed the CLA.

Hi@d-a-v, thank for your submission.

We already have an implementation of the "walk-folllowing-symlinks" here:
https://github.com/arduino/arduino-cli/blob/master/legacy/builder/gohasissues/go_has_issues.go#L39

BTW, I was wondering why the golangWalk function doesn't follow symlinks and, looking at the golang bugtracker, I found that is because it may lead to infinite loops:
golang/go#4759

This seems to be not golang-related but also other language that allows following symlinks suffer the same problem, for example in python we have:

https://docs.python.org/3/library/os.html#os.walk

that warns:

Note Be aware that setting followlinks to True can lead to infinite recursion if a link points to a parent directory of itself. walk() does not keep track of the directories it visited already.

Since it seems that there isn't a saner/simple way to avoid loop we want to propose the following solution:

• Follow symlink only once onsketchPath
• Once sketchPath is resolved, use plain golangWalk from there

this should cover the use cases in#424 (and I think 99.99% of the overall use cases).
What do you think?

Thanks,

I'm happy to close this PR when#424 is fixed by another mean.

(edited)

About the walking issue with symbolic links, we can add a decreasing counter in the recursive walk function (like the factorial recursive function we learn at school), and return an error (likeToo many levels of symbolic linksToo many levels of sub-directories) when it reaches 0.
That way there is no need for specific code anywhere, and we can have symbolic links for sketch dir, sketch files,utility/ and files underutility/.

I've also read the golang issues in their repository and I just don't understand why or how they would rewrite (or avoid) the unix features their own way. A symbolic link loop is on user responsibility Golang sys lib's responsibility is only to not fail (heap here), not to remove any OS functionality IMHO.

$ ls -l tlrwxrwxrwx 1 user group 1 Oct  9 16:42 t -> t$ cd tbash: cd: t: Too many levels of symbolic links

Hi@d-a-v ,
May we ask you to add a couple of test for this PR in order to outline yourwalk function behavior?

We want to use your PR as a starting point to introduce a more robustwalk function, following symlinks only once onsketchPath (as@cmaglie mentioned), in order to fix rapidly the bugs and avoid symlink related issues.

As a second step, we will make thewalk generic, adding a symlink loop check mechanism likehttps://gitlab.openebs.ci/openebs/maya/blob/5bfd0e581446d188292000626cc4dd1150e4d368/vendor/golang.org/x/tools/internal/gopathwalk/walk.go#L172

Thanks for your help!

I'd be glad to.

First I need to know whether you agree with the idea that a path depth shall not be more than a given constant (like 40, similar to what's done inlinux kernel"Linux imposes a limit of at most 40 symlinks in any one path lookup") ?

Shall I make this proposal overthis file (instead of mine) ? Is it intended to stay where it is ?

My 2c:

• 40 is good because it's what the Linux kernel uses
• let's not add anything to thelegacy package, I'd keep the function where it is now insketch.go, we can move it later if other modules/packages need to walk the filesystem

@d-a-v go ahead using40! Please keep thewalk insketch.go as@masci said. Thanks!

Testing the error was missing in the user walk function,
Can you please validate the print I added there ?

A reference to this issue is added in a comment about the discussion on40.
Usually one doesn't write raw constants.
Please direct me to the golang way/place for doing better if the comment is not sufficient.

How to test this code (with linux):
In some sketch directory, create a symlink loop, detectable by OS:

$ mkdir utility$ cd utility$ ls -s x x$ arduino-builder ...[...]error: stat /home/gauchard/dev/proj/arduino/y/utility/x: too many levels of symbolic links[...]

(OS error printed)

Or not detectable by OS:

$ mkdir utility$ cd utility$ ln -s .. x$ arduino-builder ...[...]error: Filesystem bottom is too deep (directory recursion or filesystem really deep): /path/to/mysketchdir/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/x/utility/xgithub.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive        /local/users/gauchard/arduino/gh-arduino-cli/arduino/builder/sketch.go:77github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive        /local/users/gauchard/arduino/gh-arduino-cli/arduino/builder/sketch.go:83github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive        /local/users/gauchard/arduino/gh-arduino-cli/arduino/builder/sketch.go:83github.com/arduino/arduino-cli/arduino/builder.simpleLocalWalkRecursive[...]

(depth detection error)

Maybe we should differentiate two cases here:

1. a symlink that has a cycle to himself -> easy
2. a symlink that points to a directory that contains another symlink that points back to (any parent of...) the first one -> hard

Here the tricky case (2):

:~/tmp$ mkdir A:~/tmp$ mkdir B:~/tmp$ cd A/:~/tmp/A$ ln -s ../B/:~/tmp/A$ cd ..:~/tmp$ cd B:~/tmp/B$ ln -s ../A/:~/tmp/B$ cd ..:~/tmp$ find -L .../A./A/Bfind: File system loop detected; './A/B/A' is part of the same file system loop as './A'../B./B/Afind: File system loop detected; './B/A/B' is part of the same file system loop as './B'.

it seems thatls has no way to list files following symlinks recursively (it follows symlinks only if they are listed on the command line as arguments).
find seems to have a good algorithm to detect loops, but it will require more work.

These two cases are handled by this PR.
Maybe I wasn't clear enough: the examples I show above are these two cases.
The tricky case is handled by the depth counter. The second quoted part of the examples is scrollable to the right and you can count the number of subdir levels: it's around 40.
Did I misunderstand something ?

Aboutfind -L's algorithm, I believe it retains every inode and stops with a symlink-loop error when the same inode is found again (golang FileInfo - where inode could be found I guess - is OS-dependent). We could do that but given the rarity of the issue, I believe a depth counter like implemented here is enough and easier to handle.

Hi@d-a-v! I left a comment on your code.
Can you please add a couple of tests to check themaxDepth control and the symlink loop?
You can see an examplehere on how to do it. The test code leverages sample sketches in thetestdata folder, you can create a sketch that contains the symlinks you need for the testing

@rsora I think I addressed your review.

@d-a-v can you try manually runninggo fmt arduino/builder/sketch.go?

@masci
done, thanks

I don't understand CI messages though.

What about a script to run before pushing changes that would run CI tests locally ?

@d-a-v our contributing guidelines already explain how to run tests locally before pushing code:https://github.com/arduino/arduino-cli/blob/master/CONTRIBUTING.md#running-the-tests

The style check,task check isn't mentioned though, I'll add it to the guide.

Hi@d-a-v , left a comment regarding the error management.
Thanks for your time!

fixes addressed, CI passed, thanks for your feedback.