zmkfirmware / zmk

ZMK Firmware Repository
https://zmk.dev/
MIT License
2.48k stars 2.56k forks source link

feat(docs): Improve the toolchain setup page #2272

Closed Nick-Munnich closed 1 month ago

Nick-Munnich commented 2 months ago

Making the changes discussed on Discord. In particular:

Further improvement could be made by identifying a better way to handle Raspberry OS, since it is an outlier, but I feel that this is good enough for now.

Nick-Munnich commented 1 month ago

Performed a larger redo of this by combining its content with that found in #437

This updates the approach used there to the modern docs, and continues to leverage the Zephyr docs. The previous comments were both resolved in the process, but another careful look is likely due.

Note that there are some blocks written in #437 which I was uncertain about including, so I'm listing them here:

:::note Windows Users Please note the ZMK builds run slower (up to 3-5 minutes on slower hardware) with Docker on Windows if you don't use the WSL2 filesystem to store files. If you run into performance problems you can checkout the ZMK sources inside a WSL2 environment and run code . to open the sources. This can make builds run at near-native speed.

This approach will also need the Remote - WSL extension installed in VS Code.

Files stored within WSL2 can be accessed via Windows Explorer by navigating to \\wsl$. :::

I don't know how accurate this is, and Zephyr does not recommend WSL, so I'm not sure this should be included.

:::danger The Docker environment will NOT run on arm CPUs like the Raspberry Pi or Apple Silicon. You must use the native environment if using an arm CPU. :::

Googling this seems to imply that there are workarounds, but I do not have access to a device to test it out. It is not included under the current docs, so I don't see a reason to include it until it is brought up by someone.

:::caution Windows Users If you're using the Docker environment on Windows, you must checkout the sources to a folder within C:\Users[your_user_here] to avoid a potential permissions issue.

If you're using the WSL2 filesystem the sources should go under ~/ to avoid potential permissions issues. :::

I think this is true, but I have not tested. I've included it anyway, as it certainly does not hurt.

Thoughts on these would be appreciated. I am aware that there is duplicated content in the Native section - this is because I thought it looked nicer that way, but would be willing to change it upon request.

infused-kim commented 1 month ago

I can confirm that it’s working fine on Apple Silicon.

I don’t know if the zmk images have ARM versions. So it might be running using emulation, but it works just fine on Apple silicon and doesn’t need any special configuration.

caksoylar commented 1 month ago

FYI I'll try to review this weekend.

caksoylar commented 1 month ago

To address the points in your comment:

:::note Windows Users [...] I don't know how accurate this is, and Zephyr does not recommend WSL, so I'm not sure this should be included.

While the WSL un-recommendation doesn't apply to most ZMK users (because of uf2 flashing) I wouldn't put any WSL-specific instructions in our docs anyway. Users can simply follow the corresponding Linux distro instructions.

:::danger The Docker environment will NOT run on arm CPUs like the Raspberry Pi or Apple Silicon. You must use the native environment if using an arm CPU. ::: Googling this seems to imply that there are workarounds, but I do not have access to a device to test it out. It is not included under the current docs, so I don't see a reason to include it until it is brought up by someone.

I agree, it shouldn't be our responsibility to teach this to users.

:::caution Windows Users If you're using the Docker environment on Windows, you must checkout the sources to a folder within C:\Users[your_user_here] to avoid a potential permissions issue. [...] I think this is true, but I have not tested. I've included it anyway, as it certainly does not hurt.

I don't think this is very necessary either, but we can keep it I guess.

Thoughts on these would be appreciated. I am aware that there is duplicated content in the Native section - this is because I thought it looked nicer that way, but would be willing to change it upon request.

Like I noted in the review, I'd take the worse look over the maintenance difficulty.

Nick-Munnich commented 1 month ago

Did the necessary refactoring to make maintenance easier at the cost of appearance.

Two points from the previous review are not marked as resolved, the rest should be fine now I think.

EDIT: Also, my apologies for the increased review difficulty! Thanks for putting in the time. My hope is that by doing this refactoring into multiple pages, future improvements (like devcontainers) will be easier to review, making up for this.

Nick-Munnich commented 1 month ago

I deduplicated the glob tab, but in a slightly awkward way by getting rid of the function exporting the OsTabs environment. glob needing two tabs on 1 and 4 looks horrible, so I introduced a css class to just hide the second tab menu while keeping the switching functionality. There ought to be be a better way to do this, but I don't know if or how.

Tried integrating the bullet points, I quite like it.

Nick-Munnich commented 1 month ago

I re-added the missing information and made the information in the ubuntu section a bit nicer too -- this is exactly what "note" blocks are for, imo. I have no idea if the powershell script works either, though.

caksoylar commented 1 month ago

I'll do a final pass and if everything's good I'll merge tomorrow, unless anyone has objections.