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

"Library Name Priority" feature impacts some platform bundled libraries #1292

Closed
per1234 opened this issue May 14, 2021 · 8 comments
Closed
Assignees
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project

Comments

@per1234
Copy link
Contributor

per1234 commented May 14, 2021

#1276 added a nice new "Library Name Priority" feature where the library dependency system uses the similarity of the library name value and the filename of an #include directive as one of the factors in deciding which library to use when multiple matching matching libraries are installed. This was released with Arduino CLI 0.18.2 (and the Arduino IDE 1.8.14 and 2.0.0-beta.6 releases which use it).

Unfortunately, this change resulted in some platform bundled libraries which had previously been correctly correctly chosen no longer being given priority over the general purpose libraries from Arduino Library Manager.

An example:

$ arduino-cli version
arduino-cli.exe alpha Version: 0.18.2 Commit: 7b5a22a4 Date: 2021-05-10T14:30:17Z
$ arduino-cli lib install SD
$ arduino-cli core update-index --additional-urls https://raw.githubusercontent.com/espressif/arduino-esp32/gh-pages/package_esp32_index.json
$ arduino-cli core install esp32:esp32 --additional-urls https://raw.githubusercontent.com/espressif/arduino-esp32/gh-pages/package_esp32_index.json
$ mkdir /tmp/FooSketch
$ printf "#include <SD.h>\nvoid setup(){}\nvoid loop(){}" > /tmp/FooSketch/FooSketch.ino
$ arduino-cli compile --fqbn esp32:esp32:esp32 /tmp/FooSketch
In file included from C:\Users\per\Documents\Arduino\libraries\SD\src/utility/Sd2Card.h:26:0,
                 from C:\Users\per\Documents\Arduino\libraries\SD\src/utility/SdFat.h:29,
                 from C:\Users\per\Documents\Arduino\libraries\SD\src/SD.h:20,
                 from C:\Users\per\AppData\Local\Temp\FooSketch\FooSketch.ino:1:
C:\Users\per\Documents\Arduino\libraries\SD\src/utility/Sd2PinMap.h:524:2: error: #error Architecture or board not supported.
 #error Architecture or board not supported.
  ^
Multiple libraries were found for "SD.h"
 Used: C:\Users\per\Documents\Arduino\libraries\SD
 Not used: C:\Users\per\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.6\libraries\SD

Error during build: exit status 1

You can see that the esp32:esp32 platform's bundled library, which is written specifically for ESP32 architecture, was not chosen, contrary to the expected behavior. The reason can be seen in the library's library.properties metadata file here:
https://github.com/espressif/arduino-esp32/blob/1.0.6/libraries/SD/library.properties#L1

name=SD(esp32)

While the general purpose library's metadata is a perfect match for the SD.h file of the #include directive:
https://github.com/arduino-libraries/SD/blob/1.2.4/library.properties#L1

name=SD

The reason the names of some platform bundled libraries have these suffixes is to workaround a bug which caused libraries to be considered perpetually updatable by the the Arduino IDE's Library Manager when they had the same name as the platform bundled library:
arduino/Arduino#4189

This bug was fixed in Arduino IDE 1.8.6, ~3 years ago, but there was never a reason for the platform authors to change them back to their proper values.

Once this unexpected state of affairs was revealed, it was decided that the best approach would be to revert the change (#1290) and make a 0.18.3 release of Arduino CLI (accompanied by Arduino IDE 1.8.15 and 2.0.0-beta.7) in order to allow time for the maintainers of affected projects to prepare. The "Library Name Priority" feature will be added back in the next release of Arduino CLI and the IDEs. In order to ensure platform bundled libraries get the correct priority, the only necessary change is simply to change the library.properties name values to match the primary header file after any spaces have been replaced by _:

--- a/library.properties
+++ b/library.properties
@@ -1,4 +1,4 @@
-name=SD(esp32)
+name=SD
 version=1.0.5
 author=Arduino, SparkFun
 maintainer=Arduino <info@arduino.cc>

Standalone libraries should not be affected by this feature because the Library Manager installation folder name is determined by the name value, and thus their dependency resolution priority was already dependent on the name.

@d-a-v
Copy link
Contributor

d-a-v commented May 30, 2021

For those who are not fully familiar with the history of the library selection priority management,
can you please summarize the consequences of the proposed changes ?

Platform release without fix (past, present) Platform release with fix (future)
Arduino IDE < 1.8.15 works ?
Arduino IDE v1.8.15 works works?
Arduino IDE > 1.8.15 ? works

Where

  • works means that the good library will be selected (ex: AVR's SD will not be selected over ESP32's internal SD)
  • a typical example of proposed fix on platform embedded libraries' library.properties file is (that is what your PRs do above):
-name=SD(esp32)
+name=SD

Please feel free to reformulate my question if I did not get it right :)

@per1234
Copy link
Contributor Author

per1234 commented May 30, 2021

Hi @d-a-v. I believe this is the full summary:

Platform release without fix Platform release with fix
Arduino IDE <1.6.2 (introduction of Library Manager) works works
Arduino IDE 1.6.2 - 1.8.5 works Library Manager can show lib perpetually updatable under specific circumstances
Arduino IDE 1.8.6 - 1.8.13 works works
Arduino IDE 1.8.14 doesn't work works
Arduino IDE 1.8.15 works works
Arduino IDE > 1.8.15 doesn't work works

Please let me know if anything is unclear.

@d-a-v
Copy link
Contributor

d-a-v commented May 30, 2021

Then for previous (and current) platform release up to now,

  • A note has to be added stating that supported Arduino IDE versions are any up to 1.8.15 except 1.8.14,
  • Current and future maintainers of such platform have to be aware that users on older platform release may run into such issue (using older release happens quite often with esp8266 platform).

Thanks this is now very clear @per1234 !

My next question would be whether it is fair/easy/possible to add some kind of warning in Arduino IDE 1.8.16+ (or matching Arduino CLI) that would inform the user that a deprecated workaround is used in some library specification, that would potentially lead to incompatibilities ? Such message would be, I think, valuable in bug reports.

@per1234
Copy link
Contributor Author

per1234 commented May 31, 2021

Current and future maintainers of such platform have to be aware that users on older platform release may run into such issue

I agree. This was part of the motivation behind temporarily reverting the change and immediately making new releases without the change once the scope of the impact became clear. This time around, we are making extra effort to give affected projects time to prepare for the change, providing clear documentation and communication about it to make sure everyone is aware in advance.

whether it is fair/easy/possible to add some kind of warning in Arduino IDE 1.8.16+ (or matching Arduino CLI) that would inform the user that a deprecated workaround is used in some library specification, that would potentially lead to incompatibilities ?

Well, in some ways this is already done via the "Multiple libraries were found for ..." message shown by all Arduino development software in this situation. For example:

Multiple libraries were found for "SD.h"
 Used: C:\Users\per\Documents\Arduino\libraries\SD
 Not used: C:\Users\per\AppData\Local\Arduino15\packages\esp32\hardware\esp32\1.0.6\libraries\SD

The tricky thing is that library overrides are a normal occurrence and a very useful feature. The boards platforms use it to provide their own board's versions of the standalone libraries. Library Manager uses it to provide updates for the IDE's pre-installed "built-in" libraries. The users may use it to override stock libraries with customized versions.

So you can't treat an occurrence of an override as an error, nor even a warning. In fact, I regularly see the "Multiple libraries were found for ..." informational message cause undue alarm to users who think it indicates a problem when in reality it is a case where everything is working just as intended and the correct library was selected.

I suppose an additional message could be added for when "Library Name Priority" resulted in a priority inversion when compared to how it would have been prior to the introduction of this new priority determination factor. But you can't treat that as a warning because this new factor was introduced specifically to deal with other cases where the previous priority factors resulted in the wrong library being selected. So a different priority result after this change is not necessarily a bad thing (otherwise there would be no point in it).

I definitely am interested in hearing any ideas others have on the subject.

@d-a-v
Copy link
Contributor

d-a-v commented May 31, 2021

To make the future incompatibility crystal clear, I still have some interrogations.
In this example, in the future versions of Arduino-CLI, the (esp32) part in the name in the library properties file is not going to work anymore:

-name=SD(esp32)
+name=SD

I guess it won't work because the name SD(esp32) won't be describing a library name called SD. Is that so ?
Is this kind of name SD(blah) going to be treated as an error or as a plain valid library name (for sure different from "SD but only for blah platform" like before) ?

@per1234
Copy link
Contributor Author

per1234 commented May 31, 2021

it won't work because the name SD(esp32) won't be describing a library name called SD

It won't work because, when using the library's primary header file name in an #include directive, it doesn't provide as high a "Library Name Priority" as the standalone SD library which is bundled with the classic Arduino IDE and also available for installation via Library Manager.

The old priority system is documented here:
https://arduino.github.io/arduino-cli/latest/sketch-build-process/#dependency-resolution
and implemented here:
https://github.com/arduino/arduino-cli/blob/0.18.3/arduino/libraries/librariesresolver/cpp.go

Comparison of priority scores for #include <SD.h> with the old system:

Platform bundled standalone
architecture 1010 1000
folder name 500 500
location 2 0 or 3
total 1512 1500 or 1503

Platform bundled library wins because it is "Architecture optimized" (e.g., architectures=esp32) while the standalone library is "Architecture compatible" (architectures=*).

The new priority system is documented here:
https://arduino.github.io/arduino-cli/dev/sketch-build-process/#dependency-resolution
and implemented here:
https://github.com/arduino/arduino-cli/blob/cdbebe98f895c18146ea2607cfb706d002b01191/arduino/libraries/librariesresolver/cpp.go

Comparison of priority scores for #include <SD.h> with the new system:

Platform bundled standalone
architecture 1010 1000
library name or folder name 500 600
location 2 0 or 3
total 1512 1600 or 1603

The standalone library wins because, due to its name value matching exactly with the header filename, it has perfect "Library Name Priority" and "Folder Name Priority", whereas the platform bundled library only has perfect "Folder Name Priority" due to it having a name value (e.g., name=SD(esp32)) that doesn't match the header filename.

Is this kind of name SD(blah) going to be treated as an error or as a plain valid library name (for sure different from "SD but only for blah platform" like before) ?

It is currently "sanitized" to sd_esp32_ before being compared against the "simplified" library header filename.

However, note that the previous use of the parentheses characters in the library name technically makes it in violation of the Arduino Library Specification:
https://arduino.github.io/arduino-cli/latest/library-specification/#libraryproperties-file-format

name - the name of the library. Library names must contain only basic letters (A-Z or a-z) and numbers (0-9), spaces ( ), underscores (_), dots (.) and dashes (-). They must start with a letter or number. They must contain at least one letter.

This specification is not currently enforced on platform bundled libraries, but it's definitely a good idea to always follow the specification, as there is no guarantee of support for non-compliance. I'm actually to blame for the choice to use parentheses in the library name when modifying the library names as a workaround for arduino/Arduino#4189 back in the 2015 era. At that time, we didn't have any specification as to the library name requirements. I certainly cringed later when I realized I had done this while establishing that documentation. But the poorly chosen characters has not done any harm so far. Although the workaround itself has now become problematic, the specification violation introduced coincidentally via the workaround is not relevant to the current situation. There are still some remaining platform bundled libraries with the non-compliant names (Servo being the one I noticed), but those were not affected by the "Library Name Priority" change, so I considered changing those names to be out of scope for the PRs I submitted for mitigating the harmful impact of "Library Name Priority".

for sure different from "SD but only for blah platform" like before

I should make clear that there is no magic specific to the (esp32) style of suffix. The workaround for arduino/Arduino#4189 only required that the library name value be changed to be different from the name of a library in Library Manager. Something like name=Foo SD would have worked just the same for that purpose.

@fstasi fstasi removed the type: bug label Sep 16, 2021
@rsora rsora added the type: imperfection Perceived defect in any part of project label Sep 22, 2021
@per1234
Copy link
Contributor Author

per1234 commented Nov 13, 2021

The "Library Name Priority" feature will be added back in the next release of Arduino CLI and the IDEs

I'll provide an update on this: the "Library Name Priority" feature was reintroduced in the following Arduino software versions

At the time I write this, it has not yet been reintroduced into arduino-builder or the classic Arduino IDE. They are still using (reference: builder, IDE) Arduino CLI 0.18.3, the last release before the reintroduction of "Library Name Priority".

@per1234
Copy link
Contributor Author

per1234 commented Mar 7, 2022

Since it has now been 6 months since the release of the change, the problematic library metadata has been corrected in all known affected platforms, and no significant recent reports of impacts, I think the need to leave this open for maximum visibility has passed.

@per1234 per1234 closed this as completed Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conclusion: resolved Issue was resolved topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

No branches or pull requests

4 participants