Phoscur / QMK-compile

Github Action to compile QMK firmware
MIT License
0 stars 3 forks source link

Dynamic Fork & Firmware Round #2 #3

Closed 5andr0 closed 6 months ago

5andr0 commented 1 year ago

actions/checkout@v3 was overwriting the workdir which also deleted the layout_src folder fix: checkout to subfolder with path: ./QMK-Compile

volumes:
      - ${GITHUB_WORKSPACE}:${GITHUB_WORKSPACE}

I decided to mount the github workspace folder at the same path to mimic the basic docker action behvaiour. Now the parent can specify the artifact path as a full path (same inside the container used by entrypoint.sh) or as a relative path because of working_dir: ${GITHUB_WORKSPACE}

I saw you wanted to add a fallback branch to firmware21 but that's the same as hardcoding the branch. IMHO it's best to keep it empty as default to clone the latest default branch from zsa, which is always stable and backwards compatible.

Here is another idea that came up: Do we want oryx download as a separate/sibling or derived GA? I think it's better as a workflow step and keep everything in one repo instead of bloating it up.

Check out my take on oryx-macro-hax I'm only extracting the relevant keymap files directly to layout_src, so you don't have to specify the layout folder every time as secret and inside the my-macros.ts . OryxId as input is enough. Also having my-illicit-macros for local edits was redundant.

Is there a reason you wanted the setup part inside the dockerfile? To my knowledge the docker image cannot be cached anyway, so you could do the setup inside entrypoint.sh where you have access to environment vars instead of build args. Then you can use the basic docker container again and safe a lot of code. See https://github.com/Phoscur/QMK-compile/pull/4

Tested with and without fork / branch parameters. Checked the build console output and flashed the firmware to my moonlander with f20 and f21

Phoscur commented 1 year ago

actions/checkout@v3 was overwriting the workdir which also deleted the layout_src folder fix: checkout to subfolder with path: ./QMK-Compile

Yeah, something like that was on my mind yesterday when I tried to fix it (just before I stopped ) ;)

Here is another idea that came up: Do we want oryx download as a separate/sibling or derived GA?

I think it's better as a workflow step and keep everything in one repo instead of bloating it up.

Mhhh I think it could become another utility outside of the "my-macros" repos, we can also discuss project architecture at a later date again when it's grown a bit more…!

Is there a reason you wanted the setup part inside the dockerfile? To my knowledge the docker image cannot be cached anyway, so you could do the setup inside entrypoint.sh where you have access to environment vars instead of build args. Then you can use the basic docker container again and safe a lot of code. See #4

I was trying to naturally (prematurely?) optimize, push things up into cached docker layers, you should be correct with GA not taking advantage of this (at least not if you build only once a week), if there is caching it is not long-lived to my experience. The Hax Pipeline takes about 2mins to run, I think that is fair.

D'accord with your more simplistic third approach in #4. I would also like to get rid of putting the layout into a secret if we have a better solution!

Check out my take on oryx-macro-hax I'm only extracting the relevant keymap files directly to layout_src, so you don't have to specify the layout folder every time as secret and inside the my-macros.ts . OryxId as input is enough. Also having my-illicit-macros for local edits was redundant.

That is so great to see, you know that the original author (@gittyeric) is no longer investing in this project? And I am not very fast on taking over, rather looking for partners to support forks for everyone! :) please leave a comment here: https://github.com/gittyeric/ergodox-macro-hax/issues/7#issue-1111385121

I'll wait a bit until our discussions have calmed down a bit, but apart from that I am looking to merge #4 asap :)

Tested with and without fork / branch parameters. Checked the build console output and flashed the firmware to my moonlander with f20 and f21

Vollkommener Test, thank you! I'd like to put this into contribution guidelines now - we don't have any yet…

Phoscur commented 6 months ago

Thank you for your work @5andr0, I've started merging your code, but I went with #4 instead of this one ;)