Details

Reviewers

Campbell Barton (campbellbarton)
Brecht Van Lommel (brecht)

Summary

This is mostly a Makefile cleanup diff, but it does address a few specific issues:

All makefile targets (in .\GNUMakefile) happen to be "phony targets" (as defined by the GNU make manual), but only target all was marked as such. Not marking a target as "phony" means that make attempts to delete a file with the target's name whenever the target's recipe either fails or is interrupted. It is unlikely a file with one of these names exists in the source directory, but as it's possible, this is technically a fix.
The makefile makes rather extensive use of targets as modal options, but this concept was neither pointed out clearly, nor given a name. I decided to name them "pseudo-targets" as I had used this name in other projects previously. I clearly spelled out in the comments what these are and how they work... which relates directly to the next item:
I added hopefully much more clear comments indicating how the makefile functions in hopes of making it easier to read for developers less familiar with more advanced features of GNU Make.
I switched "pseudo-target" detection to use $(filter ...) within a function with a meaningful name, instead of $(findstring ...). The function encapsulation is intended for readability, while the switch to filter is intended to not get false-positives from substrings. (With the current list of targets this is only a theoretical problem.)
I changed the behavior of "pseudo-targets" slightly, so they no longer imply the default goal, unless only pseudo-targets are specified. This allows specifying something like make debug test which will build and run the tests in debug mode, without building all. Previously specifying any pseudo-target automatically built all as a hard dependancy.
I found that make fails with an error when running make clean on a non-existent build directory. I avoided this error condition.

Goals

The intent of this diff is simply to cleanup the existing GNUMakefile and not implement anything new. My primary goal for this diff is to increase clarity and readability of the makefile.

Feedback

As my main goal is clarity, feedback from other developers is really the only valid measure of success of this diff. I'm especially interested in your feedback if you are not a GNU Make expert, and something I added was unclear. (Also please speak up if you notice somewhere I was in error.)

Origin

Be advised that these changes originated as unrelated parts of https://developer.blender.org/D15529

Diff Detail

Repository: rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Loren Osborn (linux_dr) requested review of this revision.Sep 1 2022, 4:13 AM

Loren Osborn (linux_dr) edited the summary of this revision. (Show Details)

make ninja test is not the best example since running tests does not involve ninja. However being able to run make debug test or make debug clean without re-building makes sense to me.

Extensive comments can be helpful, however it's easier if you don't have to understand all those concepts in the first place. I also don't think we have to explain how $\ syntax works, abstract $$ with a variable, or define any FN_ utility functions. I think the implementation can be as simple as this:

P3169 (An Untitled Masterwork)

1	diff --git a/GNUmakefile b/GNUmakefile
2	index a82d1be..19c697d 100644
3	--- a/GNUmakefile
4	+++ b/GNUmakefile
5	@@ -229,7 +229,9 @@ endif
6
7
8	# -----------------------------------------------------------------------------
9	-# additional targets for the build configuration
10	+# Pseudo targets that modify build configuration.
11	+
12	+PSEUDO_TARGETS:=bpy ccache cycles debug developer full headless lite ninja release
13
14	# support 'make debug'
15	ifneq "$(findstring debug, $(MAKECMDGOALS))" ""
16	@@ -269,6 +271,11 @@ ifneq "$(findstring ccache, $(MAKECMDGOALS))" ""
17	CMAKE_CONFIG_ARGS:=-DWITH_COMPILER_CCACHE=YES $(CMAKE_CONFIG_ARGS)
18	endif
19
20	+# Define pseudo targets. When only pseudo targets are specified, default to the
21	+# "all" target". Otherwise only act as modifications on other targets.
22	+$(foreach pseudo_target, $(PSEUDO_TARGETS),$\
23	+ $(eval $(pseudo_target): $(if $(filter-out $(PSEUDO_TARGETS),$(MAKECMDGOALS)),,all)))
24	+
25	# -----------------------------------------------------------------------------
26	# build tool
27
28	@@ -359,17 +366,6 @@ all: .FORCE
29	@echo Blender successfully built, run from: $(BLENDER_BIN)
30	@echo
31
32	-debug: all
33	-full: all
34	-lite: all
35	-release: all
36	-cycles: all
37	-headless: all
38	-bpy: all
39	-developer: all
40	-ninja: all
41	-ccache: all
42	-
43	# -----------------------------------------------------------------------------
44	# Build dependencies
45	DEPS_TARGET = install

This revision now requires changes to proceed.Sep 3 2022, 4:21 AM

Loren Osborn (linux_dr) mentioned this in D15529: Add `verbose` pseudo-targets to Makefile wrapper.Sep 3 2022, 6:13 AM

In D15830#430950, @Brecht Van Lommel (brecht) wrote:

make ninja test is not the best example since running tests does not involve ninja. However being able to run make debug test or make debug clean without re-building makes sense to me.

Good point… make ninja test was simply an example I pulled out of thin air without ever having used ninja.

Extensive comments can be helpful, however it's easier if you don't have to understand all those concepts in the first place. I also don't think we have to explain how $\ syntax works, abstract $$ with a variable, or define any FN_ utility functions. I think the implementation can be as simple as this:
P3169 (An Untitled Masterwork)
1 diff --git a/GNUmakefile b/GNUmakefile
2 index a82d1be..19c697d 100644
3 --- a/GNUmakefile
4 +++ b/GNUmakefile
5 @@ -229,7 +229,9 @@ endif
6
7
8 # -----------------------------------------------------------------------------
9 -# additional targets for the build configuration
10 +# Pseudo targets that modify build configuration.
11 +
12 +PSEUDO_TARGETS:=bpy ccache cycles debug developer full headless lite ninja release
13
14 # support 'make debug'
15 ifneq "$(findstring debug, $(MAKECMDGOALS))" ""
16 @@ -269,6 +271,11 @@ ifneq "$(findstring ccache, $(MAKECMDGOALS))" ""
17 CMAKE_CONFIG_ARGS:=-DWITH_COMPILER_CCACHE=YES $(CMAKE_CONFIG_ARGS)
18 endif
19
20 +# Define pseudo targets. When only pseudo targets are specified, default to the
21 +# "all" target". Otherwise only act as modifications on other targets.
22 +$(foreach pseudo_target, $(PSEUDO_TARGETS),$\
23 + $(eval $(pseudo_target): $(if $(filter-out $(PSEUDO_TARGETS),$(MAKECMDGOALS)),,all)))
24 +
25 # -----------------------------------------------------------------------------
26 # build tool
27
28 @@ -359,17 +366,6 @@ all: .FORCE
29 @echo Blender successfully built, run from: $(BLENDER_BIN)
30 @echo
31
32 -debug: all
33 -full: all
34 -lite: all
35 -release: all
36 -cycles: all
37 -headless: all
38 -bpy: all
39 -developer: all
40 -ninja: all
41 -ccache: all
42 -
43 # -----------------------------------------------------------------------------
44 # Build dependencies
45 DEPS_TARGET = install

I believe there may be a bit of a happy medium here:

Your proposed implementation has a few issues:
- does not mark all the targets as .PHONEY, so make might inadvertently remove poorly named files on failure.
- does not change $(findstring to $(filter. This solves a potential rather than current problem, but does not introduce any additional complexity.
- I’m unsure if the $(evaled rules, as you wrote them in a $(foreach will actually be valid without explicit linebreak that I did not see. (I have since realized that this rule doesn’t really need a $(foreach nor an $(eval)
- the silenced commented out recipe line for the pseudo target rules actually does have a purpose. Leaving it out, actually sometimes causes a error message.
- uses the made-up term “pseudo target” while only barely suggesting what it means
I agree I went a bit overboard with the comments, but I feel, especially with the focus on CMake, more blender developers are less familiar with GNU Make than other tools. As such I thought documenting more obscure and confusing features I thought other blender developers may be unfamiliar with. I concede, not using the features prevents the confusion.
I had two reasons for defining the functions in this diff:
- The one that I really can’t justify is wanting to keep all the pseudo target code together, as I was thinking of this as a unit I took from else where. This is also part of why I kept it overly generic.
- While I could have chose better names, I was trying to defer to Robert Martin’s “clean code” practice of replacing an unclear code fragment with a descriptive function name. Unfortunately, user defined functions in GNU Make are sufficiently esoteric to make that plenty confusing in it’s own right.

I think I can easily move the pseudo target rules to the end of the file and get rid of the $(eval… and drop the FN_EVAL__ and FN_SH__ comments, get rid of $(DOLLARS) and related lines… that might also get rid of the $\ and the giant explanation that goes with it…

I’m not sure that will satisfy you, but I think it’s a strong step in the right direction.

In D15830#430964, @Loren Osborn (linux_dr) wrote:

does not mark all the targets as .PHONEY, so make might inadvertently remove poorly named files on failure.

This seems reasonable to fix, maybe .PHONY: all $(MAKECMDGOALS) will do.

does not change $(findstring to $(filter. This solves a potential rather than current problem, but does not introduce any additional complexity.

Doing that change is fine.

I’m unsure if the $(evaled rules, as you wrote them in a $(foreach will actually be valid without explicit linebreak that I did not see. (I have since realized that this rule doesn’t really need a $(foreach nor an $(eval)

the silenced commented out recipe line for the pseudo target rules actually does have a purpose. Leaving it out, actually sometimes causes a error message.

...
I think I can easily move the pseudo target rules to the end of the file and get rid of the $(eval… and drop the FN_EVAL__ and FN_SH__ comments, get rid of $(DOLLARS) and related lines… that might also get rid of the $\ and the giant explanation that goes with it…

Not sure what you exactly have in mind for the pseudo targets, but as long as it's a few relatively simple lines of code it should be fine.

In D15830#430987, @Brecht Van Lommel (brecht) wrote:

In D15830#430964, @Loren Osborn (linux_dr) wrote:

does not mark all the targets as .PHONEY, so make might inadvertently remove poorly named files on failure.

This seems reasonable to fix, maybe .PHONY: all $(MAKECMDGOALS) will do.

I agree that listing every .PHONEY target is brittle, which I don't like, but using $(MAKECMDGOALS) breaks if any phoney target depends on another... ~~It also breaks for the default goal where $(MAKEMDGOALS) is empty.~~

I'd like to use a more robust solution, but listing each target seems best in the meantime.

In D15830#430950, @Brecht Van Lommel (brecht) wrote:

...I also don't think we have to ... abstract $$ with a variable...

I have to admit this was a (selfish) concession to the broken syntax highlighter in Sublime Text's Makefile syntax (my current preferred editor). Sorry.

Revised makefile cleanup based on @Brecht Van Lommel (brecht)'s feedback

Loren Osborn (linux_dr) added inline comments.Sep 3 2022, 9:47 PM

GNUmakefile
161	I think I should have chosen a better name: either `PSEUDO_TARGET_OPTIONS` or even just `PSEUDO_TARGETS`

Renamed OPTION_PSEUDO_TARGETS to PSEUDO_TARGETS

Brecht Van Lommel (brecht) requested changes to this revision.Sep 5 2022, 1:45 PM

Brecht Van Lommel (brecht) added inline comments.

GNUmakefile
161	Do we really need `$(strip`?
171	Suggest to rename `ALL_PHONY_TARGETS` to just `TARGETS`. `ALL` is not adding information, and they are all phony anyway.
199	Don't define a function, just replace findstring with filter below.
328–334	I don't think we should be using `$(MAKE)` here, let's not change this. I expect CMake to generate makefiles for whatever `make` is, without taking into account how `GNUMakefile` was invoked.

This revision now requires changes to proceed.Sep 5 2022, 1:45 PM

Loren Osborn (linux_dr) marked an inline comment as done.Sep 5 2022, 3:46 PM

Loren Osborn (linux_dr) added inline comments.

GNUmakefile
161	Naw… I was showing off… I’ll fix it.
171	Good catch on `ALL_`. `PHONEY_` conveys the semantic purpose of this variable and exactly how it’s used. I fully expect this makefile to have non-phoney targets in the future.
199	I’m conflicted here: The value of this function lies entirely in its name, which provides meaningful semantic context, making the code more readable. GNU make functions are obscure enough that I felt the need to heavily document how to call them, which detracts from code readability. I will defer to your judgement and ditch the function.
328–334	The GNU Make manual actually specifies you should always use `$(MAKE)` in a Makefile… never `make`

Brecht Van Lommel (brecht) added inline comments.Sep 5 2022, 6:37 PM

GNUmakefile
171	I don't expect it to, but either way is fine.
328–334	The manual is not going to anticipate all scenarios, I don't think it's the correct usage here.

Loren Osborn (linux_dr) added inline comments.Sep 5 2022, 10:45 PM

GNUmakefile
328–334	Ok… I admit I somewhat stuck “always use `$(MAKE)`” in the back of my mind and mostly took it on faith. Now I’ve dug into exactly what it does, and think we should use it here, but I’ll document my findings and defer to your judgement. First let me document the three possible recipe line prefixes, as I was only aware of `@` myself: `@` “silent” Execute this recipe line, but do not first echo the expanded line to STDERR `+` “recursive” Execute this recipe line, even if `-t`/ `--touch`, `-q`/ `--question` or `-n` / `--just-print` are specified. `-` “ignore errors” Execute the recipe line, but ignore the exit status. So it turns out that `$(MAKE)` or `${MAKE}` in any recipe line: implies a `+` line prefix. implies a `-w` / `—print-directory` whenever `-C` / `—directory=` is specified but `-s` / `—silent` isn’t Forces variable `MAKEFLAGS` to be exported to the environment. This definitely seems like the correct behavior for `-n`. It also seems like, when a `VERBOSE` option is setup to disable `-s`, the implied `-w` will also be the expected behavior. We may even want to add a `+` prefix and `—dry-run` option to some of the Python scripts that dispatch to other commands for `-n` mode, but that’s out of the scope of this diff.

Just noting that T100843 is a known preexisting GNUmakefile issue not addressed by this diff.

More cleanup based on @Brecht Van Lommel (brecht) 's code review.

Resolved failure case of make clean discovered in testing.

missing diff EOF final newline character.

Loren Osborn (linux_dr) added inline comments.Sep 6 2022, 3:06 AM

GNUmakefile
605	found in testing

Loren Osborn (linux_dr) marked 4 inline comments as done.Sep 6 2022, 5:27 AM

Loren Osborn (linux_dr) edited the summary of this revision. (Show Details)Sep 6 2022, 5:31 AM

Loren Osborn (linux_dr) edited the summary of this revision. (Show Details)Sep 6 2022, 5:34 AM

The changes to PHONY targets are beyond my understanding of Makefile's, while the summary makes sense I'm not able to properly review them (trusting @Loren Osborn (linux_dr)'s changes are correct).

The issue with files existing that match make target names is something I've run into before, while it's fairly unlikely it's the kind of issue that can trip up developers occasionally and is worth fixing.

GNUmakefile
167	`dependancy` - typo.
169	`non-existant` - typo.
171–184	Prefer single column lists for a few reasons: Easier to keep sorted and visually scan. Simpler to add new items and sort the list. Adding a new item in the middle doesn't mean re-wrapping the columns (or always adding new items at the end, making the list unordered). Applies to `PSEUDO_TARGETS`too. e.g. PHONY_TARGETS:= \ all \ check_clang_array \ check_cmake \ check_cppcheck \ check_deprecated \ check_descriptions \ check_licenses \ check_mypy \ check_pep8 \ check_smatch \ check_sparse \ check_spelling_c \ check_spelling_osl \ check_spelling_py \ check_splint \ clean \ config \ deps \ doc_dna \ doc_doxy \ doc_man \ doc_py \ format \ help \ help_features \ icons \ icons_geom \ package_archive \ project_eclipse \ project_netbeans \ project_qtcreator \ source_archive \ source_archive_complete \ test \ update \ update_code \ $(PSEUDO_TARGETS)
328–334	Switching from `make` to `$(MAKE)` seems out of scope for this patch. While I'm not all that fussed, it's better in general to avoid smaller changes creeping in. Can always be made as a separate patch.
606	Prefer multi-line if statements as they read more easily. if [ -d "$(BUILD_DIR)" ]; then \ $(BUILD_COMMAND) -C "$(BUILD_DIR)" clean; \ fi
608	`existance` - typo.
610	`dependancy` - typo.

This revision now requires changes to proceed.Sep 6 2022, 5:53 AM

Loren Osborn (linux_dr) added inline comments.Sep 6 2022, 6:43 AM

GNUmakefile
167	Once I thought I could learn to spell… I no longer harbor this delusion. 😜
171–184	While I think @Brecht Van Lommel (brecht) and I would prefer to not repeat a brittle list of all targets in the file, I do agree the single column list is the best alternative for exactly the reasons you gave.
328–334	While the only practical behavioral change this makes is `make -n` (or `-t` or `-q`) recursing into sub-makes or not, your comment about scope-creep reminded me that the makefiles we would be recursing into may not exist, so we likely need at least one or two `+` prefixes on `cmake` lines to generate the applicable makefiles. In light of that, I think breaking this into a separate diff may make sense.
606	I debated this myself… good catch

Campbell Barton (campbellbarton) added inline comments.Sep 7 2022, 3:46 AM

GNUmakefile
265	If having space after the comma is optional (I assume it is), prefer to keep it.

If we are going to use .PHONY for all targets, then I think .FORCE mechanism is redundant and can be removed.

Next round of GNUmakefile cleanup revisions, with suggestions from both @Campbell Barton (campbellbarton) and @Brecht Van Lommel (brecht),

Made lists single column. (@Campbell Barton (campbellbarton))
Fixed spelling errors. (@Campbell Barton (campbellbarton))
Restored accidentally stripped space in $(filter ...) function calls. (@Campbell Barton (campbellbarton))
$(MAKE) changes:
- Reverted $(MAKE) changes. (@Brecht Van Lommel (brecht))
- Discovered related Makefile generation issue with -t / -n / -q
- Added TODO: comments for the proposed $(MAKE) changes that will be addressed in a separate diff.
Found a broken dependency in package_archive target:
- Added new phony target build as more descriptive alias for default phony target all
- Added new non-phony target $(BUILD_DIR)/Makefile shared by package_archive and new target build
- Added new non-phony target $(DEPS_BUILD_DIR)/Makefile used by deps to parallel $(BUILD_DIR)/Makefile
Added line-breaks to shell if statement in recipe for clean (@Campbell Barton (campbellbarton))
Converted all shell comments (except for $(PSEUDO_TARGETS)) back to make comments.
Got rid of dependance on .FORCE from all phony targets, as that is redundant. (@Brecht Van Lommel (brecht))
Renamed .FORCE to FORCE to be more consistent with the GNU Make manual and common usage. (Leading . is for builtin special targets, where FORCE is not a builtin special target)

Loren Osborn (linux_dr) marked 10 inline comments as done.Sep 7 2022, 10:35 PM

Loren Osborn (linux_dr) added inline comments.Sep 7 2022, 10:56 PM

GNUmakefile
265	In general, spaces around commas in Makefile functions between arguments are significant, but I believe `$(filter ...)` specifically strips whitespace out of the result.
491	This target had an actual broken dependency, as you could be running make on a Makefile in a directory that doesn't exist yet. This should be fixed now.

Minor whitespace fixup

Missed a few inconsistencies in the whitespace fixup

The diff looks fine to me now, tested bpy target, check_cmake & icons_geom - they work fine.

GNUmakefile
347–352	These comments aren't clear to me (in that I wouldn't know how approach resolving them). If there is a problem, then it should be clear what the problem is and why it needs to be solved. If it's more an issue of being technically correct, then that should be noted instead. Not blocking issues but I think these kinds of issues could be handled outside of comments, tasks even - although I suspect they are quire low priority unless they are actually causing problems.

Brecht Van Lommel (brecht) requested changes to this revision.Sep 8 2022, 2:19 AM

Brecht Van Lommel (brecht) added inline comments.

GNUmakefile
347–352	I don't think recursive make behavior is better, so I think this comment and following TODO comments to suggest using `$(MAKE)` should be removed.
432	This breaks ninja builds, which don't have a `Makefile`.
454	Also breaks ninja builds.
491	This adds a dependency on running cmake config, but creating a package archive doesn't work until there is a Blender build to package. So not sure this actually helps much, would be simpler to add a dependency on `build` and not have these `Makefile` targets. This `package_archive` is not what we use for packaging Blender and doesn't seem to be working correctly, I suggest to remove this target entirely.

This revision now requires changes to proceed.Sep 8 2022, 2:19 AM

Loren Osborn (linux_dr) added inline comments.Sep 8 2022, 2:55 AM

GNUmakefile
432	Yes, this occurred to me about an hour ago, and I was discussing it with @Campbell Barton (campbellbarton). This is complicated by `package_archive` depending on the make (and therefore the Makefile) even in ninja mode. In light of this, I’m happy to revert this.

Loren Osborn (linux_dr) added inline comments.Sep 8 2022, 5:10 AM

GNUmakefile
347–352	Umm… I’m unclear what your desired behaviors are here. Make calls make, so, no matter how you slice it, it’s recursive. The only change is when using `-n` / `-t` / `-q`. My understanding is `-n` is supposed to tell you what commands make will execute. Is there a reason it shouldn’t do this recursively? I’m not disagreeing; I’m simply trying to understand your point of view. Once we’re on the same page about how this should operate, I can get this to align with it.

Brecht Van Lommel (brecht) added inline comments.Sep 8 2022, 1:41 PM

GNUmakefile
347–352	I think it's more useful for `make -n` to show the cmake and make commands rather than the massive list of operations the cmake generated makefile does. Someone wanting to do something more advanced can directly use the makefiles in the build directory.

In future I'd request these kinds of patches are more limited - and don't (for e.g.) change arguments to make, they can slip through review too easily and complicate patch review.

This would be better split into multiple smaller patches which we can review more easily.

In D15830#432585, @Campbell Barton (campbellbarton) wrote:

In future I'd request these kinds of patches are more limited - and don't (for e.g.) change arguments to make, they can slip through review too easily and complicate patch review.

This would be better split into multiple smaller patches which we can review more easily.

I certainly agree in theory… but as this was split off from a diff that has now been subdivided into at least 4 diffs now, I think it was mostly a case of my not understanding the scope of my change.

As far as not “changing make arguments”, I think this is likely a misunderstanding, where handling unanticipated complexities introduced by the behavior of existing makefile options..

As I am currently completing paperwork accepting a new job starting the end of the month, this may delay my (hopefully final) revision to this diff by a few days.

Thank you for your patience

GNUmakefile
347–352	While I find this behavior totally counter-intuitive, it makes a lot more sense why you said much of what you said previously… including seeing the verbose option being of minimal value. I may add a pseudo-target modal for recursive behavior of these three operation modes in a separate diff, but this expectation certainly simplifies this diff.

[BUILD SYSTEM] ./GNUMakefile cleanup
Needs RevisionPublic
Actions

Details

Summary

Goals

Feedback

Origin

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 55288

GNUmakefile

[BUILD SYSTEM] ./GNUMakefile cleanupNeeds RevisionPublicActions

Details

Summary

Goals

Feedback

Origin

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 55288

GNUmakefile

[BUILD SYSTEM] ./GNUMakefile cleanup
Needs RevisionPublic
Actions

Revision Contents
Changeset List