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

Generate make dependency file during build for info.json's etc. #20451

Merged
merged 1 commit into from
May 15, 2023

Conversation

tzarc
Copy link
Member

@tzarc tzarc commented Apr 15, 2023

Description

This adds automated generation of a make dependency file, specifically tying the generated-files target to each of the "interesting" files that contribute to codegen (listed in interesting_files).

Upshot is, changes to info.json or any of the other metadata files should now properly trigger rebuilds.

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

@tzarc tzarc requested a review from a team April 15, 2023 02:47
@github-actions github-actions bot added cli qmk cli command core python labels Apr 15, 2023
@tzarc
Copy link
Member Author

tzarc commented Apr 15, 2023

Need to test configurator json files.

EDIT: works fine.

@tzarc
Copy link
Member Author

tzarc commented Apr 15, 2023

Keymaps specified in community layouts work fine as well.

Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

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

Doesn't seem to break anything, at least

@drashna drashna requested a review from a team May 6, 2023 08:12
@github-actions github-actions bot added CI documentation keyboard keymap translation via Adds via keymap and/or updates keyboard for via support labels May 14, 2023
@tzarc tzarc merged commit 507e32b into qmk:master May 15, 2023
@tzarc tzarc deleted the generated-deps branch May 15, 2023 11:58
@noroadsleft
Copy link
Member

Esoteric bug(?) caused by this PR: its changeset prevents me from compiling any QMK firmware at all:

 noroadsleft  ~/Documents/GitHub/qmk-0.10.0 ((d6f8df4be8...)|BISECTING)
$ git bisect good
507e32b28c5067fb01cb85c3259a50bec7ec1907 is the first bad commit
commit 507e32b28c5067fb01cb85c3259a50bec7ec1907
Author: Nick Brassel <[email protected]>
Date:   Mon May 15 21:58:12 2023 +1000

    Generate `make` dependency file during build for info.json's etc. (#20451)

 builddefs/build_keyboard.mk                      |  9 ++++
 lib/python/qmk/cli/__init__.py                   |  1 +
 lib/python/qmk/cli/generate/make_dependencies.py | 56 ++++++++++++++++++++++++
 3 files changed, 66 insertions(+)
 create mode 100755 lib/python/qmk/cli/generate/make_dependencies.py

 noroadsleft  ~/Documents/GitHub/qmk-0.10.0 ((d6f8df4be8...)|BISECTING)
$ git checkout 507e32b28c5067fb01cb85c3259a50bec7ec1907
Previous HEAD position was d6f8df4be8 hazel/bad_wings update  (#20947)
HEAD is now at 507e32b28c Generate `make` dependency file during build for info.json's etc. (#20451)

 noroadsleft  ~/Documents/GitHub/qmk-0.10.0 ((507e32b28c...)|BISECTING)
$ qmk compile -kb kc60 -km default -c
Ψ Compiling keymap with gmake --jobs=4 --output-sync=target kc60:default


QMK Firmware 0.20.7
Making kc60 with keymap default and target clean

Generating: .build/obj_kc60_default/src/info_deps.d                                                 [OK]
QMK Firmware 0.20.7
Making kc60 with keymap default

Generating: .build/obj_kc60_default/src/info_deps.d                                                 [OK]
avr-gcc (GCC) 5.4.0
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

gmake[1]: *** No rule to make target '/media/noroadsleft/256GB', needed by 'generated-files'.  Stop.
gmake[1]: *** Waiting for unfinished jobs....
Generating: .build/obj_kc60/src/default_keyboard.h                                                  [OK]
Generating: .build/obj_kc60/src/default_keyboard.c                                                  [OK]
Generating: .build/obj_kc60/src/info_config.h                                                       [OK]
Make finished with errors
gmake: *** [Makefile:392: kc60:default] Error 1

The line gmake[1]: *** No rule to make target '/media/noroadsleft/256GB', needed by 'generated-files'. Stop. indicates to me some sorf of failure relating to the fact that my qmk_firmware repository is actually a symbolic link that maps the actual directory /media/noroadsleft/256GB SSD/GitHub/qmk-0.10.0 to ~/Documents/GitHub/qmk-0.10.0. I set this up when I switched from Windows to Linux so I could still access my repo from Windows (my Linux drives use a file system that Windows cannot read).

@tzarc
Copy link
Member Author

tzarc commented May 16, 2023

Try:

diff --git a/lib/python/qmk/cli/generate/make_dependencies.py b/lib/python/qmk/cli/generate/make_dependencies.py
index 635c341897..63c6db0b3a 100755
--- a/lib/python/qmk/cli/generate/make_dependencies.py
+++ b/lib/python/qmk/cli/generate/make_dependencies.py
@@ -53,4 +53,4 @@ def generate_make_dependencies(cli):
     for file in interesting_files:
         found_files.extend((QMK_FIRMWARE / 'users' / cli.args.keymap).glob(f'**/{file}'))
 
-    dump_lines(cli.args.output, [f'generated-files: {found.resolve()}\n' for found in found_files])
+    dump_lines(cli.args.output, [f'generated-files: {found.resolve().relative_to(QMK_FIRMWARE)}\n' for found in found_files])

@noroadsleft
Copy link
Member

Adding .relative_to(QMK_FIRMWARE) fixes my issue. 👍

@sigprof
Copy link
Contributor

sigprof commented May 17, 2023

.relative_to(QMK_FIRMWARE) might still do a wrong thing if the source tree has symlinks which point outside of qmk_firmware (e.g., if something like tzarc/qmk_build is used to keep some code out of tree, and those files end up physically stored a directory with spaces in the path).

Maybe it would be better to just assume that qmk generate-make-dependencies is run in the qmk_firmware directory (which lib/python/qmk/constants.py assumes anyway — QMK_FIRMWARE = Path.cwd()), and use relative paths everywhere?

diff --git a/lib/python/qmk/cli/generate/make_dependencies.py b/lib/python/qmk/cli/generate/make_dependencies.py
index 635c341897..ff88853848 100755
--- a/lib/python/qmk/cli/generate/make_dependencies.py
+++ b/lib/python/qmk/cli/generate/make_dependencies.py
@@ -51,6 +51,6 @@ def generate_make_dependencies(cli):
 
     # If we have a matching userspace, include those too
     for file in interesting_files:
-        found_files.extend((QMK_FIRMWARE / 'users' / cli.args.keymap).glob(f'**/{file}'))
+        found_files.extend((Path('users') / cli.args.keymap).glob(f'**/{file}'))
 
-    dump_lines(cli.args.output, [f'generated-files: {found.resolve()}\n' for found in found_files])
+    dump_lines(cli.args.output, [f'generated-files: {found}\n' for found in found_files])

For something like qmk compile -kb dztech/dz65rgb/v2 -km mechmerlin this generates:

generated-files: keyboards/dztech/dz65rgb/v2/info.json

generated-files: keyboards/dztech/dz65rgb/v2/rules.mk

generated-files: keyboards/dztech/dz65rgb/v2/config.h

generated-files: keyboards/dztech/dz65rgb/info.json

generated-files: layouts/community/65_ansi/mechmerlin/rules.mk

generated-files: users/mechmerlin/rules.mk

generated-files: users/mechmerlin/config.h

Also there is one corner case when this code does not see some dependencies — if the keymap name is different from the folder name in users, but rules.mk contains USER_NAME := foo, the dependencies on files in users/foo won't be added.

Another potential improvement is writing something like $(wildcard .../config.h) into the deps file when the specified file is not found — this would catch the case when the file is added later.

@noroadsleft
Copy link
Member

noroadsleft commented May 17, 2023

@sigprof's .diff also fixes this issue for me.

I'm not knowledgeable enough in Python to know the best way forward here, or even to what end this PR serves (hey noroadsleft, try reading the PR description). I would very much like it fixed though, because the current state seriously hampers my ability to work on QMK - I can't test my PRs if I can't compile anything.

@tzarc
Copy link
Member Author

tzarc commented May 17, 2023

@sigprof can you make a PR with your suggestions?

shieldsd pushed a commit to shieldsd/qmk_firmware that referenced this pull request May 18, 2023
sudish added a commit to sudish/qmk_firmware that referenced this pull request May 21, 2023
* upstream/master: (52 commits)
  `qmk generate-make-dependencies` improvements (qmk#21001)
  scramble: Add XOSC delay for startup (qmk#20991)
  [Keyboard] Add Fancytech Fancyalice66 (qmk#20647)
  Define RGB_DI_PIN directly instead of using another define (qmk#20983)
  Add Cepstrum Rev. 1 Keyboard (qmk#20721)
  [Keyboard] Add Moondrop Dash75 (qmk#20890)
  Revert to last known working version (qmk#20967)
  Bump anothrNick/github-tag-action from 1.64.0 to 1.65.0 (qmk#20964)
  Update tg4x RGB LED count (qmk#20955)
  Fix EEPROM_DRIVER=legacy_stm32_flash (qmk#20457)
  Generate `make` dependency file during build for info.json's etc. (qmk#20451)
  hazel/bad_wings update  (qmk#20947)
  jotix keymap update (qmk#20902)
  Far better VSCode intellisense support using clangd. (qmk#20382)
  Add new keyboard "Ergomirage" (qmk#20655)
  [Keyboard] Add Zwag75 keyboard (qmk#20757)
  [Keyboard] Add kibou/fukuro (qmk#20771)
  [Keyboard] Add Teleport TKL (qmk#20469)
  momokai tap_* rgb modes and bootmagic key update (qmk#20126)
  [Keymap] Add caps word enable to dshields keymaps (qmk#20862)
  ...
coquizen pushed a commit to coquizen/qmk_firmware that referenced this pull request Jun 22, 2023
autoferrit pushed a commit to SpaceRockMedia/bastardkb-qmk that referenced this pull request Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI cli qmk cli command core documentation keyboard keymap python translation via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants