Skip to content
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

fix: ensure that pipenv is installed. #14469

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

strangemonad
Copy link
Contributor

Overview

The current "out-of-the-box" dev experience fails in the following way

  • make setup first invokes
  • make setup-js before it runs make setup-py
  • setup-js invokes a sub-make for the app shell which requires pipenv already setup to build the python protocol analysis sandbox.

This is fine if you've already run things before and have pipenv installed locally but it means the standard experience following the dev guides fails.

This pulls out the minimal python setup as a dependency of both the setup-py and setup-js build targets

Test Plan

Manually tested from a clean repo and then re-ran setup in a repo that was already setup.

Changelog

  • Fixed build for freshly cloned repos when pipenv isn't available on the system

Risk assessment

n/a

The current "out-of-the-box" dev experience fails in the following way
- `make setup` first invokes
- `make setup-js` before it runs `make setup-py`
- setup-js invokes a sub-make for the app shell which requires pipenv already setup to build the python protocol analysis sandbox.

This is fine if you've already run things before and have pipenv installed locally but it means the standard experience following the dev guides fails.

This pulls out the minimal python setup as a dependency of both the setup-py and setup-js build targets
@strangemonad strangemonad requested a review from a team as a code owner February 11, 2024 20:32
# Both the python and JS setup targets depend on a minimal python setup so they can create
# virtual envs using pipenv.
.PHONY: setup-py-toolchain
setup-py-toolchain:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open to any naming suggestions here.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me, thank you for the pull!

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f463d55) 67.79% compared to head (b061f4e) 67.72%.
Report is 1 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14469      +/-   ##
==========================================
- Coverage   67.79%   67.72%   -0.07%     
==========================================
  Files        2518     1628     -890     
  Lines       72017    54953   -17064     
  Branches     9244     4147    -5097     
==========================================
- Hits        48823    37219   -11604     
+ Misses      20990    17043    -3947     
+ Partials     2204      691    -1513     
Flag Coverage Δ
app 33.87% <ø> (-30.71%) ⬇️
components 49.41% <ø> (ø)
g-code-testing 92.43% <ø> (ø)
hardware 57.54% <ø> (ø)
hardware-testing ∅ <ø> (∅)
shared-data 75.26% <ø> (ø)
system-server 96.04% <ø> (ø)
update-server 50.56% <ø> (ø)
usb-bridge 76.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 892 files with indirect coverage changes

@sfoster1 sfoster1 merged commit bb32772 into Opentrons:edge Feb 12, 2024
46 of 50 checks passed
andySigler pushed a commit that referenced this pull request Feb 15, 2024
The current "out-of-the-box" dev experience fails in the following way
- `make setup` first invokes
- `make setup-js` before it runs `make setup-py`
- setup-js invokes a sub-make for the app shell which requires pipenv already setup to build the python protocol analysis sandbox.

This is fine if you've already run things before and have pipenv installed locally but it means the standard experience following the dev guides fails.

This pulls out the minimal python setup as a dependency of both the setup-py and setup-js build targets
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
The current "out-of-the-box" dev experience fails in the following way
- `make setup` first invokes
- `make setup-js` before it runs `make setup-py`
- setup-js invokes a sub-make for the app shell which requires pipenv already setup to build the python protocol analysis sandbox.

This is fine if you've already run things before and have pipenv installed locally but it means the standard experience following the dev guides fails.

This pulls out the minimal python setup as a dependency of both the setup-py and setup-js build targets
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
The current "out-of-the-box" dev experience fails in the following way
- `make setup` first invokes
- `make setup-js` before it runs `make setup-py`
- setup-js invokes a sub-make for the app shell which requires pipenv already setup to build the python protocol analysis sandbox.

This is fine if you've already run things before and have pipenv installed locally but it means the standard experience following the dev guides fails.

This pulls out the minimal python setup as a dependency of both the setup-py and setup-js build targets
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants