chore: Some Makefile fixes#18319
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## edge #18319 +/- ##
==========================================
+ Coverage 24.69% 25.72% +1.03%
==========================================
Files 3220 3207 -13
Lines 272857 272342 -515
Branches 25998 26003 +5
==========================================
+ Hits 67370 70054 +2684
+ Misses 205462 202262 -3200
- Partials 25 26 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| $(OT_PYTHON) -m pip install --upgrade pip | ||
| $(OT_PYTHON) -m pip install pipenv==2023.12.1 | ||
| # this needs to be installed AFTER pipenv or pipenv will update this to the bad version | ||
| # this needs to be installed AFTER pipenv or pipenv will update this to the bad version |
There was a problem hiding this comment.
This is a tabs vs. spaces thing. In Makefiles, tabs (the \t character) are semantic and not equivalent to 4 spaces. This line had 4 spaces in front of it. It seems like things were working, but it made me nervous, so this "fixes" it.
This entirely removes the leading whitespaces to the comment is parsed as a Makefile comment, instead of being passed to the shell and the shell evaluating it as a comment and printing it out.
| .PHONY: setup-js | ||
| setup-js: | ||
| setup-js: setup-py-toolchain | ||
| setup-js: setup-py-toolchain $(addsuffix -py-setup, $(USB_BRIDGE_DIR)) |
There was a problem hiding this comment.
make setup-js seems to depend on make -C usb-bridge happening beforehand. Otherwise it fails with ENOENT.
The presentation of the error is a little weird and it makes me think that something else is going on here, but I didn't investigate further.
There was a problem hiding this comment.
Uh hm, this "new" dependency on Python is making CI fail. Which means CI has never needed make -C usb-bridge to come before make setup-js. Which means, yeah, something else is going on here and I need to investigate it further.
Reverting for now.
| .PHONY: teardown | ||
| teardown: | ||
| $(pipenv) --rm | ||
| -$(pipenv) --rm |
There was a problem hiding this comment.
Each Python project's teardown target would fail if the project didn't have a virtualenv, e.g. if the project was already torn down or if it was never set up to begin with. This tolerates those errors so they don't bubble up and cause a top-level make teardown-py to abort prematurely.
| # todo(mm, 2025-05-12): Evaluating $(wheel_file) here means we'll call pipenv and create | ||
| # a virtualenv whenever this Makefile is read--even if you're just doing something | ||
| # like `make teardown`. |
There was a problem hiding this comment.
No idea what to do about this. api handles it by not depending on $(wheel_file), which seems gross in another way.
There was a problem hiding this comment.
if you make wheel_file lazily evalulated with = instead of := that should fix it
There was a problem hiding this comment.
Good thought, but it turns out it already is defined with =. Apparently, make evaluates the dependency list eagerly, which undoes that.
This reverts commit 4b4b578.
|
Failing |
sfoster1
left a comment
sfoster1
left a comment
There was a problem hiding this comment.
Looks good, I think the wheel-file thing ca be fixed with lazy evaluation
| # todo(mm, 2025-05-12): Evaluating $(wheel_file) here means we'll call pipenv and create | ||
| # a virtualenv whenever this Makefile is read--even if you're just doing something | ||
| # like `make teardown`. |
There was a problem hiding this comment.
if you make wheel_file lazily evalulated with = instead of := that should fix it
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
(cherry picked from commit 40fb85e)
2 participants
Overview
Some fixes to our build system.
Test Plan and Hands on Testing
make setupworksmake teardownworksChangelog
See comments below.
Risk assessment
Low.