Automatically include build.mk after rules.mk.

[seebs]

In some cases, you might want to have rules which are conditional
on settings, but to allow later files to override the settings.

For instance:

FOO_ENABLE = no
ifeq $($(strip $(FOO_ENABLE)),yes)
    SRC += foo.c
endif

if you override FOO_ENABLE elsewhere, nothing happens.

So we split this into a rules.mk containing the setting, and
a build.mk containing the build rules, then include every
build.mk file after all the rules.mk files have been
included.

A call to include with no filename in an eval works
harmlessly, so we don't have to do existence checks.

Signed-off-by: seebs [email protected]

[drashna]

Looks like there are some merge conflicts here.

Would you mind updating this?

Especially, as I think we talked about this internally recently, and it is something that we may want to actually implement.

[seebs]

So I had completely forgotten about this, and when I went to master and the patch wasn't there, I thought someone had removed the rules/build thing... But now I realize I just never got it merged.

I can try to regenerate it. I do still want it, for exactly the same reason; it makes conditional inclusion of modules easier, and makes it possible for individual layouts to control presence of features.

[seebs]

Updated. I haven't checked this out as carefully as I'd like, and actually it looks like something called i2c_master is getting compiled in anyway, but I don't actually understand the RGB_MATRIX_ENABLE setting. Checking with $(info ...) did suggest that the SRC list was being changed, though.

[drashna]

i2c_master.c is used by the RGB Matrix, and the Ergodox EZ has been updated to using /drivers/avr/i2c_master.c, rather than a custom i2c implementation. (if that helps)

[seebs]

That helps for the i2c case, but it doesn't address the other issue I had, in my LCD branch, where I have source files that are only useful to add if a given feature is enabled, but I'd want to be able to control it in the keyboard config. So the updated patch is probably actually sane now, despite the i2c_master case being irrelevant.

[drashna]

I think that @fredizzimo was trying to implement something along these lines, at one point.

I think this is probably a good change, but documentation for it would be ... useful.

[skullydazed]

I think there's a couple open questions to be answered before this is merged:

• Is build.mk the name we want to use here?
• How do we document this in a way that avoids user confusion?
  • My main concern here is that inexperienced users won't know which file to use when, and will just try them at random until something works.

I'd also like to understand better why USE_RULES is defined twice, but that's a nit more than anything.

[seebs]

The Makefile definition has a comment semi-explaining this; it's magic because it's being used by PARSE_KEYBOARD which is itself magic in a way that requires an extra $ to ensure expansions happen after PARSE_KEYBOARD is expanded, rather than before it's expanded (because it changes how USE_RULES would/should expand.)

[drashna]

This change isn't actually necessary anymore.

i2c is added via QUANTUM_LIB_SRC which has some special handling that prevents conflicts with it being added multiple times (also, prevents LTO from being applied to it, as it can cause weird timing related issues)

[seebs]

oh, neat. that would indeed resolve this part. i'm not sure it also resolves the corresponding issue with the other optional modules, added in the later patch, though?

[drashna]

Yeah. There are still a number of use cases that would make this very useful still. Such as per-keyboard features/options, that can be adjusted by the keymaps. As well as by userspace, in general.

[mtei]

I suggest using a different name instead of build.mk. For example, prepare_build.mk, post_rules.mk, etc.

[drashna]

A clearer name, or at least documentation of this would go a long way, I think.

[drashna]

• Is build.mk the name we want to use here?

I think post_rules.mk would work, since we already have post_config.h. Would keep it consistent.

• How do we document this in a way that avoids user confusion?

  • My main concern here is that inexperienced users won't know which file to use when, and will just try them at random until something works.

From what I can tell, the rules.mk would be for enabling and disabling features. The build.mk/post_rules/whatever, would be for setting defineds, including files, etc.

Basically, all of this stuff would got into the "post" file:

qmk_firmware/users/drashna/rules.mk

Lines 7 to 54 in 46d0fe4

	ifneq ($(strip $(NO_SECRETS)), yes)
	ifneq ("$(wildcard $(USER_PATH)/secrets.c)","")
	SRC += secrets.c
	endif
	ifeq ($(strip $(NO_SECRETS)), lite)
	OPT_DEFS += -DNO_SECRETS
	endif
	endif
	ifeq ($(strip $(TAP_DANCE_ENABLE)), yes)
	SRC += tap_dances.c
	endif
	ifeq ($(strip $(RGBLIGHT_ENABLE)), yes)
	SRC += rgb_stuff.c
	ifeq ($(strip $(INDICATOR_LIGHTS)), yes)
	OPT_DEFS += -DINDICATOR_LIGHTS
	endif
	ifeq ($(strip $(RGBLIGHT_TWINKLE)), yes)
	OPT_DEFS += -DRGBLIGHT_TWINKLE
	endif
	ifeq ($(strip $(RGBLIGHT_NOEEPROM)), yes)
	OPT_DEFS += -DRGBLIGHT_NOEEPROM
	endif
	ifeq ($(strip $(RGBLIGHT_STARTUP_ANIMATION)), yes)
	OPT_DEFS += -DRGBLIGHT_STARTUP_ANIMATION
	endif
	endif
	RGB_MATRIX_ENABLE ?= no
	ifneq ($(strip $(RGB_MATRIX_ENABLE)), no)
	SRC += rgb_stuff.c
	endif
	ifdef CONSOLE_ENABLE
	ifeq ($(strip $(KEYLOGGER_ENABLE)), yes)
	OPT_DEFS += -DKEYLOGGER_ENABLE
	endif
	endif
	ifeq ($(strip $(MAKE_BOOTLOADER)), yes)
	OPT_DEFS += -DMAKE_BOOTLOADER
	endif

Also, it would allow for stuff like the helix's custom options to be properly processed, rather than what it's doing now:
https://github.com/qmk/qmk_firmware/blob/master/keyboards/helix/rev2/local_features.mk

And it would allow for the userspace's rules.mk to be processed correctly, as opposed to what we have now, due to how the userspaces (such as mine) are set up.

But yes, ideally, there should be some documentation on how to use this properly.

Edit:

Also, due to the mcu_selection.mk setup, and moving everything to that, it has invalidated checks like:

qmk_firmware/layouts/community/ortho_4x12/drashna/rules.mk

Lines 32 to 36 in cee8df3

	ifeq ($(strip $(PROTOCOL)), VUSB)
	NKRO_ENABLE = no
	else
	NKRO_ENABLE = yes
	endif

Specifically, because PROTOCOL is populated AFTER the rules.mk files have all been read. Moving the mcu_selection.mk doesn't work, and re-including the rules.mk sounds like a very good way to break things.

So, more and more, this is something that we may really need.

[tzarc]

@drashna @seebs
Apart from changing things to post_rules.mk and removing the ergodox change, what else is outstanding for this PR?

[skullydazed]

We also need documentation.

On the name I think post_rules.mk is not a very good name. I have to stop and think about what it means, and that will lead to confusion by people who don't know what it means.

[drashna]

On the name I think post_rules.mk is not a very good name. I have to stop and think about what it means, and that will lead to confusion by people who don't know what it means.

Naming is hard. I'm more than open to whatever name. I'm just not sure which is best.

[seebs]

So the fundamental distinction, I think, is between "things a layout should be able to override" and "things that need to be set in response to those overrides", although all the examples I can think of are file inclusions.

Maybe rules.mk and files.mk, and files.mk is "the one that decides to include files based on per-board settings"? Except that I guess the VUSB/NKRO_ENABLE setting seems problematic.

Alternative notion: Can we do this with defines? Something like defining a thing that will expand to that ifeq/endif chain, but will expand to it and be processed after all the rules.mk get done? I think gnu make is insane enough to allow that, so you could do something like

$POST_RULES += ...

[drashna]

Well, I know we already have post_config.h supported. I'd have to double check where that is, in the chain...

[skullydazed]

We held a collaborator's meeting on this subject yesterday. We have new people on the team who have brought some fresh perspective here. I wish I could say we had a definitive answer on what the path forward is, but we don't yet. We have clarified a few issues, however.

Our primary concern right now revolves around complexity and maintainability. We're worried that this will create a situation where one needs keyboard specific context to do any maintenance at all, and since the majority of keyboards in the tree do not have an active maintainer this will put a load on anyone doing maintenance programming.

We think most of the use cases for this feature are addressed by #9058. The big open question right now is what use cases are not addressed by that PR.

[vomindoraan]

Most of my thoughts on what this feature could bring to the table moving forward are laid out in #9058 (comment).

Here I'd just like to address the naming of the new file. While post_rules.mk might not be an obvious name when considered in isolation, in the context of the current build pipeline it makes a lot of sense. Out of all of the names proposed so far, it's the only one that instantly “clicks” for me. It intuitively suggests that whatever these files do, happens after the chain of rules.mk has been processed. The presence of an existing post_config.h class of files is another plus for the name, in my opinion.

@skullydazed Could you elaborate a bit on what you mean by “create a situation where one needs keyboard specific context to do any maintenance at all”?

[drashna]

This has been added in a different PR, in a different matter/naming.