Users/t anmah/simulation dependency checker fix #169

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

nasadigital

Solid work, very neat solution :)

nasadigital · 2020-01-21T15:54:55Z

src/requirements-windows.txt

+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.

README.md

src/extension_utils/utils.ts

isadorasophia

Nice job! ⚡️

src/requirements-windows.txt

src/extension_utils/utils.ts

isadorasophia · 2020-01-21T18:53:48Z

README.md

-   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).

src/constants.ts

nasadigital

Thanks for applying fixes!
LGTM :)

src/constants.ts

src/extension_utils/utils.ts

vandyliu · 2020-01-22T00:05:53Z

README.md

+    - 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?

isadorasophia · 2020-01-22T00:23:55Z

README.md

+    - 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)

src/extension_utils/utils.ts

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

andreamah requested review from xnkevinnguyen and vandyliu January 21, 2020 00:26

andreamah requested review from abmahdy, adclements, iennae, LukeSlev, markAtMicrosoft and smmatte as code owners January 21, 2020 00:26

nasadigital reviewed Jan 21, 2020

View reviewed changes

isadorasophia suggested changes Jan 21, 2020

View reviewed changes

incorporated PR feedback

76c04c5

nasadigital approved these changes Jan 21, 2020

View reviewed changes

vandyliu reviewed Jan 21, 2020

View reviewed changes

src/constants.ts Outdated Show resolved Hide resolved

vandyliu reviewed Jan 22, 2020

View reviewed changes

src/extension_utils/utils.ts Outdated Show resolved Hide resolved

vandyliu reviewed Jan 22, 2020

View reviewed changes

isadorasophia approved these changes Jan 22, 2020

View reviewed changes

andreamah added 2 commits January 21, 2020 17:13

incorporating more PR comments for clarity

528c123

resolved merge conflicts

104e40e

markAtMicrosoft approved these changes Jan 22, 2020

View reviewed changes

andreamah merged commit 33ad456 into dev Jan 22, 2020

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

Users/t anmah/simulation dependency checker fix #169

Users/t anmah/simulation dependency checker fix #169

Uh oh!

Conversation

andreamah commented Jan 21, 2020

Description:

Type of change

Limitations:

Testing:

Checklist:

Uh oh!

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!

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!

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!

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!

Uh oh!

isadorasophia Jan 22, 2020 •

edited

Loading