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.

Sign up for GitHub

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

Jump to bottom

chore: tidy up makefile (DEV-1166) #223

Merged
merged 17 commits into from Sep 8, 2022
Merged

chore: tidy up makefile (DEV-1166) #223

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fe64c19
edit
jnussbaum Aug 31, 2022
648ca4c
edit
jnussbaum Sep 6, 2022
c172927
Merge branch 'main' into wip/dev-1166-improve-makefile
jnussbaum Sep 6, 2022
6fa2191
edit
jnussbaum Sep 6, 2022
73e295c
edit
jnussbaum Sep 6, 2022
1d05f13
make freeze-requirements
jnussbaum Sep 6, 2022
defea0e
remove mkdocstrings
jnussbaum Sep 6, 2022
4377ce1
apply reviewer's feedback
jnussbaum Sep 6, 2022
7663c6e
pipenv uninstall rdflib validators click pyparsing
jnussbaum Sep 7, 2022
4c0b6ad
add pyparsing to dev-requirements.txt
jnussbaum Sep 7, 2022
8d4fad6
pipenv uninstall xlrd
jnussbaum Sep 7, 2022
17c0932
pipenv install xlrd
jnussbaum Sep 7, 2022
027acd0
pipenv uninstall rfc3987
jnussbaum Sep 7, 2022
145c426
give permission 777 to .tmp
jnussbaum Sep 7, 2022
44e0432
improve rm command for .tmp
jnussbaum Sep 7, 2022
67cceee
add test-end-to-end-ci
jnussbaum Sep 8, 2022
ebba80b
fix
jnussbaum Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion .github/workflows/daily-test.yml
View file
Open in desktop
Expand Up @@ -19,7 +19,6 @@ jobs:
python-version: 3.9
- name: Install dependencies
run: |
make upgrade-dist-tools
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I deleted this command. It is better to include the dist-tools as dev-dependencies (in the dev-section of Pipfile). This way, they get installed when make install-requirements is executed. We don't need a separate command just to install the dist-tools.

make install-requirements
make install
- name: Run e2e tests
Expand Down
3 changes: 1 addition & 2 deletions .github/workflows/release.yml
View file
Open in desktop
Expand Up @@ -29,10 +29,9 @@ jobs:
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install -r requirements.txt
make install-requirements
# release new version to PyPI
- run: |
make upgrade-dist-tools
make dist
make upload

Expand Down
1 change: 0 additions & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Expand Up @@ -23,7 +23,6 @@ jobs:
python-version: 3.9
- name: Install dependencies
run: |
make upgrade-dist-tools
make install-requirements
make install
- name: Run tests
Expand Down
64 changes: 28 additions & 36 deletions Makefile
View file
Open in desktop
Expand Up @@ -3,21 +3,23 @@
THIS_FILE := $(lastword $(MAKEFILE_LIST))
CURRENT_DIR := $(shell dirname $(realpath $(firstword $(MAKEFILE_LIST))))

#################################
############################
# Make targets for dsp-tools
#################################

.PHONY: clone-dsp-repo
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't need an extra command to clone the DSP-API. I included this in the dsp-stack command.

clone-dsp-repo: ## clone the dsp-api git repository
@git clone --branch main --single-branch --depth 1 https://github.com/dasch-swiss/dsp-api.git $(CURRENT_DIR)/.tmp/dsp-stack
############################

.PHONY: dsp-stack
dsp-stack: ## run the dsp-stack (deletes existing volumes first)
$(MAKE) -C $(CURRENT_DIR)/.tmp/dsp-stack env-file
$(MAKE) -C $(CURRENT_DIR)/.tmp/dsp-stack stack-down-delete-volumes
$(MAKE) -C $(CURRENT_DIR)/.tmp/dsp-stack init-db-test
$(MAKE) -C $(CURRENT_DIR)/.tmp/dsp-stack stack-up
$(MAKE) -C $(CURRENT_DIR)/.tmp/dsp-stack stack-logs-api-no-follow
dsp-stack: ## clone the dsp-api git repository and run the dsp-stack
@mkdir -p .tmp
@git clone --branch main --single-branch --depth 1 https://github.com/dasch-swiss/dsp-api.git .tmp/dsp-stack
$(MAKE) -C .tmp/dsp-stack env-file
$(MAKE) -C .tmp/dsp-stack init-db-test
$(MAKE) -C .tmp/dsp-stack stack-up
$(MAKE) -C .tmp/dsp-stack stack-logs-api-no-follow

.PHONY: stack-down
stack-down: ## stop dsp-stack and remove the cloned dsp-api repository
$(MAKE) -C .tmp/dsp-stack stack-down-delete-volumes
@sudo rm -rf .tmp
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For developer's machines, we need a command to stop the dsp-stack, otherwise it continues running for a long time, consuming resources.
The command to remove the .tmp folder needs a sudo because otherwise, it produces an error on GitHub (this error here: https://github.com/dasch-swiss/dsp-tools/runs/8204985409?check_suite_focus=true). The sudo wouln't be necessary for local machines, but for GitHub, it is necessary. If there are better solutions than sudo, I'm open for correction.

Copy link

Choose a reason for hiding this comment

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

Is it necessary to remove the tmp folder? Why not just leave it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

make dsp-stack is the only place in the entire repo where the tmp folder is created. So it makes sense to me to delete it in the stack-down command, instead of the make clean command.


.PHONY: dist
dist: ## generate distribution package
Expand All @@ -27,21 +29,13 @@ dist: ## generate distribution package
upload: ## upload distribution package to PyPI
python3 -m twine upload dist/*

.PHONY: upgrade-dist-tools
upgrade-dist-tools: ## upgrade packages necessary for testing, building, packaging and uploading to PyPI
python3 -m pip install --upgrade pip setuptools wheel twine pytest mkdocs

.PHONY: docs-build
docs-build: ## build docs into the local 'site' folder
mkdocs build

.PHONY: docs-serve
docs-serve: ## serve docs for local viewing
mkdocs serve

.PHONY: docs-publish
docs-publish: ## build and publish docs to GitHub Pages
mkdocs gh-deploy
mkdocs serve --dev-addr=0.0.0.0:7979
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • docs-publish is not used anymore, because the docs are only on docs.dasch.swiss.
  • for mkdocs serve, I need another port, because the standard port is occupied on my machine.


.PHONY: install-requirements
install-requirements: ## install requirements
Expand All @@ -56,37 +50,35 @@ install: ## install from source (runs setup.py)
pip3 install -e .

.PHONY: test
test: clean local-tmp clone-dsp-repo dsp-stack ## run all tests
test: dsp-stack ## run all tests
pytest test/
$(MAKE) stack-down

.PHONY: test-no-stack
test-no-stack: ## run tests without starting the stack (if a dsp-stack is already running)
pytest test/

.PHONY: test-end-to-end
test-end-to-end: clean local-tmp clone-dsp-repo dsp-stack ## run e2e tests
test-end-to-end: dsp-stack ## run e2e tests
pytest test/e2e/
$(MAKE) stack-down

.PHONY: test-end-to-end-no-stack
test-end-to-end-no-stack: ## run e2e tests without starting the dsp-stack (if a dsp-stack is already running)
pytest test/e2e/

.PHONY: test-unittests
test-unittests: ## run unit tests
pytest test/unittests/

.PHONY: local-tmp
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now included at the only place where it is necessary: before cloning DSP-API.

local-tmp: ## create local .tmp folder
@mkdir -p $(CURRENT_DIR)/.tmp

.PHONY: clean
clean: ## clean local project directories
@rm -rf $(CURRENT_DIR)/.tmp
jnussbaum marked this conversation as resolved.
Show resolved Hide resolved
@rm -rf dist/ build/ site/ knora.egg-info/
@rm -rf dist/ build/ site/ dsp_tools.egg-info/

.PHONY: help
help: ## show this help
@awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' $(MAKEFILE_LIST) | sort

.PHONY: run
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This command is not necessary, IMHO.

run: ## create dist, install and run
$(MAKE) clean
$(MAKE) dist
$(MAKE) install
dsp-tools

.PHONY: freeze-requirements
freeze-requirements: ## update (dev-)requirements.txt and setup.py based on pipenv's Pipfile.lock
pipenv requirements > requirements.txt
Expand Down
2 changes: 2 additions & 0 deletions Pipfile
View file
Open in desktop
Expand Up @@ -32,6 +32,8 @@ wheel = "*"
pipenv-setup = "*"
pytest = "*"
types-requests = "*"
twine = "*"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two dependencies were "hidden" until now in docs/requirements.txt and in make upgrade-dist-tools. I think they belong here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we use mkdocstrins at all - it's a Mkdocs plugin that generates documentation based on the docstrings in the source code... and we don't have that, as far as I know. So probably this could be removed, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed it, and GitHub CI runs through without errors. So I assume we really don't need it.

mkdocstrings = "*"

[requires]
python_version = "3"