Skip to content

Conversation

@blag
Copy link
Contributor

@blag blag commented Aug 29, 2019

The make tool is really great when it's building artifacts, but when it doesn't have artifacts, it's the wrong tool for the job.

We use .PHONY for almost every single make target in the root Makefile, which just turns it into a glorified centralized task runner. It's a good idea to have a centralized task runner, but it doesn't really make sense to use make for this task when there are better tools available.

A great example to illustrate how bad make is at running tasks: coverage. There are multiple ways to run tests with code coverage in our Makefile, and they very quickly get overcomplicated.

Look at this graph:

out

Now, we control a lot of this with environment variables that are passed to make, but this is a kludge at best (and I say that as the author of a lot of that).

Instead of having multiple make targets, or have make targets that behave drastically differently depending on environment variables, we should just have a task that runs tests, and have that task accept options that enable/disable coverage for "product code", enable/disable coverage for test code, or pass through options for Python's nose test runner.

Python's invoke library does this, and it does this well. Instead of having one monolithic root Makefile, the tasks (which are just Python code) can be broken up into their own modules within the tasks/ package/directory.

While the invoke executable is also installable as a Linux package (python-invoke and python3-invoke) on Ubuntu 16.04 (xenial) and later, I don't think this is the case for CentOS 7 and later, so it will need to be added as a test dependency/requirement.

I reimplemented all Makefile targets using invoke, pulled in some of the tools in tools/ and scripts/, and then added a compatibility "shim" into tasks/__init__.py for reverse compatibility with the Makefile.

The size of the Makefile has also been minimized. It now only sets up the root virtualenv, installs invoke, and passes all make targets on to invoke.

Finally, it is now possible to list all of the invoke tasks:

invoke --list

Please give me your feedback.

See:

@blag blag added this to the 3.2.0 milestone Aug 30, 2019
@blag blag force-pushed the use-invoke branch 13 times, most recently from c09d726 to ae96ea9 Compare September 4, 2019 22:07
@blag blag force-pushed the use-invoke branch 3 times, most recently from 7c350f6 to e239d19 Compare September 9, 2019 19:24
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

I'm wondering how removing 1K lines of code and adding 2K is considered useful and is simplification by the purpose?

I don't think there is a problem in Makefile and replacement it with Invoke we're trying to solve the wrong problem here which is initially a monstrous amount of logic we put in a Makefile. We should simplify and cut the build logic itself, as trying to replace tools and move code from one tool to another won't get us rid of that horrible amount of logic.

Add here the fact that Makefile logic is more easily understood/modified/adopted by other contributors comparing to Invoke, the tooling is widely available and there are other repositories relying on st2 Makefile.

Having said that, I'd like to postpone this change for st2 repo.
I'm however curious how Invoke adoption would work for other repositories.

@arm4b arm4b removed this from the 3.3.0 milestone Aug 11, 2020
@CLAassistant
Copy link

CLAassistant commented Sep 6, 2021

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants