qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

Userspace: add support for adding environment variables during build

Open tzarc opened this issue 2 years ago • 14 comments

Description

A few complaints on Discord and the like for not being able to specify things like CONVERT_TO and FORCE_LAYOUT when using userspace.

This adds support for the usual -e/--env parameters to qmk userspace-add and qmk userspace-remove, and ensures such extra environment variables are propagated across to each build.

Userspace repo version is bumped to 1.1 as a result of the addition to the file format.

Types of Changes

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

FIxes

  • Fixes #22850
  • Fixes #22590

Checklist

  • [x] My code follows the code style of this project: C, Python
  • [x] 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.
  • [x] I have read the CONTRIBUTING document.
  • [ ] I have added tests to cover my changes.
  • [x] I have tested the changes and verified that they work and don't break anything (as well as I can manage).

tzarc avatar Jan 11 '24 10:01 tzarc

Thanks for this change! I tested it out with my userspace, and here are my findings:

  1. The userspace-add and userspace-remove changes seem to work as expected. Here's the qmk.json file I ended up with:

    {
        "userspace_version": "1.1",
        "build_targets": [
            ["9key", "bcat"],
            [
                "ai03/polaris",
                "bcat",
                [
                    ["FORCE_LAYOUT", "60_tsangan_hhkb"]
                ]
            ],
            [
                "cannonkeys/an_c",
                "bcat",
                [
                    ["FORCE_LAYOUT", "60_tsangan_hhkb"]
                ]
            ],
            [
                "cannonkeys/instant60",
                "bcat",
                [
                    ["FORCE_LAYOUT", "60_tsangan_hhkb"]
                ]
            ],
            [
                "crkbd/rev1",
                "bcat",
                [
                    ["FORCE_LAYOUT", "split_3x6_3"]
                ]
            ],
            [
                "dz60",
                "bcat",
                [
                    ["FORCE_LAYOUT", "60_ansi_split_bs_rshift"]
                ]
            ],
            [
                "dz60",
                "bcat",
                [
                    ["FORCE_LAYOUT", "60_tsangan_hhkb"]
                ]
            ],
            ["eco/rev2", "bcat"],
            [
                "kbdfans/kbd67/hotswap",
                "bcat",
                [
                    ["FORCE_LAYOUT", "65_ansi_blocker_split_bs"]
                ]
            ],
            ["keebio/bdn9/rev1", "bcat"],
            ["keebio/quefrency/rev1", "bcat"],
            ["lily58/rev1", "bcat"],
            ["yanghu/unicorne/f411", "bcat"]
        ]
    }
    
  2. The qmk userspace-compile command succeeds. However, despite running a separate build for each (keyboard, keymap, layout) combo, it doesn't seem to actually use the specified layouts:

    $ qmk userspace-compile -c
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    QMK Firmware 0.23.5
    Deleting .build/ ... done.
    Build 9key:bcat                                                        [OK]
    Build ai03/polaris:bcat                                                [OK]
    Build cannonkeys/an_c:bcat                                             [OK]
    Build cannonkeys/instant60:bcat                                        [OK]
    Build crkbd/rev1:bcat                                                  [OK]
    Build dz60:bcat                                                        [OK]
    Build dz60:bcat                                                        [OK]
    Build eco/rev2:bcat                                                    [OK]
    Build kbdfans/kbd67/hotswap:bcat                                       [OK]
    Build keebio/bdn9/rev1:bcat                                            [OK]
    Build keebio/quefrency/rev1:bcat                                       [OK]
    Build lily58/rev1:bcat                                                 [OK]
    Build yanghu/unicorne/f411:bcat                                        [OK]
    

    Notice that there's only one dz60 firmware output, not two as expected. And none of the firmware filenames have a layout suffix:

    $ ls -l *.{bin,hex}
    -rw-r--r-- 1 bcat bcat 29697 Jan 11 15:02 9key_bcat.hex
    -rw-r--r-- 1 bcat bcat 49166 Jan 11 15:00 ai03_polaris_bcat.hex
    -rwxr-xr-x 1 bcat bcat 31520 Jan 11 15:00 cannonkeys_an_c_bcat.bin
    -rwxr-xr-x 1 bcat bcat 31532 Jan 11 15:00 cannonkeys_instant60_bcat.bin
    -rw-r--r-- 1 bcat bcat 77540 Jan 11 15:00 crkbd_rev1_bcat.hex
    -rw-r--r-- 1 bcat bcat 49088 Jan 11 15:00 dz60_bcat.hex
    -rw-r--r-- 1 bcat bcat 45525 Jan 11 15:00 eco_rev2_bcat.hex
    -rw-r--r-- 1 bcat bcat 31043 Jan 11 15:00 kbdfans_kbd67_hotswap_bcat.hex
    -rw-r--r-- 1 bcat bcat 49125 Jan 11 15:00 keebio_bdn9_rev1_bcat.hex
    -rw-r--r-- 1 bcat bcat 54472 Jan 11 15:00 keebio_quefrency_rev1_bcat.hex
    -rw-r--r-- 1 bcat bcat 54967 Jan 11 15:00 lily58_rev1_bcat.hex
    -rwxr-xr-x 1 bcat bcat 78360 Jan 11 15:01 yanghu_unicorne_f411_bcat.bin
    
  3. Compare to the expected firmware file output with a separate output file per layout:

    $ ls -l *.{bin,hex}
    -rw-r--r-- 1 bcat bcat 29697 Jan 11 15:03 9key_bcat.hex
    -rw-r--r-- 1 bcat bcat 49166 Jan 11 15:03 ai03_polaris_bcat_60_tsangan_hhkb.hex
    -rwxr-xr-x 1 bcat bcat 31520 Jan 11 15:03 cannonkeys_an_c_bcat_60_tsangan_hhkb.bin
    -rwxr-xr-x 1 bcat bcat 31532 Jan 11 15:03 cannonkeys_instant60_bcat_60_tsangan_hhkb.bin
    -rw-r--r-- 1 bcat bcat 77540 Jan 11 15:03 crkbd_rev1_bcat_split_3x6_3.hex
    -rw-r--r-- 1 bcat bcat 49088 Jan 11 15:03 dz60_bcat_60_ansi_split_bs_rshift.hex
    -rw-r--r-- 1 bcat bcat 49088 Jan 11 15:03 dz60_bcat_60_tsangan_hhkb.hex
    -rw-r--r-- 1 bcat bcat 45525 Jan 11 15:03 eco_rev2_bcat.hex
    -rw-r--r-- 1 bcat bcat 31043 Jan 11 15:03 kbdfans_kbd67_hotswap_bcat_65_ansi_blocker_split_bs.hex
    -rw-r--r-- 1 bcat bcat 49125 Jan 11 15:03 keebio_bdn9_rev1_bcat.hex
    -rw-r--r-- 1 bcat bcat 54472 Jan 11 15:03 keebio_quefrency_rev1_bcat.hex
    -rw-r--r-- 1 bcat bcat 54967 Jan 11 15:03 lily58_rev1_bcat.hex
    -rwxr-xr-x 1 bcat bcat 78360 Jan 11 15:04 yanghu_unicorne_f411_bcat.bin
    
  4. Doing a dry run, it seems the environment variables aren't being passed to qmk compile:

    $ qmk userspace-compile -n
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Preparing target list...
    Ψ Compilation targets:
    Ψ qmk compile -kb 9key -km bcat
    Ψ qmk compile -kb ai03/polaris -km bcat
    Ψ qmk compile -kb cannonkeys/an_c -km bcat
    Ψ qmk compile -kb cannonkeys/instant60 -km bcat
    Ψ qmk compile -kb crkbd/rev1 -km bcat
    Ψ qmk compile -kb dz60 -km bcat
    Ψ qmk compile -kb dz60 -km bcat
    Ψ qmk compile -kb eco/rev2 -km bcat
    Ψ qmk compile -kb kbdfans/kbd67/hotswap -km bcat
    Ψ qmk compile -kb keebio/bdn9/rev1 -km bcat
    Ψ qmk compile -kb keebio/quefrency/rev1 -km bcat
    Ψ qmk compile -kb lily58/rev1 -km bcat
    Ψ qmk compile -kb yanghu/unicorne/f411 -km bcat
    

Thanks again for the help so far! Let me know if there's more testing I can do. :)

bcat avatar Jan 11 '24 21:01 bcat

Thanks for that -- the testing I'd done was with CONVERT_TO, which seemed to be working fine when I ran it. Guess FORCE_LAYOUT is a special snowflake.

Can I get you to attach .build/parallel_kb_builds.mk from after a qmk userspace-compile, please? Dry run is "fake" and it doesn't actually run the commands internally -- takes too long and doesn't parallelise well. That said, I'll make sure to include the extra vars in its output in the final mergeable PR.

tzarc avatar Jan 11 '24 21:01 tzarc

Sure, attached (and renamed to make GitHub happy): parallel_kb_builds.mk.txt

If I'm reading this correctly, FORCE_LAYOUT=foo is getting passed along at least part of the way. I wonder if now I'm just hitting bug #22815 ?

What's interesting is that compiling with FORCE_LAYOUT using make directly in the userspace (like FORCE_LAYOUT=60_tsangan_hhkb make dz60:bcat) does build the specified layout and use the right filename.

bcat avatar Jan 11 '24 21:01 bcat

I wonder if now I'm just hitting bug https://github.com/qmk/qmk_firmware/issues/22815 ?

That would be my assumption.

FORCE_LAYOUT issue is somewhat due to locate_keymap in keymap.py where it processes community_layouts without considering the value of FORCE_LAYOUT.

https://github.com/qmk/qmk_firmware/issues/22815#issuecomment-1880090905

Chaining the context through would be possible, but messy (as that function is also used in other places or can come directly from the environment).

zvecr avatar Jan 11 '24 21:01 zvecr

Random side note: Without understanding the underlying implementation details, as a user I feel like it would be nice if layouts were more a "first class" concept in keymap selection. Being able to just say qmk compile -kb dz60 -km bcat -layout 60_tsangan_hhkb, or something like that. I understand this may be nontrivial to do, though.

(Though maybe that doesn't make sense, because someone might have two keyboards with the same layout but still want to configure them differently, and build the whole shebang via userspace infra. So actually, on further thought, maybe the environment variables are better after all, even if they're more verbose.)

bcat avatar Jan 11 '24 21:01 bcat

Is the usage of nested arrays in the JSON representation for the environment variables intentional? I would expect an object there instead:

        [
            "ai03/polaris",
            "bcat",
            {
                "FORCE_LAYOUT": "60_tsangan_hhkb"
            }
        ]

(Maybe even the outer level should be an object with keys like keyboard, keymap, environment; that would be more verbose than the existing format though.)

sigprof avatar Jan 11 '24 21:01 sigprof

Is the usage of nested arrays in the JSON representation for the environment variables intentional? I would expect an object there instead:

Tomato/tomato. Will swap it over, it's of no real consequence.

tzarc avatar Jan 12 '24 09:01 tzarc

@bcat -- would you mind giving the current branch a try, with qmk.json as the following? (it's just updated to new format with objects instead of arrays)

{
    "userspace_version": "1.1",
    "build_targets": [
        ["9key", "bcat"],
        [
            "ai03/polaris",
            "bcat",
            {
                "FORCE_LAYOUT": "60_tsangan_hhkb"
            }
        ],
        [
            "cannonkeys/an_c",
            "bcat",
            {
                "FORCE_LAYOUT": "60_tsangan_hhkb"
            }
        ],
        [
            "cannonkeys/instant60",
            "bcat",
            {
                "FORCE_LAYOUT": "60_tsangan_hhkb"
            }
        ],
        [
            "crkbd/rev1",
            "bcat",
            {
                "FORCE_LAYOUT": "split_3x6_3"
            }
        ],
        [
            "dz60",
            "bcat",
            {
                "FORCE_LAYOUT": "60_ansi_split_bs_rshift"
            }
        ],
        [
            "dz60",
            "bcat",
            {
                "FORCE_LAYOUT": "60_tsangan_hhkb"
            }
        ],
        ["eco/rev2", "bcat"],
        [
            "kbdfans/kbd67/hotswap",
            "bcat",
            {
                "FORCE_LAYOUT": "65_ansi_blocker_split_bs"
            }
        ],
        ["keebio/bdn9/rev1", "bcat"],
        ["keebio/quefrency/rev1", "bcat"],
        ["lily58/rev1", "bcat"],
        ["yanghu/unicorne/f411", "bcat"]
    ]
}

tzarc avatar Jan 19 '24 10:01 tzarc

Actually... still screwy. FORCE_LAYOUT and the CLI definitely don't mix all that well, as alluded to on #22815's comments.

tzarc avatar Jan 19 '24 10:01 tzarc

Okay, your userspace seems to now build both, as far as command line args and cflags.txt are concerned. Would be great to verify.

image

tzarc avatar Jan 19 '24 11:01 tzarc

Sorry for the delayed response! Agreed, although userspace-compile doesn't say it's using the correct layouts, the output file names seem to confirm that it is:

$ qmk userspace-compile -c -j "$(nproc)"
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
Ψ Preparing target list...
QMK Firmware 0.23.5
Deleting .build/ ... done.
Build 9key:bcat                                                        [OK]
Build keebio/bdn9/rev1:bcat                                            [OK]
Build kbdfans/kbd67/hotswap:bcat                                       [OK]
Build lily58/rev1:bcat                                                 [OK]
Build keebio/quefrency/rev1:bcat                                       [OK]
Build eco/rev2:bcat                                                    [OK]
Build ai03/polaris:bcat                                                [OK]
Build crkbd/rev1:bcat                                                  [OK]
Build dz60:bcat                                                        [OK]
Build dz60:bcat                                                        [OK]
Build cannonkeys/an_c:bcat                                             [OK]
Build cannonkeys/instant60:bcat                                        [OK]
Build yanghu/unicorne/f411:bcat                                        [OK]

$ ls -l *.{bin,hex}
-rw-r--r-- 1 bcat bcat 29685 Jan 27 12:53 9key_bcat.hex
-rw-r--r-- 1 bcat bcat 49035 Jan 27 12:53 ai03_polaris_bcat_60_tsangan_hhkb.hex
-rwxr-xr-x 1 bcat bcat 31472 Jan 27 12:53 cannonkeys_an_c_bcat_60_tsangan_hhkb.bin
-rwxr-xr-x 1 bcat bcat 31484 Jan 27 12:53 cannonkeys_instant60_bcat_60_tsangan_hhkb.bin
-rw-r--r-- 1 bcat bcat 77536 Jan 27 12:53 crkbd_rev1_bcat_split_3x6_3.hex
-rw-r--r-- 1 bcat bcat 48974 Jan 27 12:53 dz60_bcat_60_ansi_split_bs_rshift.hex
-rw-r--r-- 1 bcat bcat 48974 Jan 27 12:53 dz60_bcat_60_tsangan_hhkb.hex
-rw-r--r-- 1 bcat bcat 45529 Jan 27 12:53 eco_rev2_bcat.hex
-rw-r--r-- 1 bcat bcat 31039 Jan 27 12:53 kbdfans_kbd67_hotswap_bcat_65_ansi_blocker_split_bs.hex
-rw-r--r-- 1 bcat bcat 49011 Jan 27 12:53 keebio_bdn9_rev1_bcat.hex
-rw-r--r-- 1 bcat bcat 54930 Jan 27 12:53 keebio_quefrency_rev1_bcat.hex
-rw-r--r-- 1 bcat bcat 54959 Jan 27 12:53 lily58_rev1_bcat.hex
-rwxr-xr-x 1 bcat bcat 78356 Jan 27 12:53 yanghu_unicorne_f411_bcat.bin

I will try flashing the firmwares later this weekend and make sure everything still works, but so far so good! Thank you!!

bcat avatar Jan 27 '24 18:01 bcat

@bcat any luck on confirmation?

tzarc avatar Feb 15 '24 10:02 tzarc

@bcat any luck on confirmation?

Sorry for the long delay! Yes, I've flashed the firmware files built with this PR and they seem correct to me. Thanks again!

bcat avatar Mar 24 '24 18:03 bcat

Thanks mate. Needs some cleanup before it's good to go, will see if can get it sorted over the next week or so.

tzarc avatar Mar 26 '24 11:03 tzarc

Thank you for your contribution! This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready. For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

github-actions[bot] avatar Jun 11 '24 01:06 github-actions[bot]