Fix "lib uninstall" when library name contains spaces #443

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

Jump to bottom

Merged

masci merged 1 commit into arduino:master from howjmay:core_search

Dec 11, 2019

Merged

Fix "lib uninstall" when library name contains spaces #443

masci merged 1 commit into arduino:master from howjmay:core_search

Dec 11, 2019

Conversation

howjmay

Copy link

Contributor

@howjmay howjmay commented Oct 15, 2019

Now, the "_" is replaced with a blanket " ", to reslove the
inconsistency problem. However, use the name of the installing
library may be a better solution.

fixes #362

BTW, when we uninstall libraries, there are no messages to notify the user libraries are successfully uninstalled.
However, when we install libraries message like Installed LiquidCrystal I2C@1.1.2 is printed to notify libraries are successfully installed.

@masci

Copy link

Contributor

masci commented Oct 16, 2019

Thanks for the PR!
Before we start reviewing, can you fix these two tests that are failing?
https://github.com/arduino/arduino-cli/blob/master/legacy/builder/test/libraries_loader_test.go#L92
https://github.com/arduino/arduino-cli/blob/master/legacy/builder/test/libraries_loader_test.go#L200

@masci masci added the component/core label

Oct 16, 2019

@howjmay howjmay force-pushed the core_search branch 2 times, most recently from cdcd726 to 07c646b Compare

October 20, 2019 07:11

@howjmay howjmay changed the title ~~(削除) Libraries (un)installation name inconcistency (削除ここまで)~~ (追記) (WIP) Libraries (un)installation name inconcistency (追記ここまで)

Oct 20, 2019

@rsora rsora added the status: in progress label

Oct 22, 2019

@masci

Copy link

Contributor

masci commented Oct 22, 2019

@howjmay we're almost there, now integration tests are failing here: https://github.com/arduino/arduino-cli/blob/master/test/test_lib.py#L61
You can find the nstructions to run integration tests locally here: https://github.com/arduino/arduino-cli/tree/master/test let me know if you need any guidance, those tests require a Python3 interpreter to run.

@howjmay

Copy link

Contributor Author

howjmay commented Oct 23, 2019

Thanks @masci
It seems I may meet a problem, but I am still digging it.
I will ask for help if I can't solve alone!

@howjmay

Copy link

Contributor Author

howjmay commented Nov 1, 2019

Hi @masci Sorry for answering late. My PC was broken in the last few weeks.
I met a problem that in

arduino-cli/arduino/utils/filenames.go

Line 22 in 292277f

func SanitizeName(origName string) string {

Name with space " " would be replace with underline "_", but this would cause a problem if I simply transform an underline back to space. Because the underline may be transformed from other symbol.

I am kind confused that how can I solve this problem. Maybe we need a naming rule for libraries ?

@zmoog

Copy link

Contributor

zmoog commented Nov 8, 2019

Hi @howjmay, unfortunately, we currently don't have a specification for the library names.

The original library name is changed here

arduino-cli/arduino/libraries/librariesmanager/install.go

Line 40 in bc6c823

saneName := utils.SanitizeName(indexLibrary.Library.Name)

using the SanitizeName() function to be more filesystem friendly and is also used internally as an identifier for the library within the LibrariesManager data structures.

Why don't we limit the use of the sanitized version of the library name inside the librariesmanager package only?

We could change this:

arduino-cli/arduino/libraries/librariesmanager/librariesmanager.go

Line 212 in bc6c823

alternatives, have := sc.Libraries[libRef.Name]

into something like this:

 saneName := utils.SanitizeName(libRef.Name)
 alternatives, have := sc.Libraries[saneName]

All the sanitize/normalization stuff would remain inside the librariesmanager package making it easier to change it into something else later on.

@howjmay

Copy link

Contributor Author

howjmay commented Nov 13, 2019

@zmoog Thank you !!
I didn't be aware there is a way like this!!

@howjmay howjmay force-pushed the core_search branch 5 times, most recently from 6d07a91 to e09e367 Compare

November 13, 2019 18:22

@howjmay howjmay changed the title ~~(削除) (WIP) Libraries (un)installation name inconcistency (削除ここまで)~~ (追記) Libraries (un)installation name inconcistency (追記ここまで)

Nov 13, 2019

@howjmay

Copy link

Contributor Author

howjmay commented Nov 18, 2019

Hi @masci I failed in CI for some unknown issue. Do you know what happen?

@masci

Copy link

Contributor

masci commented Nov 18, 2019

@howjmay my apologies, the CI system is currently disabled due to administrative problems but it'll be back online soon. For the time being I'm afraid you have to rely on running the tests locally, thanks for your patience!

@howjmay

Copy link

Contributor Author

howjmay commented Dec 3, 2019

Hi @masci How is the situation of CI?
Last time I ran the test of this PR, I passed it, even though I met some problems to run tests again.

@masci

Copy link

Contributor

masci commented Dec 3, 2019

@howjmay CI is back, you can add a commit and push (or just force-push last commit) to retrigger

@howjmay


 feat: Libraries (un)install name inconcistency

c99fc5b

Fix the inconsistent name of libraries when installing
and uninstalling.

@howjmay howjmay force-pushed the core_search branch from e09e367 to c99fc5b Compare

December 4, 2019 14:15

@howjmay

Copy link

Contributor Author

howjmay commented Dec 5, 2019

done

@masci masci removed the status: in progress label

Dec 5, 2019

@masci masci self-requested a review

December 5, 2019 16:45

@masci masci added this to the 0.7.0 milestone

Dec 11, 2019

masci

masci approved these changes

Dec 11, 2019

View reviewed changes

Copy link

Contributor

@masci masci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I wrote an integration test for this feature, I'll push it in a separate PR.
Thanks for your code!

@masci masci changed the title ~~(削除) Libraries (un)installation name inconcistency (削除ここまで)~~ (追記) Fix library uninstall when library name contains spaces (追記ここまで)

Dec 11, 2019

@masci masci changed the title ~~(削除) Fix library uninstall when library name contains spaces (削除ここまで)~~ (追記) Fix "lib uninstall" when library name contains spaces (追記ここまで)

Dec 11, 2019

@masci masci merged commit 4f3fec6 into arduino:master

Dec 11, 2019

@howjmay

Copy link

Contributor Author

howjmay commented Dec 11, 2019

Thank you @masci

masci pushed a commit that referenced this pull request

Dec 11, 2019


 add test for #443

4ec3bc5

@masci masci mentioned this pull request

Dec 11, 2019

"lib list" now returns an empty json array when there are no libraries installed #511

Merged

masci pushed a commit that referenced this pull request

Dec 13, 2019


 "lib list" now returns an empty json array when there are no librarie...

e164137

...s installed (#511)
* add test for #443
* return an empty json array when there are no libs

Labels

None yet

4 participants

@howjmay @masci @zmoog @rsora

Uh oh!

Fix "lib uninstall" when library name contains spaces #443

Fix "lib uninstall" when library name contains spaces #443

Uh oh!

Conversation

@howjmay howjmay commented Oct 15, 2019

Uh oh!

masci commented Oct 16, 2019

Uh oh!

masci commented Oct 22, 2019

Uh oh!

howjmay commented Oct 23, 2019

Uh oh!

howjmay commented Nov 1, 2019

Uh oh!

zmoog commented Nov 8, 2019

Uh oh!

howjmay commented Nov 13, 2019

Uh oh!

howjmay commented Nov 18, 2019

Uh oh!

masci commented Nov 18, 2019

Uh oh!

howjmay commented Dec 3, 2019

Uh oh!

masci commented Dec 3, 2019

Uh oh!

howjmay commented Dec 5, 2019

Uh oh!

@masci masci left a comment

Choose a reason for hiding this comment

Uh oh!

howjmay commented Dec 11, 2019

Uh oh!

Uh oh!