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

[Core] RISC-V toolchain and picolibc fixes #15109

Merged
merged 2 commits into from
Nov 20, 2021

Conversation

KarlK90
Copy link
Member

@KarlK90 KarlK90 commented Nov 10, 2021

Description

This PR contains multiple fixes for the upcoming develop to master merge.

  1. Debian and Distributions based on it ship the needed RISC-V toolchain beginning with Debian 11+ and Ubuntu 21.04+. Therefore the qmk install script checks if the needed dependencies are available.
  2. QMK CLI setup: RISC-V toolchains are not widely available at the moment and the naming is not unified. The main userbase of QMK are AVR and ARM mcus therefore RISC-V should not be treated as a hard requirement. Therefore a missing toolchain produces no error but merely a warning.
  3. picolibc usage with heap allocation: picolibc internally uses __heap_start and __heap_end instead of the defacto chibios linker script standard __heap_base__ and __heap_end__ therefore we introduce these symbols as an alias. Usually all memory used within QMK is statically allocated, but some algorithms make usage of malloc and friends. Also the timeval struct is not defined by picolibc for syscalls, therefore it is declared as a stub.

Types of Changes

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

Issues Fixed or Closed by This PR

Checklist

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

@github-actions github-actions bot added cli qmk cli command core python labels Nov 10, 2021
@KarlK90 KarlK90 force-pushed the fix/riscv-dependencies-picolibc branch from cd1865b to a8cf0e0 Compare November 10, 2021 16:22
@KarlK90 KarlK90 marked this pull request as ready for review November 10, 2021 16:22
@drashna drashna requested a review from a team November 10, 2021 16:25
util/install/debian.sh Outdated Show resolved Hide resolved
util/install/debian.sh Outdated Show resolved Hide resolved
@KarlK90 KarlK90 force-pushed the fix/riscv-dependencies-picolibc branch from a8cf0e0 to 88825aa Compare November 11, 2021 11:25
Copy link
Contributor

@sigprof sigprof left a comment

Choose a reason for hiding this comment

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

I tested the PR in these environments (using WSL):

  • Debian 10
  • Debian 11
  • Ubuntu 18.04 (but the provided Python 3.6.9 is too old, needed to use ppa:deadsnakes/ppa and a venv to get QMK CLI working)
  • Ubuntu 20.04

The PR makes the QMK installation process work on all the above distros (only Ubuntu 18.04 needs some workarounds due to obsolete Python, but that's completely unrelated). So the changes are good, except for the apt-get update problem (which prevents ./util/qmk_install.sh from working on Debian-derived distros when invoked manually, but does not manifest when qmk doctor invokes that script with the -y option) and a useless warning for the missing riscv32-unknown-elf-gcc.

util/install/debian.sh Outdated Show resolved Hide resolved
Comment on lines 167 to 160
if warn:
cli.log.warning("{fg_yellow}Can't find %s in your path.", command)
Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with showing the warning here is that it can appear on a perfectly good configuration (Debian 11):

Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.0.0
Ψ QMK home: …/qmk_firmware
Ψ Detected Linux (WSL).
Ψ Git branch: test-pr-15109
⚠ Can't find riscv32-unknown-elf-gcc in your path.
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 8.3.1
Ψ Found avr-gcc version 5.4.0
Ψ Found avrdude version 6.3-20171130
Ψ Found dfu-util version 0.9
Ψ Found dfu-programmer version 0.6.1
Ψ Found riscv64-unknown-elf-gcc version 8.3.0
Ψ Submodules are up to date.
Ψ QMK is ready to go

So maybe the proper way to handle this is to remove the warning from here and add some code to print warnings only if no RISC-V toolchain has been found at all.

Also a missing RISC-V toolchain won't trigger a prompt to install dependencies (e.g., after updating the qmk_firmware code past the breaking change point); the user would need to run ./util/qmk_install.sh manually to install the RISC-V toolchain if it's actually provided by the distro. This is probably OK though.

Copy link
Member Author

@KarlK90 KarlK90 Nov 15, 2021

Choose a reason for hiding this comment

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

Oh right I didn't thought about that case as it didn't appear to me because I have both the embecosm riscv32-unknown-elf-gcc and Debian packaged riscv64-unknown-elf-gcc toolchain installed on my machine. Instead of adding more logic to the check script I just removed the riscv32-unknown-elf-gcc check as we really only "support" the Debian path at the moment. So as long as other distribution lack the support for RISC-V toolchains this should be good. Hopefully the nomenclature on how the gcc executables are named will converge in the future. At the moment there is riscv64-elf-gcc (arch) and riscv64-unknown-elf-gcc (debian) among others.

Copy link
Contributor

Choose a reason for hiding this comment

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

And on NixOS I have riscv32-none-elf-gcc (apparently they use none instead of unknown in such toolchains); their stock build did not work because of hardfloat/softfloat mismatch in libraries (multilib seems to be broken).

@KarlK90
Copy link
Member Author

KarlK90 commented Nov 15, 2021

@sigprof thanks for the review, feel free to resolve the conversations if you are fine with the solutions.

@tzarc tzarc requested a review from sigprof November 17, 2021 21:25
@drashna
Copy link
Member

drashna commented Nov 19, 2021

Some merge conflicts.

The risc-v toolchain is only available on distributions based on Debian 11+
so we check for their availability before installing them.
picolibc internally uses __heap_start and __heap_end instead of the
defacto chibios linker script standard __heap_base__ and __heap_end__
therefore we introduce these symbols as an alias. Usually all memory
used within QMK is statically allocated, but some algorithms make usage
of malloc and friends.

Also the timeval struct is not defined by picolibc for syscalls, therefore it
is declared as stub.
@zvecr
Copy link
Member

zvecr commented Nov 20, 2021

Due to the upcoming code freeze, I've dropped 9121bf6c4fd8c0804c280b645b1d3754615ed711 as there are still open discussions around its implementation.

Feel free to PR the changes separately. However given the warning would be presented to a fair few users, I can see the concept of "optional" dependencies needing more fleshing out.

@zvecr zvecr force-pushed the fix/riscv-dependencies-picolibc branch from e8b6250 to f4af6d3 Compare November 20, 2021 20:03
@github-actions github-actions bot removed python cli qmk cli command labels Nov 20, 2021
@zvecr zvecr merged commit 5c2052f into qmk:develop Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants