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

Add makefile with common commands #23

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sahcim
Copy link
Collaborator

@Sahcim Sahcim commented Jul 29, 2024

Changelog:

  • Fixes check-licences command (previous one didn't work IMO)
  • Replaces bump version, build docs, venv init and check licenses with makefile commands
  • Adjusts Github/Gitlab pipelines

TODO:

  • Test github pipeline locally with act
  • Test gitlab pipeline with gitlab-ci-local
  • Adjust docs (if applicable)

@Sahcim Sahcim marked this pull request as draft July 29, 2024 11:13

.DEFAULT_GOAL := venv/bin/activate

venv/bin/activate: requirements-dev.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe just venv?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not how makefile works, "functions" below check for existance of venv/bin/activate file if it doesn't exist then makefile will run this "function"

Comment on lines 22 to +24

- name: Cache pre-commit
uses: actions/cache@v3
with:
path: ~/.cache/pre-commit
key: pre-commit-3|${{ env.pythonLocation }}|${{ hashFiles('.pre-commit-config.yaml') }}

- name: Install pre-commit
run: pip3 install pre-commit
- name: Install venv and pre-commit
run: make
Copy link
Collaborator

Choose a reason for hiding this comment

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

i guess we shouldn't be so brutal :D we want to use cached version to make this step much quicker and avoid setting up venv from scratch

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that we can't run make functions without creating a virtual environment and sourcing it first. Because of this, I'm not sure if it's worth continuing to work on the Makefile. Since we can't use it everywhere, it would require maintaining code in two places—both the Makefile and the ci.yml.

venv/bin/activate: requirements-dev.txt
python$(PYTHON_VERSION) -m venv venv
. venv/bin/activate
pip install --upgrade pip
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am getting following output:

~/Documents/Projects/ds-template/{{ cookiecutter.repo_name }}$ make 
python3.11 -m venv venv
. venv/bin/activate
pip install --upgrade pip
make: pip: No such file or directory
make: *** [Makefile:9: venv/bin/activate] Error 127

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Does the error still occur if you run make after generating the test project? Can you check if pip is configured correctly on your machine?


.DEFAULT_GOAL := venv/bin/activate

venv/bin/activate: requirements-dev.txt
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need this requirements-dev.txt here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makefile will check if requirments file changed, if yes then venv will be rebuild :)

- . setup_dev_env.sh
- pip install build twine bump2version
- make
- pip install build twine bump2version #???
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it may be removed indeed

Comment on lines +98 to +99
before_script:
- make
Copy link
Collaborator

Choose a reason for hiding this comment

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

as we use $PRECOMMIT_IMAGE, it is not necessary to setup venv before

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