Conversation
theofidry
left a comment
There was a problem hiding this comment.
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
| run: composer global require --dev humbug/box | ||
|
|
||
| - name: Ensure the dependencies are up to date | ||
| run: make vendor |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
See the link for the effect of those flags
| MAKEFLAGS += --warn-undefined-variables | ||
| MAKEFLAGS += --no-builtin-rules | ||
|
|
||
| PHPBREW_PHAR = phpbrew |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
| composer install | ||
| touch $@ | ||
|
|
||
| .PHONY: sign |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
937e3cd to
9cd14c2
Compare
Add various changes to the Makefile:
buildcommand to rebuild the entire PHAR regardless of whether one already exists and is up to datehelpcommand which can be used to display the most important commands to help out new contributors.