qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.1k stars 38.93k forks source link

[Feature Request] Allow forcing individual community layouts in `qmk.json` for `userspace-compile` and GitHub Actions #22850

Closed bcat closed 1 month ago

bcat commented 9 months ago

Feature Request Type

Description

See QMK Discord discussion for additional context.

The userspace build infra used by qmk userspace-compile and the default GitHub Actions setup seems to treat each keyboard to compile as a pair of keyboard name and keymap name. This leads to problems when one has multiple keyboards built with the same PCB but different community layouts.

For example, I have a dz60 PCB with the 60_ansi_split_bs_rshift layout and one with the 60_tsangan_hhkb. My current handwritten build wrapper script compiles both by passing the FORCE_LAYOUT environment variable to make. I'd like to start using qmk userspace-compile instead, but qmk userspace-add only lets me set up a qmk.json file like this:

{
    "userspace_version": "1.0",
    "build_targets": [
        ["dz60", "bcat"]
    ]
}

There doesn't seem to be a way to build the dz60 with a particular community layout, nor to build it twice with different community layouts. I would suggest an amended qmk.json format like this instead that allowed specifying a particular FORCE_LAYOUT value for each firmware file to build:

{
    "userspace_version": "1.0",
    "build_targets": [
        {"kb": "dz60", "km": "bcat", "layout": "60_ansi_split_bs_rshift"},
        {"kb": "dz60", "km": "bcat", "layout": "60_tsangan_hhkb"}
    ]
}
bcat commented 9 months ago

Note that I think this is distinct from https://github.com/qmk/qmk_firmware/issues/22815. That issue seems to be about the existing ability to pass force layout to qmk compile not working, but even if that bug was fixed, it still wouldn't address this FR. (Passing -e FORCE_LAYOUT=foo to qmk userspace-compile would try to force the same community layout for every keyboard, which is probably not what anybody wants.)

tzarc commented 9 months ago

I think it'd be more likely we'd end up with per-keyboard environment variables added when invoking qmk userspace-add, with the usual -e FORCE_LAYOUT=xxx at that point -- resulting in something like this for the qmk.json file:

{
    "userspace_version": "1.1",
    "build_targets": [
        [
            "dz60",
            "bcat",
            [
                ["FORCE_LAYOUT", "60_ansi_split_bs_rshift"]  /* override layout */
            ]
        ],
        [
            "dz60",
            "bcat",
            [
                ["FORCE_LAYOUT", "60_tsangan_hhkb"]  /* override layout */
            ]
        ],
        ["40percentclub/luddite", "default"], /* no env vars, as per v1.0 */
        [
            "40percentclub/luddite",
            "default",
            [   /* multiple env vars added during build */
                ["FORCE_LAYOUT", "60_ansi"],
                ["CONVERT_TO", "proton_c"],
                ["BOOTLOADER", "tinyuf2"]
            ]
        ]
    ]
}
bcat commented 9 months ago

Ah, yeah, that would address my use case, and it's more flexible too.

tzarc commented 9 months ago

It'd be great if you could give #22887 a go to see if it addresses your issue.

github-actions[bot] commented 5 months ago

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs. For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

bcat commented 5 months ago

Not stale, and has a fix in flight. Thanks @tzarc! :)

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in the next 30 days unless it is tagged properly or other activity occurs. For maintainers: Please label with bug, in progress, on hold, discussion or to do to prevent the issue from being re-flagged.

bcat commented 2 months ago

Still not stale, and I can confirm PR #22887 seems to work (though I haven't pulled in changes from HEAD for a little while).