Fix compile command stuck in a loop when including multiple custom libraries

[silvanocerza]

Please check if the PR fulfills these requirements

• 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)

• What kind of change does this PR introduce?

Fixes a bug.

• What is the current behavior?

Including multiple custom libraries with identical name but different sources makes the compile to remain stuck in a loop.

• What is the new behavior?

compile command doesn't stuck in a loop anymore when including multiple custom libraries.

• Does this PR introduce a breaking change?

Nope.

• Other information:

Fixes #973.

---

See how to contribute

[per1234]

I haven't been able to understand how the code works yet, but I am able to reproduce the bug before this change, but not after.

I thought I'd at least make this review while I have the opportunity. I'll see if I can gain an understanding of the code with some more studying and experimentation.

[per1234]

Suggested change

	// ./Lib1/src/lib1.h and ./Lib2/src/lib2.h
	// ./libraries1/Lib/Lib1.h and ./libraries2/Lib/Lib2.h

The previous example is confusing because typically when you see a src folder it implies an Arduino library with the "recursive" layout, like this:

Lib1
|_ library.properties
|_ src
   |_ lib1.h

The correct usage of the --libraries flag is to pass a path to a directory containing libraries (which would be the parent of the Lib1 folder with the structure in the above diagram, not the path of a library (which would be the Lib1 folder in that diagram. Since the original example implies the latter, which would not trigger the bug since Lib1 != Lib2, it might be confusing to future readers.

After downloading the TestProject.zip attachment from #973, I now understand that the src folders actually contain legacy libraries, but the future reader of this comment should not need that context.

[silvanocerza]

So there are two ways to structure a library code? ./Lib1/src/lib1.h is legacy and ./libraries1/Lib/lib1.h is not? If there are multiple ways to do it I'll write a test for both, I tested just this one because it was the exact same case it has been reported.

[per1234]

So there are two ways to structure a library code?

There are three!

Here is an example libraries folder structure:

{directories.user}
|_ libraries
    |_ Lib1
    |   |_Lib1.h
    |_ Lib2
    |   |_ library.properties
    |   |_ Lib2.h
    |_ Lib3
        |_ library.properties
        |_ src
            |_ Lib3.h

Lib1 has what is referred to as the "flat" library layout, and also as "legacy" in the Arduino CLI source code, or the "1.0" format in the Arduino Library Specification.

Lib2 also has the "flat" library layout, due to the .h file being in the library root folder, but it's not "legacy" due to having a library.properties file.

Due to having a library.properties file and the .h under the src subfolder, Lib3 has what is referred to as the "recursive" library layout in the Arduino CLI source code, or the "1.5" format in the Arduino Library Specification.

There's an issue about improving the documenting for this here: #1013 and I attempted to explain it clearly here: Arduino-CI/arduino_ci#168 (comment)

---

So the presence of a src folder in the example implies the "recursive" layout, but in reality it's only a legacy library that happens to have a root folder named src. This was quite confusing to me at first while I was doing the review, so I thought that others would be likely to have the same confusion later. There's nothing special about this src folder name in this particular case (even though in the case of a recursive library, the src subfolder does have special meaning). So the example is much easier to understand if a folder name is used that doesn't have special meaning in any context.

---

I tested just this one because it was the exact same case it has been reported.

Same here. I didn't test with recursive layout libraries. The situation might be a bit different between legacy and non-legacy libraries. The reason is that legacy libraries don't have a library.properties file, so the name value is derived from the folder name. Non-legacy libraries (whether flat or recusive layout) have a name value defined in library.properties, which could be different from the folder name if manually installed.

[silvanocerza]

Interesting, the TestProject.zip was a malformed recursive/1.5 format and it was being interpreted as flat/legacy/1.0.

So technically the CLI isn't broken but the libraries in the example are, do we want to go forward with this fix with this newfound knowledge?
Feels like to me that by doing this we're handling something that's not respecting the Arduino Library Specification and creating yet another possible structure.

Paging @cmaglie and @ubidefeo too, I think this guarantees some more discussion.

[per1234]

TestProject.zip was a malformed recursive/1.5 format and it was being interpreted as flat/legacy/1.0.

I guess that's a matter of opinion. Here's the tree of TestProject.zip:

TestProject
├── libraries
│   ├── Lib1
│   │   └── src
│   │       └── lib1.h
│   └── Lib2
│       └── src
│           └── lib2.h
└── TestProject.ino

If you consider TestProject/libraries to the the Arduino libraries folder, then this does indeed look like TestProject/libraries/Lib1 and TestProject/libraries/Lib2 are malformed recursive libraries (because they are missing TestProject/libraries/Lib1/library.properties and TestProject/libraries/Lib2/library.properties.

However, the arduino-cli compile --libraries libraries/Lib1,libraries/Lib command used by the reporting party caused TestProject/libraries/Lib1 and TestProject/libraries/Lib2 to be custom libraries folders, each of which contains a legacy format library named src. That is odd, but it is compliant with the specification.

I do agree that using arduino-cli compile --libraries to point to individual library folders and having it just happen to work because those libraries are recursive layout is not how --libraries was intended to be used. However, this bug could also occur in a completely legitimate use case, so we shouldn't disregard it just because the user happened to be doing something a bit unusual when they encountered it. You already did the work to fix it, so I don't see any harm with continuing.

But I think we have conclusive proof that using this strange use case as the demonstration of the bug and test case is going to cause confusion, so the folder names of the demo/test library folders should surely be changed from src.

[silvanocerza]

Which tests cases do you think we should test? I'm talking about cases that broke before the fix and now don't.

For example I would expect this works now and then, since the "calculated" names would be different.

.
├── Lib1
│   └── lib1.h
└── Lib2
    └── lib2.h

This one though I'd expect to break previously and work now since the name Lib would be identical:

.
├── libraries1
│   └── Lib
│       └── lib1.h
└── libraries2
    └── Lib
        └── lib2.h

Any other case I would expect to work correctly before and after since the library.properties is used.

[per1234]

Which tests cases do you think we should test?

I'm happy with the single test case that's already in place. I only want the folder names changed.

I would expect this works now and then, since the "calculated" names would be different.

Yep.

This one though I'd expect to break previously and work now since the name Lib would be identical:

Yes.

Any other case I would expect to work correctly before and after since the library.properties is used.

No, I just tried it and even with recursive layout libraries with different name values in library.properties, if the folder names are the same you would get the endless loop before your fix (and no loop after your fix):

.
├── libraries1
│   └── Lib
│       └── library.properties
│       └── src
│           └── lib1.h
└── libraries2
    └── Lib
        └── library.properties
        └── src
            └── lib2.h

[silvanocerza]

Ok, then I'll change the folders name and that's it.

[ubidefeo]

After reading your comments thread I'd say let's merge it in :)

[per1234]

Thanks Silvano. The comment and tests are much more clear to me now with the new library folder names..