Replace `Makefile` with `justfile` #979

StevenMaude · 2023-08-10T18:16:22Z

And do an overhaul of the repository structure to use Ruff.

https://github.com/opensafely-core/repo-template/blob/e3d84fe3981d7f3df8f3cfa3c3a0a72e33cea5f2/justfile

StevenMaude · 2023-08-10T18:22:17Z

Issues to consider when reviewing this.

QA

Haven't tested end-to-end publication of the Docker image.

Refactoring the `just` recipes

The just recipes to install the dev and production requirements have some duplication; there might be a nicer way of passing the --extras=drivers arguments to pip-compile, and other extra arguments.
- You cannot pass --extras=drivers at the end, because its positional order matters.

Ruff

Have ignored a couple of existing issues that Ruff picks up by ignoring the rules in the Ruff configuration: mainly masking of Python built-ins. These are noted in ruff.toml. We could remove these extra ignore options.
It may be possible to move this configuration into a pyproject.toml, but having it separate seems OK for now.

Dependencies

Not for this PR, but we still have issues with updating the dependencies. So, only the dev requirements are rebuilt.
- not-from-a-package version is not PEP 440 compliant, breaking pip-compile #955 (this can be temporarily worked around by setting a valid version locally)
- Dependabot doesn't seem to ever update requirements.prod.txt #895, which is some conflicting version issue
We don't pin the dependency hashes here. I'm not sure why, but have left things as they are. We could change this if it makes sense to do so.

We typically do this now, although it's not in repo-template yet and this file was copied from there originally.

Not sure if there is a reason that this is not included in the existing files.

We can remove the `touch requirements.*.txt` workaround, because this is handled in the `justfile`.

As repo-template, except for specifying Python version: https://github.com/opensafely-core/repo-template/blob/e3d84fe3981d7f3df8f3cfa3c3a0a72e33cea5f2/pyproject.toml Let's keep this in its own file for now, as we are not yet using `pyproject.toml` in this project. This also uses a few extra ignore options than we would normally specify for new projects. We could remove these in future, but it seems less disruptive to leave the affected code as is for now.

There might be a better way, but not sure how you can pass the additional needed argument to `pip-compile`. We want: `--extras=drivers` for the production dependencies, but this fails with the dev dependencies.

And pin sqlalchemy to v1 v2 is incompatible until we change the cohort-extractor code.

This was copied over from the `Makefile`, but we need to amend it slightly to work with `just`.

madwort · 2023-08-17T16:49:57Z

justfile

+    export BUILD_DATE=$(shell date +'%y-%m-%dT%H:%M:%S.%3NZ')
+    export REVISION=$(shell git rev-parse --short HEAD)
+    # We retrieve the version in GitHub Actions differently,
+    # so allow overriding this value.
+    export VERSION=${VERSION:-$(shell git describe --tags)}
+    export DOCKER_BUILDKIT=1
+    docker-compose build --pull {{ args }} {{ env }}


I think you need to use #!/usr/bin/env bash for these env vars to be picked up by docker-compose - the just default is that each line is run in a separate shell, so they will be lost. e.g.

# justfile myenv: export MYENVVAR=yeahyeahyeah echo $MYENVVAR

% just myenv export MYENVVAR=yeahyeahyeah echo $MYENVVAR sh: MYENVVAR: unbound variable error: Recipe `myenv` failed on line 25 with exit code 1

madwort · 2023-08-17T16:54:56Z

justfile

@@ -0,0 +1,148 @@
+# just has no idiom for setting a default value for an environment variable
+# so we shell out, as we need VIRTUAL_ENV in the justfile environment
+export VIRTUAL_ENV  := `echo ${VIRTUAL_ENV:-.venv}`


can you use env_var_or_default() ? https://just.systems/man/en/chapter_31.html#environment-variables

Suggested change

export VIRTUAL_ENV := `echo ${VIRTUAL_ENV:-.venv}`

export VIRTUAL_ENV := env_var_or_default("VIRTUAL_ENV", ".venv")

export DEFVAR := env_var_or_default("DEFVAR", "Iamdefault") defvaraction: echo $DEFVAR

% just defvaraction echo $DEFVAR Iamdefault % DEFVAR=mydefvar just defvaraction echo $DEFVAR mydefvar

Maybe we should update it in the repo template?

https://github.com/opensafely-core/repo-template/blob/e3d84fe3981d7f3df8f3cfa3c3a0a72e33cea5f2/justfile#L3

we probably should...! I notice that was written two years ago - it's very possible that just did not have this feature when that comment was written...

(do you want to update the template, or should I?)

thinking about it - it would be good to land these changes here & test them before we port it upstream to the template?

madwort

I don't think just docker-build will work as expected

madwort · 2023-08-17T20:09:41Z

justfile

+_compile-dev src dst *args: virtualenv
+    #!/usr/bin/env bash
+    set -euo pipefail
+
+    # exit if src file is older than dst file (-nt = 'newer than', but we negate with || to avoid error exit code)
+    test "${FORCE:-}" = "true" -o {{ src }} -nt {{ dst }} || exit 0
+    $BIN/pip-compile --allow-unsafe --output-file={{ dst }} {{ src }} {{ args }}
+
+
+_compile-prod src dst *args: virtualenv
+    #!/usr/bin/env bash
+    set -euo pipefail
+
+    # exit if src file is older than dst file (-nt = 'newer than', but we negate with || to avoid error exit code)
+    test "${FORCE:-}" = "true" -o {{ src }} -nt {{ dst }} || exit 0
+    $BIN/pip-compile --allow-unsafe --extras=drivers --output-file={{ dst }} {{ src }} {{ args }}
+
+
+# update requirements.prod.txt if requirements.prod.in has changed
+requirements-prod *args:
+    "{{ just_executable() }}" _compile-prod setup.py requirements.prod.txt {{ args }}
+
+
+# update requirements.dev.txt if requirements.dev.in has changed
+requirements-dev *args: requirements-prod
+    "{{ just_executable() }}" _compile-dev requirements.dev.in requirements.dev.txt {{ args }}


I think we could condense this like so:

Suggested change

_compile-dev src dst *args: virtualenv

#!/usr/bin/env bash

set -euo pipefail

# exit if src file is older than dst file (-nt = 'newer than', but we negate with || to avoid error exit code)

test "${FORCE:-}" = "true" -o {{ src }} -nt {{ dst }} || exit 0

$BIN/pip-compile --allow-unsafe --output-file={{ dst }} {{ src }} {{ args }}

_compile-prod src dst *args: virtualenv

#!/usr/bin/env bash

set -euo pipefail

# exit if src file is older than dst file (-nt = 'newer than', but we negate with || to avoid error exit code)

test "${FORCE:-}" = "true" -o {{ src }} -nt {{ dst }} || exit 0

$BIN/pip-compile --allow-unsafe --extras=drivers --output-file={{ dst }} {{ src }} {{ args }}

# update requirements.prod.txt if requirements.prod.in has changed

requirements-prod *args:

"{{ just_executable() }}" _compile-prod setup.py requirements.prod.txt {{ args }}

# update requirements.dev.txt if requirements.dev.in has changed

requirements-dev *args: requirements-prod

"{{ just_executable() }}" _compile-dev requirements.dev.in requirements.dev.txt {{ args }}

_compile src dst env *args: virtualenv

#!/usr/bin/env bash

set -euo pipefail

# exit if src file is older than dst file (-nt = 'newer than', but we negate with || to avoid error exit code)

test "${FORCE:-}" = "true" -o {{ src }} -nt {{ dst }} || exit 0

$BIN/pip-compile --allow-unsafe {{ if env == "dev" { "--extras=drivers" } else { "" } }} --output-file={{ dst }} {{ src }} {{ args }}

# update requirements.prod.txt if requirements.prod.in has changed

requirements-prod *args:

"{{ just_executable() }}" _compile setup.py requirements.prod.txt prod {{ args }}

# update requirements.dev.txt if requirements.dev.in has changed

requirements-dev *args: requirements-prod

"{{ just_executable() }}" _compile requirements.dev.in requirements.dev.txt dev {{ args }}

Thanks! I wasn't sure what the idiomatic way of doing things is.

(Just's README is very long, and I didn't find it easy to find much in its documentation either. At least without reading both end-to-end, which I haven't done.)

tbh I'm still mostly figuring out what's possible in justfiles, more than what's idomatic or conventional. I found the just programmer's manual hit the right spot for me in terms of being able to grok some useful features.

actually now you mention it, I think we can make this probably more idiomatic by removing the use of just_executable() & making requirements-* only have dependencies, like this:

# update requirements.dev.txt if requirements.dev.in has changed requirements-dev *args: requirements-prod (_compile "requirements.dev.in" "requirements.dev.txt" "dev" args)

(not sure if this is much better or not, but kind of nice not to invoke a child just process...)

iaindillingham · 2023-08-18T09:34:05Z

justfile

+
+    # exit if src file is older than dst file (-nt = 'newer than', but we negate with || to avoid error exit code)
+    test "${FORCE:-}" = "true" -o {{ src }} -nt {{ dst }} || exit 0
+    $BIN/pip-compile --allow-unsafe --extras=drivers --output-file={{ dst }} {{ src }} {{ args }}


I think --extras should be --extra.

iaindillingham · 2023-08-18T09:38:40Z

justfile

+
+# clean up temporary files
+clean:
+    rm -rf .venv


I appreciate that this line is unchanged from repo-template, but I feel we should use the value of VIRTUAL_ENV (Line 3) rather than hard-code .venv. If we don't, then just clean may not remove the virtual env.

iaindillingham · 2023-08-18T09:46:58Z

justfile

@@ -0,0 +1,148 @@
+# just has no idiom for setting a default value for an environment variable
+# so we shell out, as we need VIRTUAL_ENV in the justfile environment
+export VIRTUAL_ENV  := `echo ${VIRTUAL_ENV:-.venv}`


If VIRTUAL_ENV isn't set, then the makefile creates the virtual env in venv but the justfile creates it in .venv. I prefer the latter, but it would be good to announce the change, as someone could inadvertently end up with two virtual envs; one that their muscle memory and setup work with, and one that the justfile works with.

A good way to make the change noisy -- and something we should do regardless -- would be to change .gitignore so that it ignored .venv but not venv.

iaindillingham · 2023-08-18T10:06:46Z

justfile

If I run just devenv in a new shell, then I see:

Backend subprocess exited when trying to invoke get_requires_for_build_wheel Failed to parse /Users/iaindillingham/Code/opensafely-core/cohort-extractor/setup.py error: Recipe `_compile-prod` failed with exit code 2 error: Recipe `requirements-prod` failed on line 58 with exit code 2

If I run just virtualenv, source .venv/bin/activate, and pip install ., then within the stack trace I see:

Version 'not-from-a-package' is not valid according to PEP 440.

Admittedly, I see the same when I run make devenv; but I think we should fix this issue before replacing the makefile with the justfile.

you're already seeing the entire output from pip-compile, as there's not a difference between make & just maybe it's a separate ticket?

cohort-extractor % ./.venv/bin/pip-compile --allow-unsafe --extra=drivers --output-file=requirements.prod.txt setup.py Backend subprocess exited when trying to invoke get_requires_for_build_wheel Failed to parse /Users/tom/Documents/bennett-institute/cohort-extractor/setup.py

True. I've made #980. It blocks my review, I'm afraid.

benbc · 2024-02-19T15:00:38Z

@StevenMaude @inglesp is it worth doing this given that Cohort Extractor is now deprecated? Or should we just close this PR?

StevenMaude added 2 commits August 10, 2023 17:37

Add current version of justfile from repo-template

bc621ed

https://github.com/opensafely-core/repo-template/blob/e3d84fe3981d7f3df8f3cfa3c3a0a72e33cea5f2/justfile

Remove Node.js related entries in justfile

e8c185a

StevenMaude marked this pull request as ready for review August 17, 2023 15:42

StevenMaude added 16 commits August 17, 2023 17:03

Don't use coverage as not installed

fb17faf

Move everything from Makefile into justfile

a109503

Use set -euo pipefail for scripts

a53daac

We typically do this now, although it's not in repo-template yet and this file was copied from there originally.

Remove generate-hashes for now

2dafc7d

Not sure if there is a reason that this is not included in the existing files.

Switch make to just in workflows

94a1074

We can remove the `touch requirements.*.txt` workaround, because this is handled in the `justfile`.

Update developer information to use just

39fc8b3

Run ruff --fix

d24acf2

Fix a remaining Ruff issue

bde8574

Switch .pre-commit-config to use justfile

2702d2b

Make just check work like in ehrQL repository

17b3ac9

Split dev and prod build into separate recipes

b132576

There might be a better way, but not sure how you can pass the additional needed argument to `pip-compile`. We want: `--extras=drivers` for the production dependencies, but this fails with the dev dependencies.

Replace flake8 and isort with ruff in requirements

42d5389

Rebuild dev requirements

1316e70

And pin sqlalchemy to v1 v2 is incompatible until we change the cohort-extractor code.

Rework Docker build

5d8cbc2

This was copied over from the `Makefile`, but we need to amend it slightly to work with `just`.

Use just recipes in GitHub workflows

aa47996

StevenMaude force-pushed the steve/replace-makefile-with-justfile branch from 8b5abe5 to aa47996 Compare August 17, 2023 16:16

madwort reviewed Aug 17, 2023

View reviewed changes

madwort requested changes Aug 17, 2023

View reviewed changes

madwort reviewed Aug 17, 2023

View reviewed changes

iaindillingham reviewed Aug 18, 2023

View reviewed changes

inglesp closed this Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `Makefile` with `justfile` #979

Replace `Makefile` with `justfile` #979

StevenMaude commented Aug 10, 2023

StevenMaude commented Aug 10, 2023 •

edited

Loading

madwort Aug 17, 2023

madwort Aug 17, 2023

madwort Aug 17, 2023

StevenMaude Aug 17, 2023

madwort Aug 18, 2023

madwort Aug 18, 2023

madwort Aug 18, 2023

madwort left a comment

madwort Aug 17, 2023 •

edited

Loading

StevenMaude Aug 17, 2023 •

edited

Loading

madwort Aug 18, 2023 •

edited

Loading

iaindillingham Aug 18, 2023

iaindillingham Aug 18, 2023 •

edited

Loading

iaindillingham Aug 18, 2023

iaindillingham Aug 18, 2023

madwort Aug 18, 2023 •

edited

Loading

iaindillingham Aug 18, 2023

benbc commented Feb 19, 2024

	export VIRTUAL_ENV := `echo ${VIRTUAL_ENV:-.venv}`
	export VIRTUAL_ENV := env_var_or_default("VIRTUAL_ENV", ".venv")

Replace Makefile with justfile #979

Replace Makefile with justfile #979

Conversation

StevenMaude commented Aug 10, 2023

StevenMaude commented Aug 10, 2023 • edited Loading

QA

Refactoring the just recipes

Ruff

Dependencies

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madwort left a comment

Choose a reason for hiding this comment

madwort Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

StevenMaude Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

madwort Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iaindillingham Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madwort Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benbc commented Feb 19, 2024

Replace `Makefile` with `justfile` #979

Replace `Makefile` with `justfile` #979

StevenMaude commented Aug 10, 2023 •

edited

Loading

Refactoring the `just` recipes

madwort Aug 17, 2023 •

edited

Loading

StevenMaude Aug 17, 2023 •

edited

Loading

madwort Aug 18, 2023 •

edited

Loading

iaindillingham Aug 18, 2023 •

edited

Loading

madwort Aug 18, 2023 •

edited

Loading