Skip to content

build: Improve the Makefile #1381

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

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

theofidry
Copy link
Contributor

@theofidry theofidry commented Jun 24, 2024

Add various changes to the Makefile:

  • Add safer default MAKEFLAGS
  • Add a build command to rebuild the entire PHAR regardless of whether one already exists and is up to date
  • Add a help command which can be used to display the most important commands to help out new contributors.
  • Leverage the Makefile in the CI to avoid duplicating code.
  • Various other tiny changes detailed in the diff

Copy link
Contributor Author

@theofidry theofidry left a comment

Choose a reason for hiding this comment

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

This PR proposes various changes, feel free to point out the elements you don't like. I can also break it up more if you prefer, I didn't want to do that initially because as a reviewer this would also make it a bit hard to see where this is going and the feedback loop is quite slow due to our different availabilities and timezone differences. So having a slightly bigger PR makes it a bit easier to collect all the necessary feedback IMO

@@ -31,9 +31,12 @@ jobs:
- name: Install Box
run: composer global require --dev humbug/box

- name: Ensure the dependencies are up to date
run: make vendor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is purely to avoid noise. It can be that the vendor install does not have the right timestamp in which case calling make build will trigger another composer install. It is not a big deal, but it is noisy, so extracting this in a dedicate step helps to not pollute the important step bellow.

TARGET = phpbrew
SIGNATURE = $(TARGET).asc
# See https://tech.davis-hansson.com/p/make/
MAKEFLAGS += --warn-undefined-variables
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the link for the effect of those flags

Makefile Outdated
MAKEFLAGS += --warn-undefined-variables
MAKEFLAGS += --no-builtin-rules

PHPBREW_PHAR = phpbrew
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed TARGET to PHPBREW_PHAR for readability

$(TARGET): vendor $(shell find bin/ shell/ src/ -type f) box.json.dist .git/HEAD
box compile
touch -c $@
PHAR_SRC_FILES := $(shell find bin/ shell/ src/ -type f)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted $(shell find bin/ shell/ src/ -type f) into a dedicated variable

PHAR_SRC_FILES := $(shell find bin/ shell/ src/ -type f)


.DEFAULT_GOAL := help
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduce a help command. I don't think it is wise to display all the commands there, but it will be good to document the most important ones. For now I've only documented the building PHAR one.


.PHONY: sign
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the sign and SIGNATURE because now the PHAR is signed directly in the CI so there is no need for it in the Makefile anymore.

sign: $(SIGNATURE)
.PHONY: build
build: ## Builds PHPBrew PHAR
build:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty much the previous make phpbrew, with the difference that it will always compile the PHAR regardless of whether the PHAR was already built and up to date.

You also have _build, which I call "shadow command" and mimicks build, but there it does not care about the targets so it will nto rebuild the PHAR if not necessary.

I like to have this kind of command + shadow command to have a good UX whilst being able to keep shortucts of one needs it.

For example, in my projects I often have a make serve which will start the webserver, but will also boot up docker if not already booted up, install deps if not installed, migrate the DB if it is not up to date etc. It's a lot of stuff, and sometimes I just want to boot up the webserver without caring about all of that, and that's where I use a "shadow command", i.e. I would have make _serve that would just do that.

Note that here in this case, _build is effectively just an alias for phpbrew so might no be super useful. We can do without really

touch -c $@

PHONY: vendor_install
vendor_install:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some extra targets to make sure the vendor is up to date.

In general one can use composer install as usual, but if you want to force it to make sure the Make targets are up to date then you can call make vendor_install, and it will update all the targets.

@theofidry theofidry force-pushed the build/makefile branch 3 times, most recently from 937e3cd to 9cd14c2 Compare July 2, 2024 09:21
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.

1 participant