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

Convert Makefile to use Python's Invoke project #79

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

blag
Copy link
Contributor

@blag blag commented Sep 21, 2019

This PR converts almost all of the targets in the Makefile into Invoke tasks. The Makefile is kept and vastly simplified into just setting up the environment for invoke, then passing all targets to it. Invoke handles everything else.

This PR isn't strictly 100% necessary, but since the Makefile was fairly simple and straightforward it was a good target to illustrate how a Makefile can be broken up into multiple different modules and run with Invoke with full reverse compatibility for any scripts that still call make. Some of the scripts in this repo do call make with this Makefile, and those were intentionally left alone as a sentinel for reverse compatibility. This PR is a good baby step towards replacing the (very complex/convoluted) Makefile in st2, since this Makefile was 100% composed of targets marked with .PHONY, so it was just a centralized way to run scripts and didn't actually build anything or cache any build artifacts, which is the entire purpose of make to begin with.

I also simplified some of the behavior in .circle/dependencies, for the sake of sanity:

  1. The requirements and requirements-ci targets both had the exact same recipe (verified with diff), so I also changed the call to make requirements-ci to just make requirements.
  2. There was a repeated pattern in the Makefile:
    .PHONY:
    install-runners: .install-runners  # non-dotted target is just redirected to the dotted target
    
    .PHONY:
    .install-runners:  # the dotted target actually did the work
        # actually do stuff here
    and usually the dotted target was not called directly. However, there were a few direct calls to the .install-runners (dotted) target, so I replaced them to just call the install-runners (non-dotted) target.

I explicitly relatively import the task modules since implicit relative imports have been phased out in Python 3. Do note that in tasks/utils.py, there is still an import git - this is not an implicit relative import of a tasks module, it is an absolute import for the GitPython module in the virtualenv.

Tested with the AWS pack:

Please review this thoroughly, since this is used for all pack CI runs, including the 130+ runs over the weekend.

tasks/check.py Outdated Show resolved Hide resolved
tasks/tests.py Outdated Show resolved Hide resolved
.circle/Makefile Show resolved Hide resolved
@Kami
Copy link
Contributor

Kami commented Sep 23, 2019

I'm fine with that change. Definitely much better than convoluted Makefile.

As mentioned in my other PR, it perhaps wouldn't be a bad idea to document some of that behavior, etc. since in theory, users could also use this Makefile for CI for their custom packs.

@nmaludy
Copy link
Contributor

nmaludy commented May 19, 2020

I'm good with this too! Much easier to debug than Make.

+1

Copy link
Contributor

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

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

Need to fix merge conflicts, otherwise this looks great!

@CLAassistant
Copy link

CLAassistant commented May 11, 2022

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