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

refactor: remove unused files; improve build UX #2087

Merged
merged 2 commits into from
Dec 31, 2023

Conversation

lesshonor
Copy link
Contributor

@lesshonor lesshonor commented Dec 31, 2023

The Kconfig files were mistakenly placed under boards/shields as part of #224.

Willing to break out the makefile change into a separate PR RFC-style, but IMO the "Did not find ${shield}" message here is actively unhelpful:

  • Users mistakenly latch onto it as a reason for build failures because it is the first "error" they see written in plain English, and separated from the rest of the build output by a blank line
  • It displays for all out-of-tree shields
  • It displays for in-tree shields if they are splits, or built with an out-of-tree module/config repo (e.g. my_shield nice_view_adapter nice_view)
  • The build will fail within seconds of the warning if any shield specified is actually missing
  • Per refactor: Hook into CMake loading in a better spot. #1989, it should not be WARN-worthy that a shield does not exist in the main repo when (emphasis mine):

    [An independently managed Zephyr module repository] is the preferred way to handle boards and shields moving forward in ZMK [...] ZMK does have a collection of boards/shields in the ZMK main repository, which are planned to be phased out [...]

* Among other issues, this message is often misinterpreted by users
  building out-of-tree shields -- leading them to think the shield
  "not being found" is the cause of a build failure.
@lesshonor lesshonor requested a review from a team as a code owner December 31, 2023 00:45
@caksoylar
Copy link
Contributor

I like these changes. Kconfig files do seem look the result of a mistake to me, so makes sense to remove them.

I also agree that that warning has caused a lot of confusion. I think it would be great to remove that (thanks for digging up the cause!).

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

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

Thanks. Agreed on all points.

@petejohanson petejohanson merged commit d35311a into zmkfirmware:main Dec 31, 2023
20 checks passed
@lesshonor lesshonor deleted the ux-cleanup branch December 31, 2023 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants