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

[Core] Re-order user space rules inclusion #17459

Merged
merged 1 commit into from
Aug 13, 2022

Conversation

filterpaper
Copy link
Contributor

@filterpaper filterpaper commented Jun 23, 2022

Description

Re-order inclusion of userspace rules.mk before mcu_selection.mk to support overriding micro controller settings, such as chibios CONVERT_TO converters or MCU selection.

Confirmed binary reproducibility with planck/rev6 and crkbd/rev1.

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: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • 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).

@filterpaper filterpaper force-pushed the userspace-builddef-reorder branch from cdb0c24 to 1cfaf59 Compare June 23, 2022 04:06
@KarlK90 KarlK90 requested a review from a team July 4, 2022 05:24
@drashna
Copy link
Member

drashna commented Jul 4, 2022

I'm not sure how I feel about allowing userspace to override the MCU.

It very much feels the opposite of what is supposed to happen (specific to general, keyboard -> keymap -> userspace).

@filterpaper
Copy link
Contributor Author

filterpaper commented Jul 4, 2022

keyboard/keymap/rules.mk can overwrite MCU selection but not the copy in userspace. I had the impression that "overwrite" priority is userspace > keymap > keyboard by design.

In its current position, CONVERT_TO= directive also cannot be used in userspace rules.mk.

@tzarc tzarc merged commit 69fa2d8 into qmk:develop Aug 13, 2022
@filterpaper filterpaper deleted the userspace-builddef-reorder branch August 13, 2022 13:58
@spidey3
Copy link
Contributor

spidey3 commented Aug 13, 2022

This seems to have broken some compiles (I noticed it with when using layouts/community/75_ansi/spidey3).

It seems to have changed ordering of community layout's rules.mk vs. user level rules.mk. In this specific case, layouts/community/75_ansi/spidey3/rules.mk used to be read before users/spidey3/rules.mk; now that is reversed, and it's breaking the ability to turn settings on/off in the community layout, and then react to those settings in the build of the userspace code. It's also inconsistent with the config.h ordering.

An an aside, the docs at https://docs.qmk.fm/#/hardware_keyboard_guidelines?id=rulesmk need to be updated (for this change, and also to explain when community layout rules.mk is loaded).

This could be somewhat fixed by allowing post_rules.mk at the user level, but I suspect that a better way might be to add a pre_rules.mk that would allow user level overrides.

spidey3 added a commit to spidey3/qmk_firmware that referenced this pull request Aug 13, 2022
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
nolanseaton pushed a commit to nolanseaton/qmk_firmware that referenced this pull request Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants