Skip to content
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

Add post processing support to Userspace #9070

Closed
wants to merge 1 commit into from

Conversation

drashna
Copy link
Member

@drashna drashna commented May 12, 2020

Description

Moves rules.mk processing for userspace to a proper position (eg, keyboard -> userspace -> keymap/layout), and adds a post_keymaps.mk that runs after keymap processing.

However, I'm hard pressed to even come up with an example where this is needed, or even useful. ESPECIALLY in favor of something like #9058, as that is something we sort of already have (LAYOUT_HAS_RGB).

Because of that, I haven't added documentation, since ... I would need an example to use.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@drashna drashna added wontfix needs doc core breaking_change Changes that need to wait for a version increment labels May 12, 2020
@vomindoraan
Copy link
Contributor

Would this in combination with #2046 match the behavior I described in #9058 (comment)?

@drashna
Copy link
Member Author

drashna commented May 12, 2020

This by itself would, I think. It moves the rules.mk to before the keymap's rules.mk is checked, and then run the post_keymap.mk.

Honestly, I'm not convinced that we need either.

@@ -299,6 +302,11 @@ ifneq ("$(wildcard $(KEYMAP_PATH)/config.h)","")
CONFIG_H += $(KEYMAP_PATH)/config.h
endif

# Userspace post config.h pass
ifneq ("$(wildcard $(USER_PATH)/config.h)","")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should check for post_config.h instead of config.h.

Suggested change
ifneq ("$(wildcard $(USER_PATH)/config.h)","")
ifneq ("$(wildcard $(USER_PATH)/post_config.h)","")

@vomindoraan
Copy link
Contributor

vomindoraan commented May 13, 2020

Here's an example of how this could help me significantly clean up my keymap rules.mk files, reduce duplication, and also allow me to override feature flags on a per-keymap, as-needed basis (see kbd6x keymap): vomindoraan@bc4b8e0
Tested and confirmed working (albeit with the above suggestion applied).

This can also work with <FEATURE>_SUPPORTED flags proposed in #9058 by default, without needing to explicitly check for those flags in userspace code (see #9058 (comment)). This is possible by running disable_features.mk after the userspace and keymap rules.mk, just like the current implementation in that PR does it, but before the new post_keymap.mk file.

See this commit for a proof of concept → vomindoraan@deb080b

Let me know if you see any problems with this approach. The entire demo branch can be found here.

@vomindoraan vomindoraan mentioned this pull request May 13, 2020
8 tasks
@drashna drashna changed the base branch from master to develop June 5, 2020 09:38
@sigprof
Copy link
Contributor

sigprof commented Jun 21, 2020

How many current rules.mk files would break after changing the include order? Would it be better to keep rules.mk included where it is currently, and include pre_keymap.mk before the keymap instead?

With the change proposed by @vomindoraan we could even have 3 files there:

  • pre_keymap.mk — can set options which then could be overridden by the rules.mk
    from the keymap;
  • rules.mk — can override options which were set in the keymap, as it works currently (to minimize the amount of changes made to the current code);
  • post_features.mk — can see final values of all options after unsupported ones were reset, but should not change any options.

However, the problem with this approach is that the rules.mk name could become confusing (it does not say anything about the processing order). So we could look for post_keymap.mk first (which should be used in new userspace folders), but fall back to rules.mk for compatibility if post_keymap.mk does not exist.

@sigprof
Copy link
Contributor

sigprof commented Jun 22, 2020

How should this change work together with the documented support for overriding the userspace name by setting the USER_NAME variable in rules.mk at the keymap level? The patch suggested at the moment will just break that feature.

Looks like at least some .mk file from the keymap must come before the userspace .mk, and if the possibility of having a keymap makefile which can override some options from the userspace makefile is really desired, it could be possible to add support for having something like post_userspace.mk in keymaps.

Alternatively, it is possible to do nothing, because the same person who adds a keymap using their userspace may also change rules.mk files in both the keymap and the corresponding userspace, so they do not conflict with each other (maybe by setting some custom variables in the keymap .mk and using them in the userspace .mk).

The change which adds post_config.h support for userspace also does not look correct, because this file would come after keyboard-level post_config.h.

@vomindoraan
Copy link
Contributor

Any updates on this?

@tzarc tzarc closed this Feb 27, 2021
@tzarc tzarc deleted the branch qmk:develop February 27, 2021 20:27
@noroadsleft noroadsleft reopened this Feb 27, 2021
@drashna drashna force-pushed the features/post_keymap.mk branch from c883c1b to 7d5762d Compare May 30, 2021 07:17
@drashna drashna force-pushed the features/post_keymap.mk branch from 7d5762d to 22e2b3f Compare July 3, 2021 23:55
@drashna drashna closed this Sep 5, 2021
@drashna drashna deleted the features/post_keymap.mk branch September 5, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change Changes that need to wait for a version increment core needs doc wontfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants