Users/t anmah/simulation dependency checker fix by andreamah · Pull Request #169 · microsoft/vscode-python-devicesimulator

Andrea Mah (andreamah) · 2020-01-21T00:26:55Z

Description:

Fixed automated package installer for Python libraries needed for custom adafruit_circuitplayground.express library.

Fixed pywin32 requirements version for Python 3.8
Improved error handling for Python package installation; used to fail silently, but now pops up with an error message.
More robust way of checking whether package installation failed; now checks for presence of ./out/python_libs to see if installation failed.
improved README + etc. docs for new dependency workflow (no more need to pip install packages globally).

Type of change

[ x ] Bug fix (non-breaking change which fixes an issue)

Limitations:

Sound functionality still fails on Mac OS. Issue already recorded on backlog item 28661.

Testing:

Manual testing (test with/without dependencies installed prior, test with bad requirements.txt, tested on Mac OS, and Windows.

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

… for unix

David Serafimov (nasadigital)

Solid work, very neat solution :)

David Serafimov (nasadigital) · 2020-01-21T15:54:55Z

+applicationinsights==0.11.9
+python-socketio==4.3.1
+requests==2.22.0
+pywin32>=224


Do you think it would make sense to fix pywin32's version to 224 in case there are breaking changes in future versions?

pywin32 can't download version 224 on Python 3.8 :(

Thanks for clarifying!
What about fixing it to a newer version? Are new versions compatible with Python 3.7?

I tested pywin32 ver 227 on python 3.7 and it seemed to work! I'll fix it to ver 227.

Isa (isadorasophia)

Nice job! ⚡️

Isa (isadorasophia) · 2020-01-21T18:53:48Z

-   install by typing the following commands in a console: `pip install requests`
- _**Application Insights**_  
-  install by typing the following commands in a console: `pip install applicationinsights`
+- Python Modules for Deploy to Device


This is not on the scope of this PR, so don't worry! I wonder if we should move these install instructions from the README.md and just point to the install.md instead? Otherwise we just have dupe information (or we can make the whole README overwhelming for new users).

I think that the instructions were here beforehand too (or at least the pip install commands). But yeah, it would maybe be a good idea to move all installation-related stuff to install.md for clarity (I noticed that lots of parts repeat between docs)

Yup -- you just followed the pattern of what we have before, so it's all good! However, I think it might be worth adding a work item to do that in the future? (basically linking stuff at README so it's more doc-based).

David Serafimov (nasadigital)

Thanks for applying fixes!
LGTM :)

Vandy Liu (vandyliu) · 2020-01-22T00:05:53Z

+    - If you're on Windows, use the following command in the console to install pywin32: `pip install pywin32`
+
+- Python Modules for Simulation
+  - **Note:** On extension activation, you will be prompted with a popup message asking if you want the modules to be automatically installed for you. The following Python modules should be downloaded when you select "yes" on the prompt message. **If modules are not installed correctly, please use the "pip install" commands listed below.**


should this note be at the top/or placed somewhere else because it also applies to "Deploying to Device"

Just a suggestion on how we can address this, instead of having these two topics, we can have something along these lines:
(I guess the same goes for the text on Install.md and developers-setup.md)

Python Modules for Deploying to Device

Python Modules for Simulation

We can just have:

(Notes here)

Python Modules

Pywin32 (only windows etc)
Running the simulator:

(list extra modules here)

I tried to make it clearer with the newest commit. Is it a bit more organized now?

Isa (isadorasophia) · 2020-01-22T00:23:55Z

+    - If you're on Windows, use the following command in the console to install pywin32: `pip install pywin32`
+
+- Python Modules for Simulation
+  - **Note:** On extension activation, you will be prompted with a popup message asking if you want the modules to be automatically installed for you. The following Python modules should be downloaded when you select "yes" on the prompt message. **If modules are not installed correctly, please use the "pip install" commands listed below.**


Just a suggestion on how we can address this, instead of having these two topics, we can have something along these lines:
(I guess the same goes for the text on Install.md and developers-setup.md)

Python Modules for Deploying to Device

Python Modules for Simulation

We can just have:

(Notes here)

Python Modules

Pywin32 (only windows etc)
Running the simulator:

(list extra modules here)

Andrea Mah (andreamah) added 2 commits January 20, 2020 16:07

fixed python package downloads for py 3.8 and removed pywin32 install…

6c77641

… for unix

documentation updates for deploy to device package correction

dd00ae1

Andrea Mah (andreamah) requested review from Vandy Liu (vandyliu) and Kevin Nguyen (xnkevinnguyen) January 21, 2020 00:26

Andrea Mah (andreamah) requested review from Luke Slevinsky (LukeSlev), Ahmed Mahdy (abmahdy), Andrew Clements (adclements), Jennifer Davis (iennae), Mark Schramm (markAtMicrosoft) and smmatte as code owners January 21, 2020 00:26

David Serafimov (nasadigital) reviewed Jan 21, 2020

View reviewed changes

Isa (isadorasophia) suggested changes Jan 21, 2020

View reviewed changes

incorporated PR feedback

76c04c5

David Serafimov (nasadigital) approved these changes Jan 21, 2020

View reviewed changes

Vandy Liu (vandyliu) reviewed Jan 21, 2020

View reviewed changes

Comment thread src/constants.ts Outdated

Vandy Liu (vandyliu) reviewed Jan 22, 2020

View reviewed changes

Comment thread src/extension_utils/utils.ts Outdated

Vandy Liu (vandyliu) reviewed Jan 22, 2020

View reviewed changes

Isa (isadorasophia) approved these changes Jan 22, 2020

View reviewed changes

Andrea Mah (andreamah) added 2 commits January 21, 2020 17:13

incorporating more PR comments for clarity

528c123

resolved merge conflicts

104e40e

Mark Schramm (markAtMicrosoft) approved these changes Jan 22, 2020

View reviewed changes

Andrea Mah (andreamah) merged commit 33ad456 into dev Jan 22, 2020

Vandy Liu (vandyliu) deleted the users/t-anmah/simulation-dependency-checker-fix branch January 23, 2020 03:23

Conversation

Andrea Mah (andreamah) commented Jan 21, 2020

Description:

Type of change

Limitations:

Testing:

Checklist:

Uh oh!

David Serafimov (nasadigital) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Isa (isadorasophia) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Isa (isadorasophia) Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

David Serafimov (nasadigital) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Isa (isadorasophia) Jan 22, 2020 •

edited

Loading