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

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

Closed
1 of 4 tasks
bcat opened this issue Jan 7, 2024 · 8 comments · Fixed by #22887
Closed
1 of 4 tasks

Comments

@bcat
Copy link
Contributor

bcat commented Jan 7, 2024

Feature Request Type

  • Core functionality
  • Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior

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
Copy link
Contributor Author

bcat commented Jan 7, 2024

Note that I think this is distinct from #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
Copy link
Member

tzarc commented Jan 8, 2024

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
Copy link
Contributor Author

bcat commented Jan 9, 2024

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

@tzarc
Copy link
Member

tzarc commented Jan 11, 2024

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

Copy link

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.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Apr 11, 2024
@bcat
Copy link
Contributor Author

bcat commented Apr 27, 2024

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

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Apr 28, 2024
Copy link

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.

@github-actions github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Jul 27, 2024
@bcat
Copy link
Contributor Author

bcat commented Jul 27, 2024

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

@github-actions github-actions bot removed the stale Issues or pull requests that have become inactive without resolution. label Jul 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants