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(api): Make the Makefile have more real prerequisites/targets #2510

Merged
merged 5 commits into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 47 additions & 21 deletions api/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,15 @@
SHELL := /bin/bash

# add yarn CLI dev deps to PATH (for cross platform POSIX commands via shx)
PATH := $(shell cd .. && yarn bin):$(PATH)
# and also make an explicit version for shx for use in the shell function,
# where PATH won’t be propagated
yarn_path := $(shell cd .. && yarn bin)
PATH := ${yarn_path}:${PATH}
# Make an explicit version of this for shx so it can be used in the $(shell)
# function, where the PATH won’t be propagated
SHX := ${yarn_path}/shx
Copy link
Contributor

Choose a reason for hiding this comment

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

This forward slash appears to be working on Windows 10, but makes me a little nervous. For node_modules/.bin dependencies in $(shell ...) this also works:

Suggested change
SHX := ${yarn_path}/shx
SHX := npx shx


# make push wheel file (= rather than := to expand at every use)
wheel = $(wildcard dist/*.whl)
firmware = $(wildcard smoothie/*.hex)

# python and pipenv config
Expand All @@ -16,9 +21,34 @@ pytest := pipenv run py.test
pipenv_opts := --dev
pipenv_opts += $(and $(CI),--keep-outdated)

# Find the version of the wheel from package.json using a helper script. We
# use python here so we can use the same version normalization that will be
# used to create the wheel.
norm_vers := $(shell ${python} normalized_version.py)
WHEEL_FILE := dist/opentrons-${norm_vers}-py2.py3-none-any.whl
Copy link
Contributor

Choose a reason for hiding this comment

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

We haven't been super consistent about this in the past, but what is the meaning on the all caps and lowercase variable names? Something I've been trying to move to is what's recomended in this style guide:

  • Variables inherited from env defined in all-caps and defined with ?=
  • Internal variables in lowercase, usually defined with :=


# These variables can be overriden when make is invoked to customize the
# behavior of pytest. For instance,
# make test tests=tests/opentrons/tools/test_qc_scripts.py would run only the
# specified test
tests ?= tests
test_opts ?=

# Source discovery
# For the python sources
OT_PY_SOURCES := $(filter %.py,$(shell ${SHX} find src/opentrons/))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick and not a hill I'm trying to die on, but the GNU make manual recommends sticking to one one type variable delimiter for all references. I get there's probably a readability argument in the other direction so 🤷‍♂️

Also, we appear to be using a mixture in this file, so outside of nested vars in function calls, it would be nice to pick a convention and stick with it

# And the out of tree shared data
OT_SHARED_DATA_SOURCES := $(filter %.json,$(shell ${SHX} find ../shared-data/))
# And the arbitrary stuff in resources
OT_RESOURCES := $(filter %,$(shell ${SHX} find src/opentrons/resources))
OT_SOURCES := ${OT_PY_SOURCES} ${OT_SHARED_DATA_SOURCES} ${OT_RESOURCES}
# And the tests
OT_TEST_SOURCES := $(filter-out data/configs% configs/%.json,$(shell %{SHX} find tests/))
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: should be ${SHX}


# Defined separately than the clean target so the wheel file doesn’t have to
# depend on a PHONY target
CLEAN_CMD = shx rm -rf build dist .coverage coverage.xml '*.egg-info' '**/__pycache__' '**/*.pyc'

.PHONY: install
install:
pipenv sync $(pipenv_opts)
Expand All @@ -28,23 +58,25 @@ all: lint test

.PHONY: clean
clean:
shx rm -rf \
build \
dist \
.coverage \
coverage.xml \
'*.egg-info' \
'**/__pycache__' \
'**/*.pyc'
${CLEAN_CMD}

${WHEEL_FILE}: setup.py ${OT_SOURCES}
${CLEAN_CMD}
$(python) setup.py bdist_wheel
shx rm -rf build
shx ls dist

.PHONY: wheel
wheel: ${WHEEL_FILE}

.PHONY: test
test: local-install
test: local-install ${OT_TEST_SOURCES}
${pytest} ${tests} ${test_opts}

.PHONY: lint
lint:
lint: ${OT_PY_SOURCES}
$(python) -m mypy src/opentrons
$(python) -m pylama src/opentrons tests
$(python) -m pylama $? src/opentrons tests

.PHONY: docs
docs:
Expand All @@ -62,12 +94,6 @@ dev: export ENABLE_VIRTUAL_SMOOTHIE := true
dev: local-install
$(python) -m opentrons.main -P 31950

.PHONY: wheel
wheel: clean
$(python) setup.py bdist_wheel
shx rm -rf build
shx ls dist

.PHONY: local-install
local-install: wheel
$(pip) install --ignore-installed --no-deps dist/opentrons-*.whl
Expand All @@ -79,8 +105,8 @@ local-shell: local-install
.PHONY: push
push: wheel
curl -X POST \
-H "Content-Type: multipart/form-data" \
-F "whl=@$(wheel)" \
-H "Conte<nt-Type: multipart/form-data" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo?

-F "whl=@${WHEEL_FILE}" \
http://$(host):31950/server/update

.PHONY: flash
Expand Down
9 changes: 9 additions & 0 deletions api/normalized_version.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import json
import os
from setuptools.extern import packaging

pkg_json_path = os.path.join('src', 'opentrons', 'package.json')
old_ver = json.load(open(pkg_json_path))['version']
vers_obj = packaging.version.Version(old_ver)

print(str(vers_obj))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this print statement supposed to be here?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, the $(shell) function has its "return value" whatever the thing it invokes prints out. that's how we pass the information up to make.