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

Deprecate DEFAULT_FOLDER #23281

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
Draft

Deprecate DEFAULT_FOLDER #23281

wants to merge 2 commits into from

Conversation

zvecr
Copy link
Member

@zvecr zvecr commented Mar 14, 2024

Description

Currently DEFAULT_FOLDER serves 2 main purposes:

  1. Maintaining backwards compatibility when new keyboard revision is added
    • The existing make <vendor>/<product> can be mapped to make <vendor>/<product>/rev1
  2. Parent folders contain common configuration
    • The presence of a rules.mk creates a build target that only has partial information and cannot be built

Unfortunately an additional purpose has been adopted:

  1. Providing a shorter form for users to specify in terminal commands

Issues

While the current solution "works", there are a fair few problems this functionality causes:

  1. Many keyboards within the repo put config in incorrect locations and rely on DEFAULT_FOLDER to patch up the problems
  2. Keyboard updates can change DEFAULT_FOLDER to a new revision and not maintain backwards compatibility
  3. DEFAULT_FOLDER can be set incorrectly (eg on keyboard relocation) and fail to compile
  4. rules.mk gets parsed multiple times netting slower builds
  5. CI can fail to understand the implications of a parent file changing
  6. CI can end up building the same build target multiple times

As keyboard updates generally now require going to develop, purpose 1 is mostly mitigated.

With the introduction of keyboard.json as a build target, rules.mk can now become pure config. This solves purpose 2.

Purpose 3 can use the existing framework of keyboard aliases, but could generally target a solution more user focused (eg fuzzy matching, searching, TUI, etc).

Proposal

This PR aims to...

  • Remove the DEFAULT_FOLDER functionality
  • Maintain backwards compatibility for CLI commands

Notes

Currently this PR does change direct make invocation to now require the full keyboard revision. This will likely be staged in a later breaking changes cycle to ease the migration.

rules.mk gets parsed multiple times netting slower builds

Build is actually slower right now due to the backward compatibility...

Types of Changes

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

Issues Fixed or Closed by This PR

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).

@github-actions github-actions bot added core python cli qmk cli command dd Data Driven Changes labels Mar 14, 2024
@zvecr zvecr changed the title Deprecate DEFAULT_FOLDER Deprecate DEFAULT_FOLDER Mar 14, 2024
This was referenced Mar 15, 2024
@drashna drashna requested a review from a team March 29, 2024 04:55
@lesshonor lesshonor mentioned this pull request Jul 7, 2024
14 tasks
@zvecr zvecr force-pushed the deprecate_default_folder branch from 4453160 to af1f965 Compare August 10, 2024 13:12
VeyPatch added a commit to splitkb/qmk_firmware that referenced this pull request Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli qmk cli command core dd Data Driven Changes enhancement keyboard python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant