-
Notifications
You must be signed in to change notification settings - Fork 79
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
Check if Python has sqlite3 is installed before attempting to install ipykernel #5149
base: main
Are you sure you want to change the base?
Conversation
@@ -23,6 +23,7 @@ export class ProductService implements IProductService { | |||
this.ProductTypes.set(Product.python, ProductType.Python); | |||
// --- Start Positron --- | |||
this.ProductTypes.set(Product.fastapiCli, ProductType.DataScience); | |||
this.ProductTypes.set(Product.sqlite3, ProductType.DataScience); |
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.
This is not really a data science package, but it is most like that given the current options in ProductType
. I did try adding a new type but then we would need to make an installer for it and that doesn't seem important, given that we can't install the missing system requirement for people in a straightforward way.
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.
IIUC the names are misleading. This looks like the right place.
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.
@@ -303,6 +303,8 @@ export function translateProductToModule(product: Product): string { | |||
// --- Start Positron --- | |||
case Product.fastapiCli: | |||
return 'fastapi_cli'; | |||
case Product.sqlite3: | |||
return '_sqlite3'; |
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.
Is this correct?
return '_sqlite3'; | |
return 'sqlite3'; |
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.
This is what generates the original error:
https://github.com/python/cpython/blob/main/Lib/sqlite3/dbapi2.py#L27
from _sqlite3 import *
@@ -23,6 +23,7 @@ export class ProductService implements IProductService { | |||
this.ProductTypes.set(Product.python, ProductType.Python); | |||
// --- Start Positron --- | |||
this.ProductTypes.set(Product.fastapiCli, ProductType.DataScience); | |||
this.ProductTypes.set(Product.sqlite3, ProductType.DataScience); |
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.
IIUC the names are misleading. This looks like the right place.
I just kicked off a release build here so we can have folks with easy access to a Linux machine to test it out. To test it out, you will need:
|
Using Fedora 40 desktop, I was able to see the new warning.
To confirm the happy path:
Note that sqlite by itself was not enough, it needed to be the |
I added this info to our docs here: posit-dev/positron-website#11 What do you think would be the most clear error message? Maybe a slight edit such as:
|
Addresses #4698
Currently the message says this if your Python doesn't have sqlite3 there:
Thoughts or feedback? We believe this is most likely to happen on Linux when you are missing a sysreq.
QA Notes
To confirm this is working as expected, you need two Python envs:
pyenv virtualenv
). You should be able to choose this env via "Python: Select Interpreter", agree to the ipykernel installation, and see the ipykernel installation succeed. ✅