Support development using makefile #777

neiljp · 2020-08-16T21:46:21Z

This has been in the works since before 0.5.2, so here's a PR for wider review.

The main feature is how it makes setting up, maintaining and resetting a development venv even easier - simply run make and activate the venv. If setup.py changes, then make sets up the environment accordingly on the next run. This latter approach is adapted from a number of examples I've seen. If an install fails or gets too big, then it can just be wiped and made afresh.

I've also added and applied some explicit 'fixing' tools. We can iterate on these, or drop these in the first instance.

If we move to zulint in time, then we could still maintain this approach and swap-out linting/fixing commands.

I've updated some documentation, but there may be other parts I've missed.

kaustubh-nair

@neiljp Functionally, this looks good! I tested linting, testing, and changes in dependencies and they seem to work well.

Left some minor comments.

kaustubh-nair · 2020-08-17T12:05:05Z

README.md

+If using make with pip, you should also be able to use the following:
+* `make` - ensure the development environment is up to date with specified dependencies
+* `make lint` - equivalent to `tools/lint-all`
+* `make test` - equivalent to running `make lint` and pytest
+


We should add make pytest here too. I run the linters only after all my tests pass which could take a few iterations.

I wasn't sure how much to duplicate the table - some things don't run via make, and others are equivalent except for testing venv. Prefixing pytest with make each time felt like maybe too much. Happy to include it though.

'fix' should likely depend on venv too.

I am wary of make with pytest as well given that writing make pytest takes more time and hinders tab completion.

You're meaning for pytest options?

No, I meant in general. It is easier to do pyte + <TAB> than write make pytest.

To clarify, we can still run make pytest and it will work (because of the way its listed in makefile) . Are we considering here whether to only mention it README or not?

makefile

preetmishra

@neiljp Thanks for automating the dev setup. It would greatly help new contributors to set up the dev environment easily. 👍

I tested the PR locally and it seems to work well. However, I have a few concerns that I have left in-line.

preetmishra · 2020-08-17T18:32:00Z

makefile

@@ -0,0 +1,34 @@
+.PHONY: install-devel test lint pytest check-clean-tree fix force-fix venv
+
+# Customize your venv name by running make as "ZT_VENV=my_venv_name make <command>"


We should mention that the ZT_VENV=... would not only be required for the first time but every time it is run.

Additionally, perhaps rename <command> to <zt-command>?

Also, should we rather include this in the README so that it is easier to find?

I had specifically left these notes in the makefile for now as they seemed more 'advanced', and may be scope for further extension - they work, but for now it's easier to just have one environment with a standard name.

I'm not sure we gain from zt-command - what matters is running the different makefile 'targets', which are more just specific to those in the makefile.

preetmishra · 2020-08-17T18:50:11Z

tests/model/test_model.py

@@ -283,15 +283,15 @@ def test_set_narrow_not_already_set(self, model, initial_narrow, narrow,
            }
        }, {0, 1}),
        ([['stream', 'FOO'],
-         ['topic', 'BOO']], {
+          ['topic', 'BOO']], {


The fact that make fix made formatting changes in files which were already passing the linting tests is concerning. Do we have a plan to enforce make fix promptly?

Otherwise, it could potentially accumulate a bunch of unrelated changes in PRs which build on a master commit that didn't run make fix.

I believe the difference is that autopep8 fixes many pep8 rules as specified, but also some additional ones as here: https://pypi.org/project/autopep8/#features

We could add this to CI without --in-place and with --diff, as a linter. Also autoflake. Ideally this would be part of flake8, so we wouldn't need another checker :/

Ah, I see. :/ Nonetheless, we should perhaps enforce this with our CI for consistency.

preetmishra · 2020-08-17T18:54:14Z

makefile

+# Short name for file dependency
+venv: $(ZT_VENV)/bin/activate
+
+# If setup.py is updated or activate script doesn't exist, update virtual env


We should perhaps include the fact that the makefile will continue to make a new venv even if a custom venv exists (without the ZT_VENV=... command).

You mean if you don't use a custom environment name for all make invocations, it will build the standard one?

Yes, it seems to build the standard one without that.

On second thought, since the custom one is a bit awkward to run and manage, perhaps we should only promote the standard one.

neiljp · 2020-08-17T23:52:27Z

Just pushed with the .gitignore.

sumanthvrao

@neiljp As mentioned previously I think this will be very useful to ppl who want to get a quick ZT dev environment setup. Manually the PR works fine and the code changes look good as well. 👍

(As I side note, the docker PR I had open would end up being much simpler now as well.)

sumanthvrao · 2020-08-19T01:11:16Z

makefile

+
+# Customize your venv name by running make as "ZT_VENV=my_venv_name make <command>"
+
+BASEPYTHON?=python3


I think the only restriction this poses is being able to choose the exact python version during venv creation. This isn't a limitation on its own, but if we are going for an "advanced" setup option, we should consider this being customizable.

sumanthvrao · 2020-08-19T01:15:12Z

README.md

+If using make with pip, you should also be able to use the following:
+* `make` - ensure the development environment is up to date with specified dependencies
+* `make lint` - equivalent to `tools/lint-all`
+* `make test` - equivalent to running `make lint` and pytest
+


To clarify, we can still run make pytest and it will work (because of the way its listed in makefile) . Are we considering here whether to only mention it README or not?

Also upgrade flake8 plugins & relax pinned versions.

Documentation on new development installation approach added to README. Default venv directory added to .gitignore.

Note that the README is not updated to document `make fix` as yet, as this is considered an experimental development feature.

neiljp · 2020-08-20T17:54:59Z

Following discussion on czo:

targets renamed: test -> check, pytest -> test
note added to makefile indicating ZT_VENV and BASEPYTHON are advanced undocumented features
make fix documentation removed from README for now
make documentation simplified to just indicate it's useful for keeping environment up to date and when
documentation added to indicate when make check is useful

Also fixed .phony targets for 'fix', which are moved to the correct commit.

Does this address the outstanding feedback?

kaustubh-nair

@neiljp This looks good to me!

sumanthvrao · 2020-08-21T07:09:31Z

@neiljp Thanks for updating this. Looks good to me!

neiljp · 2020-08-21T07:11:20Z

Thanks for the reviews everyone! 🎉

zulipbot added the size: L [Automatic label added by zulipbot] label Aug 16, 2020

neiljp added the feedback wanted label Aug 16, 2020

kaustubh-nair reviewed Aug 17, 2020

View reviewed changes

preetmishra reviewed Aug 17, 2020

View reviewed changes

neiljp force-pushed the 2020-05-26-makefile branch from 4d82097 to 0b2c405 Compare August 17, 2020 23:45

sumanthvrao reviewed Aug 19, 2020

View reviewed changes

requirements: Upgrade from flake8 ==3.7.9 to ~=3.8.3.

cfdda12

Also upgrade flake8 plugins & relax pinned versions.

neiljp force-pushed the 2020-05-26-makefile branch from 0b2c405 to 52ad22d Compare August 19, 2020 04:15

neiljp added 2 commits August 20, 2020 10:43

tools/README/gitignore: Add makefile to set up venv & run tests.

a2f48ee

Documentation on new development installation approach added to README. Default venv directory added to .gitignore.

requirements: Add development dependencies on autopep8 & autoflake.

c150641

neiljp force-pushed the 2020-05-26-makefile branch from 52ad22d to 90c3d3e Compare August 20, 2020 17:48

neiljp added 2 commits August 20, 2020 10:49

makefile: Add targets to fix code layout using installed tools.

0a722c6

Note that the README is not updated to document `make fix` as yet, as this is considered an experimental development feature.

boxes/buttons/views/test_model: Minor formatting changes from make fix.

68b6c4c

neiljp force-pushed the 2020-05-26-makefile branch from 90c3d3e to 68b6c4c Compare August 20, 2020 17:50

kaustubh-nair approved these changes Aug 20, 2020

View reviewed changes

neiljp mentioned this pull request Aug 20, 2020

TRACKING: Installation methods for users & developers #291

Open

neiljp merged commit 0b20905 into zulip:master Aug 21, 2020

neiljp added this to the Next Release milestone Aug 21, 2020

		@@ -0,0 +1,34 @@
		.PHONY: install-devel test lint pytest check-clean-tree fix force-fix venv

		# Customize your venv name by running make as "ZT_VENV=my_venv_name make <command>"


		# Customize your venv name by running make as "ZT_VENV=my_venv_name make <command>"

		BASEPYTHON?=python3

Uh oh!

Support development using makefile #777

Support development using makefile #777

Uh oh!

Conversation

neiljp commented Aug 16, 2020

Uh oh!

kaustubh-nair left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

preetmishra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

preetmishra Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Aug 17, 2020

Uh oh!

sumanthvrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neiljp commented Aug 20, 2020

Uh oh!

kaustubh-nair left a comment

Choose a reason for hiding this comment

Uh oh!

sumanthvrao commented Aug 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neiljp commented Aug 21, 2020

Uh oh!

Uh oh!

preetmishra Aug 17, 2020 •

edited

Loading

sumanthvrao commented Aug 21, 2020 •

edited

Loading