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

Fix platform tool dependency determination #1020

Merged
merged 2 commits into from
Oct 12, 2020
Merged

Fix platform tool dependency determination #1020

merged 2 commits into from
Oct 12, 2020

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Oct 9, 2020

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?

Bug fix.

  • What is the current behavior?

The platform tool dependency check was giving false negatives due to comparing pointers instead of version values.

This caused tools to be removed during platform uninstallation even when another installed platform had a dependency on
that tool.

  • What is the new behavior?

The semver package's Equal method is used for the comparison, resulting in accurate tool dependency determination.

  • Does this PR introduce a breaking change?

No

  • Other information:

Steps to reproduce the bug fixed by this PR:

arduino-cli core install arduino-beta:mbed
arduino-cli core install arduino:samd
arduino-cli core uninstall arduino:samd
arduino-cli sketch new /tmp/Foo
arduino-cli compile -b arduino-beta-development:mbed:envie_m7 /tmp/Foo

arduino-cli core uninstall arduino:samd removes arduino:arm-none-eabi-gcc@7-2017q4, even though it's a dependency of the installed arduino-beta:mbed platform, causing the compilation for any board of that platform to fail.

Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Great job as always Per!
Would be awesome if you could add an integration test too, I feel it's an important thing we should test for.

The platform tool dependency check was giving false negatives due to comparing pointers instead of version values.

This caused tools to be removed during platform uninstallation even when another installed platform had a dependency on
that tool.
@per1234
Copy link
Contributor Author

per1234 commented Oct 12, 2020

Would be awesome if you could add an integration test too

Excellent suggestion @silvanocerza! I have now added an integration test: b13a29c

test/test_core.py Outdated Show resolved Hide resolved
Tool dependency by `arduino-cli core uninstall` is somewhat complex because it must only uninstall the tools that no
other platforms have a dependency on. If it doesn't, it breaks the other platform and the cause of this breakage would
likely not be obvious to the user.

So it's important to test to ensure this functionality continues to work correctly.
@per1234 per1234 merged commit 10d0790 into arduino:master Oct 12, 2020
@per1234 per1234 deleted the fix-platform-tool-dep-uninstall branch October 12, 2020 10:42
@per1234 per1234 self-assigned this Nov 23, 2021
@per1234 per1234 added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants