-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add initial devcontainer support #20960
Conversation
.devcontainer/Dockerfile
Outdated
@@ -0,0 +1,4 @@ | |||
FROM mcr.microsoft.com/devcontainers/python:0-3.10 |
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 should be 3.7
, also, this should install the test requirements.
.devcontainer/Dockerfile
Outdated
FROM mcr.microsoft.com/devcontainers/python:0-3.10 | ||
|
||
RUN apt-get update | ||
# RUN apt-get install -y libatk1.0-0 libatk-bridge2.0-0 libdrm2 libgtk-3-0 libgbm-dev libasound2 |
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.
One more thing to do is set the python as CI_PYTHON_PATH
.
.devcontainer/devcontainer.json
Outdated
// Use 'forwardPorts' to make a list of ports inside the container available locally. | ||
// "forwardPorts": [], | ||
// Use 'postCreateCommand' to run commands after the container is created. | ||
"postCreateCommand": "npm install ; npm run compile", |
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 should also run gulp task to setup the python packages used by the extension.
bb5692f
to
ae3a0f4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
ae3a0f4
to
0d0d159
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.
Can you have a look at https://github.com/microsoft/vscode-pylint/pull/304/files? There seems to be several properties missing which are required, for eg. extensions
.
Also have a look at pr-check.yml
, we need to install certain requirement files as well. Feel free to @ mention me if you need help.
707660e
to
1341df1
Compare
@karrtikr Added Related to the 2nd point of |
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.
Please make sure to add these settings as well (microsoft/vscode-pylint#304):
.devcontainer/devcontainer.json
Outdated
"features": { | ||
"ghcr.io/devcontainers/features/node:1": { | ||
"nodeGypDependencies": true, | ||
"version": "16" |
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.
We're currently using Node 14.18: https://github.com/microsoft/vscode-python/wiki/Coding#prerequisites
1341df1
to
831a2c0
Compare
I don't know if it's the best approach but, among other things, I've considered the idea of creating a new task to achieve |
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.
Sorry, please don't remove any existing settings, only add new ones if it doesn't already exist in some form. I like you using gulp for this instead, but make sure it is available and installed in the codespace container.
|
||
|
||
RUN apt-get update | ||
RUN apt-get install -y libatk1.0-0 libatk-bridge2.0-0 libdrm2 libgtk-3-0 libgbm-dev libasound2 |
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.
Curious, where're these coming from?
scripts/post_create_command.sh
Outdated
|
||
gulp installPythonLibs | ||
npm ci | ||
npm run compile |
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.
Please remove this, it can be done manually.
831a2c0
to
1cea782
Compare
Added defaultFormatter for Python and tests location for Pytest.
Used `richie5um2.vscode-sort-json` extension
`python.sortImports.args` property will be removed soon
Set it as the 1st one to the existent `installPythonLibs` gulp task
- Replace npm install by npm ci in devcontainer - Remove npm run compile
1cea782
to
315681d
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.
LGTM otherwise
"features": { | ||
"ghcr.io/devcontainers/features/node:1": { | ||
"nodeGypDependencies": true, | ||
"version": "14.18" |
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.
@karthiknadig Please advise the node version to use here.
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.
16.17.1
"features": { | ||
"ghcr.io/devcontainers/features/node:1": { | ||
"nodeGypDependencies": true, | ||
"version": "14.18" |
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.
"version": "14.18" | |
"version": "16.17.1" |
"dbaeumer.vscode-eslint", | ||
"ms-python.python", | ||
"ms-python.black-formatter", | ||
"ms-python.vscode-pylance" |
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.
"ms-python.vscode-pylance" | |
"ms-python.vscode-pylance", | |
"charliermarsh.ruff" |
Hello! Thank you for the PR. While bulk of what you had in the devcontainer.json was great, we decided to go with slightly different approach when it comes to the Dockerfile and post/on/update Create commands. Instead of the python image that you have used, we decided to use the official Microsoft node version for the base docker image, while also adding pyenv to manage and install different versions of Python (3.7, 3.8, 3.9, 3.10, 3.11 latest..etc) All the commands would also be better suited to be relocated into onCreateCommand.sh, to leverage the benefits of prebuild and so users don't have to sit for their container to load with all the necessary requirement installation that we are doing! I do see you had some other changes in the PR, so please let us know or provide a different PR if you would like to get those changes in! Thank you so much!! Related: #21435 #21675 |
Closes #20942
Results inside DevContainer of
npm scripts
:test
❎ (see detailed steps taken below)test:unittests
✔️test:functional
❎testDebugger
❎testSingleWorkspace
❎testMultiWorkspace
❎testPerformance
❎Steps taken related to execution of
npm run test
command:1. After executing
npm run compile
successfully as apostCreateCommand
of thedevcontainer.json
file:2. After executing
sudo apt-get install libatk1.0-0
(ref: https://www.masmasit.com/2021/08/how-to-install-package-libatk-10so0-on_01058658241.html)3. After executing
sudo apt-get install libatk-bridge2.0-0
(ref: https://www.masmasit.com/2021/08/how-to-install-package-libatk-bridge-.html)4. After executing
sudo apt-get install libdrm2
(ref: https://ubuntu.pkgs.org/20.04/ubuntu-main-amd64/libdrm2_2.4.101-2_amd64.deb.html)5. After executing
sudo apt-get install libgtk-3-0
(ref: https://zoomadmin.com/HowToInstall/UbuntuPackage/libgtk-3-0)6. After executing
sudo apt-get install libgbm-dev
(ref: https://stackoverflow.com/a/67407105/4315310)7. After executing
sudo apt-get install libasound2
(ref: https://www.masmasit.com/2021/08/how-to-install-package-libasound-so-2-on.html)