Makefile refactor #119

ingydotnet · 2025-09-23T21:46:02Z

Refactored the Makefile in 15 commits, with details in each commit message.

Found and fixed some bugs along the way.

All previous targets should work as before.

jfhamlin

thanks for the cleanup! looks awesome and works well :). just had a few questions/nits and then i think we're good to merge!

jfhamlin · 2025-09-25T04:21:36Z

Makefile

+
+SHELL := bash
+
+GO-VERSION ?= 1.19.3


why the switch from SNAKE_CASE to KEBAB-CASE? i've seen kebab for targets but snake seems like the norm for variables

I don't know if you read through my explanations in the commit messages, but 7f2d72d

Makefiles inherit from the env which only allows SNAKE_CASE. KEBAB-BASE is only allowed in Makefile vars, not env ones, so it makes the vars ownership very clear.

You can override vars from the command line like this:

ENV_VAR=foo make all MAKE-VAR=bar

Also in commit 1b0f4cd I made almost all of the SINGLEWORD Makefile vars into MULTI-WORD vars, to drive home that they don't ever come from (can't come from) the environment.

fair point! don't feel strongly, but i wanted to understand the motivation and i failed to read the (useful!) commit messages :)

jfhamlin · 2025-09-25T04:22:03Z

Makefile

+STDLIB-ORIGINALS-DIR := scripts/rewrite-core/originals
+STDLIB-ORIGINALS := $(wildcard $(STDLIB-ORIGINALS-DIR)/*.clj)
+STDLIB-NAMES := $(STDLIB-ORIGINALS:scripts/rewrite-core/originals/%=%)
+STDLIB-ORIGINALS := $(STDLIB-NAMES:%=scripts/rewrite-core/originals/,%)


is this line needed?

Yes. It's this line in main: https://github.com/glojurelang/glojure/blob/main/Makefile#L6

jfhamlin · 2025-09-25T04:36:39Z

Makefile

+#-------------------------------------------------------------------------------
+default: all

-.PHONY: all


should we keep the .PHONY declarations for the targets that don't produce files?

No. .PHONY is not only noisy and useless. I found out from this refactor that it can be dangerous.

Here https://github.com/glojurelang/glojure/blob/main/Makefile#L44 all deps on go-generate but that target doesn't exist. The Makefile should error but it doesn't because https://github.com/glojurelang/glojure/blob/main/Makefile#L52 marks go-generate as PHONY.

See commit messages:

2c4a126

cdd242f

fwiw i've been bitten by missing .PHONY before, but i don't feel strongly. happy to run with this

Me too, but those bites are rare and almost instantly recognized.
The bite I described (where PHONY masked a typo) remained hidden for a while.

Here's an interesting fact about PHONY you might now know...

You can put all your PHONY targets on one line at the end of the file if you want:

.PHONY: all test clean etc

You can put all your PHONY targets on one line at the end of the file if you want:

ah, i'm familiar with that, but i prefer to keep them adjacent to their targets so it's harder for them to get out of sync — which i managed to do with one of the targets anyway :)

ingydotnet · 2025-09-25T05:08:11Z

thanks for the cleanup! looks awesome and works well :). just had a few questions/nits and then i think we're good to merge!

Thanks! FWIW, I learned a few new Makefile tricks from your work along the way. 👍

jfhamlin

Thanks for the clarifications! I'll merge after CI finishes :)

ingydotnet · 2025-09-25T05:21:27Z

I can resolve if you want...

ingydotnet · 2025-09-25T05:21:45Z

Let me know.

jfhamlin · 2025-09-25T05:23:00Z

I can resolve if you want...

ah, please do if you don't mind!

I was getting failures for `make test` until I realized I hadn't pulled the test suite submodule. Now we always update submodules first so should work for everyone. Added report.html to clean target and ignored it.

Setting SHELL to bash allows us to make use of a more dependable environment. It also allows us to use bash syntax which in many cases is cleaner because you don't need to be concerned with supporting any shell. `bash` is preferable to `/bin/bash` since it will pick up possibly more preferable bash version from the PATH. Like on macOS, /bin/bash is stuck at 3.2, but its easy to `brew install bash` to get the latest version in /usr/local/bin/bash.

Make allows `-` in var names. (Actually it allows nearly all chars :). Environment variables allow allow \w chars (alphanum and _). Make inherits vars from the environment. By using `-` over `_` we make clear which vars come from the environment and which are local to the Makefile.

For consistency, added `-` where 2 words were made into one. Added 2nd word to a few one word vars.

Only `.PHONY: test` is needed because we have a `test` dir. The other .PHONY targets add little value and are noisy.

The generate target was not being run. It did not cause an error becase it was hidden by a bad .PHONY target.

This makes things easier to understand and maintain.

and make them overridable by user. Added a usage comment to top. Note: Where env var overrides go before a command, Makefile var overrides go after the `make` command.

Simplify to aot command

Using @ is for noisy or inconsequential commands. It's important to see what make is actually running though. Helps see obvious mistakes as well.

Let's us make these individually if needed.

'make test' runs both like before. Before TEST-FILES was picking up the test suite glj file by mistake, I believe.

a || b && c && d in bash always runs c and d, which was not the intent, I believe.

The command `make` runs the first Makefile target. Having the first target be called 'default' makes it obvious what is going on. I often specify no deps for the default target so running `make' prints: Nothing to be done for 'default'.

Add force=1 to the end of a make command to force rebuilds. Adds update-clojure-sources to the front of `make all`.

For use in Makefiles that sit in front of this one. For forks that are working on new AOT and platforms.

ingydotnet · 2025-09-25T05:44:36Z

Rebased origin/main and added commit: 751490c

ingydotnet requested a review from jfhamlin as a code owner September 23, 2025 21:46

ingydotnet mentioned this pull request Sep 23, 2025

Makefile refactor to use the Makes makefile system #75

Closed

ingydotnet force-pushed the makefile-refactor branch from f2b04c9 to 44d449a Compare September 24, 2025 19:38

jfhamlin reviewed Sep 25, 2025

View reviewed changes

ingydotnet requested a review from jfhamlin September 25, 2025 04:58

jfhamlin approved these changes Sep 25, 2025

View reviewed changes

ingydotnet added 19 commits September 24, 2025 22:24

Always update submodules for make test

d960960

I was getting failures for `make test` until I realized I hadn't pulled the test suite submodule. Now we always update submodules first so should work for everyone. Added report.html to clean target and ignored it.

All Makefiles vars have multiple words

04a5861

For consistency, added `-` where 2 words were made into one. Added 2nd word to a few one word vars.

Remove most .PHONY targets from Makefile

9b54cf1

Only `.PHONY: test` is needed because we have a `test` dir. The other .PHONY targets add little value and are noisy.

Fix bad target name in Makefile

eed89b6

The generate target was not being run. It did not cause an error becase it was hidden by a bad .PHONY target.

Wrap long lines in Makefile

1541bab

This makes things easier to understand and maintain.

Move VERSION vars to top of Makefile

538b858

and make them overridable by user. Added a usage comment to top. Note: Where env var overrides go before a command, Makefile var overrides go after the `make` command.

Move aot namespaces into Makefile var

27a8573

Simplify to aot command

Don't hide important Makefile commands with @

d4dc377

Using @ is for noisy or inconsequential commands. It's important to see what make is actually running though. Helps see obvious mistakes as well.

Add phony targets to groups under 'all'

1a0a701

Let's us make these individually if needed.

Separate 'make test-glj' from 'make test-suite'

ebc5ae7

'make test' runs both like before. Before TEST-FILES was picking up the test suite glj file by mistake, I believe.

Fix compound bash command in Makefile

23c5209

a || b && c && d in bash always runs c and d, which was not the intent, I believe.

In bash '&>' is cleaner way to do '2>&1 >'

dbed325

Add Makefile 'default' target

1235e30

The command `make` runs the first Makefile target. Having the first target be called 'default' makes it obvious what is going on. I often specify no deps for the default target so running `make' prints: Nothing to be done for 'default'.

Support force=1 on make commands

5747a3f

Add force=1 to the end of a make command to force rebuilds. Adds update-clojure-sources to the front of `make all`.

Remove scripts/rewrite-core/.cpcache/ on 'make clean'

16d4e81

Support adding extra aot namespaces and Go platforms

ec8bc0e

For use in Makefiles that sit in front of this one. For forks that are working on new AOT and platforms.

Changes from running 'make format; make all force=1'

751490c

ingydotnet force-pushed the makefile-refactor branch from 44d449a to 751490c Compare September 25, 2025 05:43

ingydotnet requested a review from jfhamlin September 25, 2025 05:43

jfhamlin merged commit 3b2afdd into glojurelang:main Sep 25, 2025
1 check passed

ingydotnet deleted the makefile-refactor branch September 25, 2025 19:52

Makefile refactor #119

Makefile refactor #119

Uh oh!

Conversation

ingydotnet commented Sep 23, 2025

Uh oh!

jfhamlin 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!

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!

ingydotnet commented Sep 25, 2025

Uh oh!

jfhamlin left a comment

Choose a reason for hiding this comment

Uh oh!

ingydotnet commented Sep 25, 2025

Uh oh!

ingydotnet commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfhamlin commented Sep 25, 2025

Uh oh!

ingydotnet commented Sep 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ingydotnet commented Sep 25, 2025 •

edited

Loading