Check for nulls when using programmer #10033

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

Open

tedder wants to merge 2 commits into arduino:master

from tedder:ted/test_for_programmer_npe

Open

Check for nulls when using programmer #10033

tedder wants to merge 2 commits into arduino:master from tedder:ted/test_for_programmer_npe

Conversation

tedder

Copy link

@tedder tedder commented Apr 11, 2020

Try to test and gracefully fail rather than throwing a NullPointerException.

See #10032 for an example of this exception being thrown.

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Core Features:

Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your core changes, as applicable?
Have you successfully ran tests with your changes

I have not compiled or run tests or linted this. I'm hoping this is a change that someone can shepherd into an upcoming release.

@tedder


 Check for nulls when using programmer

08ea1cc

Try to test and gracefully fail rather than throwing a NullPointerException.
See arduino#10032 for an example of this exception being thrown.

matthijskooijman

matthijskooijman reviewed

Apr 12, 2020

View reviewed changes

arduino-core/src/cc/arduino/packages/uploaders/SerialUploader.java

String programmer = PreferencesData.get("programmer");

if (programmer.contains(":")) {

String[] split = programmer.split(":", 2);

targetPlatform = BaseNoGui.getCurrentTargetPlatformFromPackage(split[0]);

Copy link

Collaborator

@matthijskooijman matthijskooijman Apr 12, 2020

Choose a reason for hiding this comment

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

I think this could again return null when an invalid package is specified in the "programmer". Or does this throw an exception instead?

Copy link

Author

Choose a reason for hiding this comment

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

As in the other comment. Trying to be defensive AND give a unique error message for future users who ask for support.

Copy link

Collaborator

@matthijskooijman matthijskooijman Apr 13, 2020

Choose a reason for hiding this comment

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

I'm a bit confused. I think this can return null without that resulting in a proper error message (but still a NullPointerException). Maybe you misunderstood my meaning?

arduino-core/src/cc/arduino/packages/uploaders/SerialUploader.java Outdated

TargetPlatform targetPlatform = BaseNoGui.getTargetPlatform();

if (targetPlatform == null) {

throw new RunnerException(tr("Gould not load platform for programmer. Ensure programmer selection is correct for the board."), false);

Copy link

Collaborator

@matthijskooijman matthijskooijman Apr 12, 2020

Choose a reason for hiding this comment

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

Do you have any idea why targetPlatform might end up null? Is this when an invalid board is selected? I think this is still unrelated to having a programmer selected?

Copy link

Author

Choose a reason for hiding this comment

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

here's where GetTargetPlatform returns null. I think it might be when a programmer that isn't connected is selected.

Copy link

Collaborator

@matthijskooijman matthijskooijman Apr 13, 2020

Choose a reason for hiding this comment

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

I don't think that this is related to the selected programmer, and I'm pretty sure that the IDE doesn't actually check for a connected programmer at all (it does not know how, often programmers have specific USB interfaces that the IDE does not know about).

In this case, I think the "targetPlatform" is based on the target package and platform preferences, which, along with the board preference, together indicate the selected board. So I guess the getTargetPlatform can return null when no board is selected (which might not be possible, since there is always a default board selected I think), or when the currently selected board is no longer available (because the platform was uninstalled, or the board was removed from it in an update).

So I guess the error message should talk about the board selection, rather than the programmer selection?

Have you been able to specifically reproduce this null, to verify that it is handled correctly with your PR?

Copy link

Author

@tedder tedder Apr 13, 2020

Choose a reason for hiding this comment

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

I haven't set up a build environment. I was hoping this would be a start for a core maintainer to take on. If you don't think it's helpful, that's fine.

Copy link

Collaborator

@matthijskooijman matthijskooijman Apr 13, 2020

Choose a reason for hiding this comment

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

AH, I see you already said you didn't compile this, sorry for missing that. I think this is helpful, but the error messages probably need a bit more tweaking to be a bit more appropriate :-)

As for a maintainer taking this on, with all the load that is already there, I would not expect that to happen quickly, but maybe some other contributor can work on top of this (I won't have time for it, probably).

@tedder


 Fix typo in string

db872fc

@cmaglie

Copy link

Member

cmaglie commented Apr 27, 2020

The best fix for this is changing BaseNoGui.getTargetPlatform() to return an Optional<TargetPlatform> instead of a TargetPlatform.

This change will trigger compile errors on every place where getTargetPlatform is used: at this point all the errors should be fixed by porting the old code to the Optional API and, at the same, audit the code and see if the Optinal.empty() (previously null) is handled correctly on every place.

@CLAassistant

Copy link

CLAassistant commented Apr 9, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Labels

None yet

4 participants

@tedder @cmaglie @CLAassistant @matthijskooijman

Uh oh!

Check for nulls when using programmer #10033

Are you sure you want to change the base?

Check for nulls when using programmer #10033

Uh oh!

Conversation

@tedder tedder commented Apr 11, 2020

All Submissions:

Changes to Core Features:

Uh oh!

@matthijskooijman matthijskooijman Apr 12, 2020

Choose a reason for hiding this comment

Uh oh!

@tedder tedder Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

@matthijskooijman matthijskooijman Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

@matthijskooijman matthijskooijman Apr 12, 2020

Choose a reason for hiding this comment

Uh oh!

@tedder tedder Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

@matthijskooijman matthijskooijman Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

@tedder tedder Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

@matthijskooijman matthijskooijman Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

cmaglie commented Apr 27, 2020

Uh oh!

CLAassistant commented Apr 9, 2021

Uh oh!

Uh oh!