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

field "cc.arduino.cli.commands.Library.Maintainer" contains invalid UTF-8 #1907

Closed
JAndrassy opened this issue Oct 21, 2019 · 19 comments · Fixed by #1969
Closed

field "cc.arduino.cli.commands.Library.Maintainer" contains invalid UTF-8 #1907

JAndrassy opened this issue Oct 21, 2019 · 19 comments · Fixed by #1969
Assignees
Labels
conclusion: resolved Issue was resolved criticality: low Of low impact topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface type: imperfection Perceived defect in any part of project

Comments

@JAndrassy
Copy link

JAndrassy commented Oct 21, 2019

Describe the problem

Library Manager doesn't show libraries. Output console shows [INFO] ERROR: 2019/10/21 21:46:10 grpc: server failed to encode response: rpc error: code = Internal desc = grpc: error while marshaling: proto: field "cc.arduino.cli.commands.Library.Maintainer" contains invalid UTF-8

Operating system

  • Linux
  • Windows

Operating system version

  • Linux Mint 18.3
  • Windows 11

Additional context

Additional reports

@JAndrassy

This comment was marked as resolved.

@spoenemann
Copy link

Do you have the arduino-cli already installed? If yes, which version? (Try arduino-cli version in a terminal)

@JAndrassy
Copy link
Author

I didn't install arduino-cli. Doesn't v0.0.3-alpha.preview have cli bundled?

@spoenemann
Copy link

spoenemann commented Jan 13, 2020

Yes, but if it's found in the global PATH, that version is taken (see arduino/arduino-pro-ide#142).

@JAndrassy
Copy link
Author

duro@nuc ~ $ arduino-cli version
arduino-cli: command not found
duro@nuc ~/Arduino Pro IDE-v0.0.3-linux $ ./arduino-pro-ide 
root WARN please install @theia/electron@0.14.0-next.0159cd5b as a runtime dependency
root INFO Theia app listening on http://localhost:42569.
daemon INFO >>> Starting arduino-cli version: 0.6.0 commit: 2049a7a daemon from /home/duro/Arduino Pro IDE-v0.0.3-linux/resources/app/node_modules/arduino-ide-extension/build/arduino-cli...

@per1234
Copy link
Contributor

per1234 commented Feb 18, 2021

Hi @JAndrassy. Thanks so much for taking the time to report this!

Would you mind downloading the new 0.1.4 release of Arduino Pro IDE or and letting us know whether the bug still occurs?

From the message, I suspect that the issue is specific to the contents of one of the package index files provided by the Boards Manager URLs. Do you remember if you were using any custom URLs, or did you just have the default configuration, which only provides Boards Manager with the official Arduino package index?

@JAndrassy
Copy link
Author

Hi @JAndrassy. Thanks so much for taking the time to report this!

Would you mind downloading the new 0.1.4 release of Arduino Pro IDE or and letting us know whether the bug still occurs?

From the message, I suspect that the issue is specific to the contents of one of the package index files provided by the Boards Manager URLs. Do you remember if you were using any custom URLs, or did you just have the default configuration, which only provides Boards Manager with the official Arduino package index?

sorry. Per. I don't have time now for this. the problem then was my name in my published library. I modified library.properties for next version, so now library.properties of my libraries doesn't have UTF character

@per1234
Copy link
Contributor

per1234 commented Feb 19, 2021

Thats enough for me (and yeah, I'm dumb for saying it was a package index). I found the culprit here (ANSI encoding):
https://github.com/jandrassy/arduino-library-wifilink/blob/45dda6563bcf9d5c35015beef93054cc1bf8af7a/library.properties#L4

maintainer=Juraj Andrássy <juraj.andrassy@gmail.com>

Steps to reproduce:

  1. Run this command from your Arduino libraries folder (or download the equivalent zip archive and install if you prefer):
    git clone https://github.com/jandrassy/arduino-library-wifilink && cd arduino-library-wifilink && git checkout 45dda6563bcf9d5c35015beef93054cc1bf8af7a
    
  2. Compile a sketch that uses the library:
    #include <WiFiLink.h>
    void setup() {}
    void loop() {}

The compilation process fails:

Sketch uses 2182 bytes (0%) of program storage space. Maximum is 253952 bytes.
Global variables use 226 bytes (2%) of dynamic memory, leaving 7966 bytes for local variables. Maximum is 8192 bytes.
Compilation error: Error: 13 INTERNAL: grpc: error while marshaling: string field contains invalid UTF-8

You can see that the code actually compiled just fine, but this error will block uploading.

The issue does not occur when doing the same with the classic Arduino IDE or Arduino CLI.

@ubidefeo
Copy link

@per1234 what's your advice on this?
should we force CLI to accept other encodings?
If so, let's turn it into a task

@per1234
Copy link
Contributor

per1234 commented Feb 19, 2021

In order to fully form an opinion, I would need to understand two things:

  • How much complexity would need to be added to the code to support them
  • What are the use cases where non-UTF-8 encodings are beneficial

If it can be done without significantly harming the maintainability of the code, I'm all for it. Even if we document a UTF-8 requirement in the specifications, people are still going to end up inadvertently using other encodings, so there will always be a benefit to supporting them. But if this is something difficult to support, then I would want to be able to understand better the cost vs benefit balance.

Maybe @silvanocerza or @kittaakos would be able to provide some insight into what would we needed to support other encodings.

Maybe @JAndrassy would be able to provide some insight into the use case for non-UTF-8 encoding.

@JAndrassy
Copy link
Author

Normally a .properties file should be ISO 8859-1 encoding (Latin-1) and non-Latin-1 characters should be entered by using Unicode escape characters. I use Eclipse IDE and it automatically uses 8859-1 for .properties files.

@kittaakos kittaakos self-assigned this Feb 26, 2021
@kittaakos
Copy link
Contributor

People suggest using bytes instead of string if the data may contain non-UTF-8 characters: protocolbuffers/protobuf#4970 (comment)

What do you think @cmaglie?

@rsora rsora transferred this issue from arduino/arduino-pro-ide Mar 1, 2021
@silvanocerza silvanocerza added the status: waiting for information More information must be provided before work can proceed label Mar 1, 2021
@rsora rsora added the priority: low Resolution is a low priority label Mar 1, 2021
@cmaglie
Copy link
Member

cmaglie commented Mar 3, 2021

Looking at this article https://en.wikipedia.org/wiki/.properties I see that, starting from Java 9, the default encoding has been changed to UTF-8

In Java 9 and newer, the default encoding for .properties files is UTF-8, and if an invalid UTF-8 byte sequence is encountered it falls back to ISO-8859-1.

maybe we can implement a fallback to ISO-8859-1 in the properties parser. BTW if this turns out to be too much work I would just stick to UTF-8.

@kittaakos
Copy link
Contributor

maybe we can implement a fallback to ISO-8859-1 in the properties parser. BTW if this turns out to be too much work I would just stick to UTF-8.

It's not about the properties parser, but the fact that the maintainer field is string:

// Value of the `maintainer` field in library.properties.
string maintainer = 3;

So no matter what you do with the parser on the CLI side, non-UTF-8 chars won't go through the wire if the gRPC property is string and not bytes.

From the docs:

A string must always contain UTF-8 encoded or 7-bit ASCII text.

@cmaglie
Copy link
Member

cmaglie commented Mar 3, 2021

Yes, what I mean is that we must check that the string is UTF8 before sending it (in this case the input comes from a library.properties file so IMHO we should improve the checks there).

Otherwise, what's the point of sending a malformed UTF-8? you won't have any chance to display it correctly since you don't know the encoding...

@cmaglie
Copy link
Member

cmaglie commented Mar 3, 2021

In the arduino-cli to read the libary.properties we use the parser here -> https://github.com/arduino/go-properties-orderedmap/blob/master/properties.go#L177

I did not investigate how to do UTF-8 sanity checks in golang, BTW fixing this function (to reject non-UFT8 or to fallback to ISO-8859-1 as for java properties files) would automatically solve this issue. We should probably move this issue to the arduino-cli.

@EliasA97

This comment was marked as duplicate.

@kittaakos kittaakos removed their assignment Jul 18, 2021
@rsora rsora added the type: imperfection Perceived defect in any part of project label Sep 22, 2021
@per1234 per1234 added the topic: code Related to content of the project itself label Nov 1, 2021
@rsora rsora added criticality: low Of low impact and removed priority: low Resolution is a low priority labels Nov 2, 2021
@EliasA97

This comment was marked as duplicate.

@ubidefeo

This comment was marked as resolved.

@per1234 per1234 transferred this issue from arduino/arduino-ide Oct 5, 2022
@per1234 per1234 added the topic: gRPC Related to the gRPC interface label Oct 5, 2022
@umbynos umbynos added this to the Arduino CLI 1.0 milestone Oct 12, 2022
cmaglie added a commit to cmaglie/arduino-cli that referenced this issue Nov 8, 2022
cmaglie added a commit that referenced this issue Nov 8, 2022
* Added test for #1907

* Updated github.com/arduino/go-properties-orderedmap to v1.7.1
@per1234 per1234 added the conclusion: resolved Issue was resolved label Nov 17, 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 criticality: low Of low impact topic: code Related to content of the project itself topic: gRPC Related to the gRPC interface type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants