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

Arduino library compatibility #92

Merged
merged 6 commits into from
Jan 19, 2022
Merged

Arduino library compatibility #92

merged 6 commits into from
Jan 19, 2022

Conversation

santaimpersonator
Copy link
Contributor

@santaimpersonator santaimpersonator commented Jun 30, 2021

As mentioned in this issue (#85), the library name and file names don't comply with Arduino's specifications:

Library names must contain only basic letters (A-Z or a-z) and numbers (0-9), spaces ( ), underscores (_), dots (.) and dashes (-).

  • The library was renamed from TinyGPS++ to TinyGPSPlus (which also reflects the repository name)
  • The source files were renamed to TinyGPS++ to TinyGPSPlus
    • All mentions of TinyGPS++ in the code and examples were also transitioned to TinyGPSPlus
  • The library version was bumped to 2.0.0 to reflect backwards incompatibility due to renaming of the source files

Rename src files and library to follow Arduino's library guidelines:
https://arduino.github.io/arduino-cli/latest/library-specification/
Bump version to 2.0.0
- Not backwards compatible with file name changes
@KenwoodFox
Copy link

Can we get this merged?

@mikalhart
Copy link
Owner

I like this idea, but first a question. Is there any way of accomplishing this without breaking every TinyGPS++ sketch ever written? I thought to try creating a dummy TinyGPSPlus.h file that simply #included the (current) TinyGPS++.h, but I'm not sure this completely solves the problem, does it?

@per1234
Copy link

per1234 commented Jul 23, 2021

The only change that is required in order to be accepted into the Arduino Library Manager index is to make the name value in library.properties compliant with the requirements mentioned in 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. Note that libraries with a name value starting with Arduino will no longer be allowed addition to the Library Manager index as these names are now reserved for official Arduino libraries.

Once a library is in Library Manager, changing this name value is more significant because it is the unique identifier used to refer to the library in machine operations (e.g., arduino-cli lib install TinyGPSPlus), but for a library not yet in the index, the only current use of the name value is to identify the library to the user (e.g., File > Examples > TinyGPS++). That could also be seen as breaking because it might make some tutorials outdated, but I don't think it will have too serious of an impact on the users.


In addition to checking the libraries for compliance with the hard requirements that allow them to play nicely with the system, the Arduino Lint tool used by the system also runs checks for best practices. Violations of these result in a warning. There is no requirement to resolve these warnings. They are only suggestions for possible improvements.

One of these suggestions is for the library to contain a header file matching the library name. The library name determines the Library Manager installation folder name and header/library folder name matching is a factor in dependency resolution when two installed libraries contain a header matching an #include directive in the code being compiled. So it is a good thing to do, but it is not required. I think the approach of adding a wrapper header is the right thing to do.

Wrapper header for library name change
Provides backwards compatibility
revert version bump
Wrapper header for dummy header
@santaimpersonator
Copy link
Contributor Author

@per1234 Thanks for the insight.
@mikalhart Sorry, I didn't consider that.

Let me know if you'd prefer that I create a new pull request from a branch with the commit history cleaned up.

@santaimpersonator
Copy link
Contributor Author

@mikalhart Just wanted to double check if the new changes work for you.

@per1234
Copy link

per1234 commented Jan 5, 2022

Hi @mikalhart. I just wanted to check in with you to see whether you have had a chance to look at this proposal.

If you have any questions or concerns regarding the Arduino Library Manager, I'll be happy to assist.

@per1234
Copy link

per1234 commented Jan 5, 2022

Update: I just noticed that while this proposal was pending another library claimed the "TinyGPSPLUS" library name in the Arduino Library Manager registry:
https://github.com/Tinyu-Zhao/TinyGPSPlus

Since it is the sole unique identifier of the library, each library must have a unique name (case insensitive). So you will need to pick another one that has not yet been taken.

@santaimpersonator
Copy link
Contributor Author

@per1234 That library (https://github.com/Tinyu-Zhao/TinyGPSPlus) appears to be a fork of this repository. If @mikalhart decides to incorporate these changes, could we overwrite the forked library with this one?

Otherwise, I can leave a note in issue #85 about the "TinyGPSPLUS" library name being taken and close all the related pull requests, until another library name is decided on.

@TD-er
Copy link
Contributor

TD-er commented Jan 5, 2022

For PlatformIO library naming, you can prefix the name with the Github account, just like in the url here.

Not sure how Arduino libraries are managed, but you could simply create a name with the Github name prefixed?

@per1234
Copy link

per1234 commented Jan 5, 2022

could we overwrite the forked library with this one?

The only way to gain possession of the name would be for the owner to request Arduino to either remove their library from the Library Manager or else to change the name it is registered under. You would need to work that out with the current owner of the name.

you could simply create a name with the Github name prefixed?

Here are the naming requirements:

https://arduino.github.io/arduino-cli/latest/library-specification/#libraryproperties-file-format

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. Note that libraries with a name value starting with Arduino will no longer be allowed addition to the Library Manager index as these names are now reserved for official Arduino libraries.

So you are welcome to use any name that meets those requirements and is not already taken.

@santaimpersonator
Copy link
Contributor Author

@per1234 @mikalhart

I'll close this pull request and arduino/library-registry#110; and leave issue #85 open with a note about the library name being taken. I am happy to submit new pull requests, when a library name is chosen.

@svdrummer
Copy link

I saw discussions on adding the name to Arduino. IT went on for months, so the other guy just went ahead and placed it in the arduino library repository. It wasn't done in secret or anything...just a guy being proactive.

@TD-er
Copy link
Contributor

TD-er commented Jan 6, 2022

Yep, it may be done with good intentions, but now there is the problem that the one that is in the Arduino library manager is not the one of the main repository.
So now there is a dependency on a fork to keep the one in the lib manager up-to-date.

@santaimpersonator
Copy link
Contributor Author

Reopening pull request in regards to checklist: #85 (comment)

@mikalhart mikalhart self-requested a review January 19, 2022 23:50
@mikalhart
Copy link
Owner

Thanks to all of you for the attention to this issue. @santaimpersonator @per1234

@mikalhart mikalhart merged commit 81ba32c into mikalhart:master Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants