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/add west config path to workflow #107

Merged
merged 7 commits into from
Jul 23, 2024

Conversation

stijnveenman
Copy link
Contributor

@stijnveenman stijnveenman commented Jul 8, 2024

Follow up of (and requires): #105

Took a bit of trial and error, but we got there.

Adds a west_config_path input to the draw workflow. If specified it will initialise and update west. If not specified, it has no affect. Thus also not slowing down any workflows.

Example of an uninitialised failed workflow: https://github.com/stijnveenman/zmk-config/actions/runs/9847346357/job/27187112733#step:9:71

Example of a initialised successful workflow: https://github.com/stijnveenman/zmk-config/actions/runs/9847355106/job/27187141109

With the following settings:

draw.yml

jobs:
  draw:
    uses: stijnveenman/keymap-drawer/.github/workflows/draw-zmk.yml@feature/west-init
    permissions:
      contents: write  # allow workflow to commit to the repo
    with:
      keymap_patterns: "config/cradio.keymap"        # path to the keymaps to parse
      destination: 'artifact'
      install_repo: 'https://github.com/stijnveenman/keymap-drawer.git'
      install_branch: 'feature/additional_includes'
      config_path: 'draw-config.yaml'
      west_config_path: './config/'

draw-config.yaml

parse_config:
    zmk_additional_includes:
      - "zmk-helpers/include"

@caksoylar
Copy link
Owner

Thanks for spending the time to figure this out. I have a few of things in mind to make this a bit faster and more seamless:

  • Omit zmk from west modules to speed things up
    • This way we could also probably get rid of the module cache step
    • Something like oq could be used to delete the item as a preprocessing step if west doesn't have an option to ignore certain projects
  • Automatically enable the extra steps, conditioned on whether any non-ZMK modules are found in west.yml
    • Can again use a tool like oq to check
  • Automatically set zmk_additional_includes to the found modules
    • This would require parsing the module.yml's after west update
    • How exactly we'd want to merge the inferred value with the user's own config is an open question
    • Because of above two, I don't know if it's worth doing. In any case, it shouldn't be part of this PR

I can look into above myself building upon your work, or if you want to tackle them yourself you are welcome to, as well.

@stijnveenman
Copy link
Contributor Author

It was fun figuring out how things worked while getting used to my new keyboard :)

I had also considered some of your points, all together i personally think "messing" with the zmk/west config is not worth it, as the entire workflow (with active cache) still only takes 1 minute. Usually you are flashing your board anyway at that time.

Automatically enabling a west init based on modules in west.yml could be nice. Though technically there is no requirement for ZMK to be named ZMK i think. But that might be an edge case.

One thing i also considered, instead of passing the zmk_additional_includes manually though the config, i think its safe to assume any subdirectory that has an include folder can be added to the preprocessor. (zmk-helpers/includes is the folder we need to add). Then, if west is initialised, no extra config might be required.

@caksoylar
Copy link
Owner

Having slept on this, I might be overthinking and overgeneralizing this. In practice the modules will want to be parsed only for very few cases, right now I can only think of zmk-helpers. So yeah, maybe not having things automated and optimized is fine -- this can be documented in the project (I am thinking of utilizing the wiki for some scenarios like this one and zmk-locale-generator) and/or in zmk-helpers.

So the current state of this looks good for that. But I'll take a day or two to mull on it :)

Copy link
Owner

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

Some small tweaks.

.github/workflows/draw-zmk.yml Outdated Show resolved Hide resolved
.github/workflows/draw-zmk.yml Outdated Show resolved Hide resolved
.github/workflows/draw-zmk.yml Outdated Show resolved Hide resolved
Copy link
Owner

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

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

Thanks!

@caksoylar caksoylar merged commit 1b3f8f1 into caksoylar:main Jul 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants