- 
          
- 
        Couldn't load subscription status. 
- Fork 511
Refactor Init process to use new CLI gRPC API #342
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
Conversation
| const { sketchUri, fqbn, compilerWarnings } = options; | ||
| const sketchPath = FileUri.fsPath(sketchUri); | ||
|  | ||
| await this.coreClientProvider.initialized; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@silvanocerza what happens if the initialized fails or rejects? is it ok to proceed calling coreClient()?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine, if the Init gRPC function fails it means it has been called, so Create was successful meaning we have an internal instance in the CLI daemon and we have a "working" client.
If Init fails completely though we must have some other huge problems, with the refactoring failure to load a platform or library just logs an error, but it doesn't stop completely.
I still don't like how I implemented this though, I'd like to show the user a notification when there are this kind of failures but am not sure how to handle that.
e6e139d    to
    f73f321      
    Compare
  
    | @silvanocerza 
 | 
| @ubidefeo I investigated a bit, the same issue is also present when uninstalling a library. It seems to me this is a race condition, the list of boards divided by platform is retrieved before the platform is uninstalled, the strange thing though is that after installing a new one I'd expect it to stop showing the uninstalled platform but it doesn't. 🤔 This requires some further investigation I guess. | 
b6b1073    to
    178a11d      
    Compare
  
    | Rebased to fix conflicts and trigger a new build after updating the version of the Arduino CLI. | 
be4546b    to
    178a11d      
    Compare
  
    178a11d    to
    13de2f8      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
been poking around for almost an hour and haven't found anything which isn't already an issue in the Beta 7.
LGTM
| This has been superseded by #506. Closing. | 

The CLI initialization process has been changed in significant ways with PR arduino/arduino-cli#1274, it needs to be thorougly tested before merging to avoid breaking the IDE.