-
Notifications
You must be signed in to change notification settings - Fork 75
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
Revamp getting started docs #404
Revamp getting started docs #404
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not bad at all @nklayman see first comments below
---------------- | ||
|
||
Make sure it boots automatically to your safe kernel. It's also recommended to test using grub-reboot to boot the test kernel, then rebooting again to make sure it goes back to the safe kernel. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt' grub-reboot a Ubuntu thing? I think on Fedora it's a different comment, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just grub2-reboot
, and I mention to use that earlier. Should I also add a note there?
************ | ||
|
||
|
||
These instructions will guide you through the process of building the SOF branch of the Linux kernel from scratch and installing it on your machine. The kernel will be installed in addition to your default kernel, so if something goes wrong you can always revert to the default kernel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should split this in two sections
a) compiling the kernel: this could be common with the ''Create a linux development environment" part
b) installing it with
b1) on your local machine
b2) or a target with ktest.pl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's a great idea to combine them. The only part in common is the dependencies (which we could just link to on the ktest page). Ktest has you clone the upstream linux, then add sof as a remote whereas this page just has you clone the sof fork directly or download a zip to make it easier. After that, nothing more is shared between the different setups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could directly clone SOF for ktest, that would make the explanations more consistent. In practice it's even better for SOF developers, where 'origin' becomes the SOF tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I redid the organization of the getting started section and merged what I could from the two docs pages. Take a look and lmk if it's good.
69f6c1d
to
4440d05
Compare
.. code-block:: bash | ||
|
||
sudo rm /boot/*-`uname -r` | ||
sudo rm -rf /lib/modules/`uname -r` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with the the warning on the previous line these commands are insanely dangerous. Replace them with a simple template:
sudo rm -rf /lib/modules/`uname -r` | |
sudo rm -rf /lib/modules/custom_kernel_version |
Thanks to TAB completion it's not just safer but even faster to type.
rm -rf
should never be "smart".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about using the output from make kernelversion
? Finding the kernel version is not trivial for people unfamiliar with this stuff. I'd like for this guide to be accessible to somewhat nontechnical people who have new hardware and need to try some unreleased code to get their audio working.
.. code-block:: bash | ||
|
||
sudo rm /boot/*-`uname -r`* | ||
sudo rm -rf /lib/modules/`uname -r` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, see above.
|
||
**Development device:** PC running Fedora 35+ or Ubuntu 20.04+. | ||
|
||
**Target device:** PC running Fedora 35+ or Ubuntu 20.04+, with secure boot disabled. If the target device is different than the development device, they must be on the same local network so you can ssh into the target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You absolutely don't need to be on the same local network to use ssh, I regularly use ktest.pl
over a VPN and an ocean (at the same time)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true indeed, I've done remote debug as well. ssh support with the relevant proxy setup is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware that you could use a VPN (or open the port over the network), but so are people that have that setup. If someone is familiar enough that they ssh over a vpn, they'll know that's an option. However, someone unfamiliar with ssh will most likely understand "same local network" whereas "you must have a network setup that allows an ssh connection between the dev device and the target" could confuse them. It could go either way, and I'm happy to elaborate a bit more if you think that's necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggested edit:
You must be able to ssh into the target device, which is typically on the same local network/VPN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone is familiar enough that they ssh over a vpn, they'll know that's an option.
This sounds like you have to configure something in ssh to use a VPN. The reality is that ssh (and ktest) do absolutely not care at all where the target is and your paragraph is giving that wrong impression.
"you must have a network setup that allows an ssh connection between the dev device and the target" could confuse them
I've never seen anyone confused by anything remotely close to this. In fact it's so obvious that you need a functioning network between A and B to connect ssh between A and B that it's not even worth mentioning.
What is/are the specific concerns here, firewalls maybe? In that case be more specific and say something like "any firewall between A and B mush allow ssh". Firewalls are found on local networks too, in fact Ubuntu and Fedora tend to have firewalling enabled by default.
Finally, the concept of a "local network" is vague and difficult to map to anything real in a large company network. Are you trying to refer to an Ethernet broadcast domain maybe?
grub-reboot 4; grub-editenv list | ||
=> saved_entry=6 | ||
=> next_entry=4 | ||
grub-reboot 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove the output here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't clear to me that the =>
indicated output until I ran the command (maybe it's a convention I'm unaware of). I could see someone pasting that whole thing into their terminal (although that probably wouldn't cause much of an issue). Also, it doesn't seem necessary to run the list command, as it doesn't output any additional information (ie the name of the entry). It's pretty clear that grub-reboot
will just set the next boot entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(maybe it's a convention I'm unaware of)
It's not, feel free to replace with a better format.
It's pretty clear that grub-reboot will just set the next boot entry.
This is a "getting familiar with something dangerous" section in a guide supposedly aimed at complete beginners so removing sample output does not make sense to me.
detection only works with a UART, not over SSH, so you will have to | ||
``Control-C`` manually when the console is not enabled. | ||
detection only works with a UART, not over SSH, so you will get an error if the console is not enabled. In this case, you'll have to | ||
``Control-C`` manually. Even though the script reports an error, the new kernel should still be installed and your target device should boot it. Check which kernel is booted by running ``uname -r``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People who have no prompt detection should simply use TEST_TYPE = install
instead of hitting Ctrl-C after an undefined amount of time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I just set that as the default since most people won't have that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move the ("proprietary") Github description to the git commit message. Especially important for explaining the "code" changes to configuration files. Plain English changes more rarely need explanations in... plain English :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nklayman, I'll create a subsequent pull request with edits and formatting changes after this one is merged, if it's OK with you.
************ | ||
|
||
|
||
These instructions will help you set up a development environment for the SOF branch of the Linux kernel. If you have dedicated test hardware you can use ktest to install it over ssh, otherwise you can install it locally on your device in addition to your default kernel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SOF branch or SOF fork?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it technically is a fork, I want to avoid scaring people by making them think they're installing some weird unofficial variant of Linux. While that's obviously not the case, some people are very protective of their systems yet know nothing about them, so I figured this is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "fork" is very poorly defined term. For Github (and many others) a "fork" is just a container of branches with no consideration about the actual content of these branches.
I agree with @nklayman that "fork" is often used for branches that are never meant to be merged back which can sound scary.
I went ahead and timed both. I cloned with The compression and download speeds are almost exactly the same (it's from the same server after all). However git has this extra step before starting the download:
I think saving 20 seconds is really not worth the extra documentation complexity. |
You overestimate the capabilities of our esteemed users, who are familiar with zip files but don't know what 'git' is. It's a legit option provided by GitHub, I think we should list the steps needed and move on. It's not an architecture discussion, even the --depth 1 is not useful for true developers. |
4440d05
to
bcd1795
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally speaking you seem to be avoiding references and keywords as much as possible. Good, step-by-step HOWTOs like this one are not supposed to completely hide all the complexity of what's happening behind the scenes and pretend it does not exist at all. They take an optimistic approach and assume that if you follow the steps exactly then you don't need to understand what ssh-agent and other tools do, not that ssh-agent does not exist at all. Step by step HOWTOs should still do some "tool name-dropping" that can be googled in case something goes wrong because there is always something that breaks no matter hard you try - especially when asking beginners to build a kernel and make their system unbootable install it.
Also things change and break all the time and in that case users must be able to at least "keep all the pieces" and show them to Google or a friend. You complained yourself in the description that "This was very out of date, had tons of errors and cost me a lot of time". Well in a few months after this PR is merged it will be out of date and broken again, and then even more a few months later.
@@ -0,0 +1,106 @@ | |||
.. _install-locally: | |||
|
|||
Install the Kernel locally |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The benefit is not to leave users completely stuck when something does not go according to plan which is very often the case. For complete beginners the mere existence of a reference is also making it clear that this guide is merely yet another one, not unique at all and that alternatives exist.
I've never seen one-line references to further info confuse anyone, that's a surprising idea. They're the most easy thing to skip when everything goes well.
************ | ||
|
||
|
||
These instructions will help you set up a development environment for the SOF branch of the Linux kernel. If you have dedicated test hardware you can use ktest to install it over ssh, otherwise you can install it locally on your device in addition to your default kernel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "fork" is very poorly defined term. For Github (and many others) a "fork" is just a container of branches with no consideration about the actual content of these branches.
I agree with @nklayman that "fork" is often used for branches that are never meant to be merged back which can sound scary.
|
||
**Development device:** PC running Fedora 35+ or Ubuntu 20.04+. | ||
|
||
**Target device:** PC running Fedora 35+ or Ubuntu 20.04+, with secure boot disabled. If the target device is different than the development device, they must be on the same local network so you can ssh into the target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone is familiar enough that they ssh over a vpn, they'll know that's an option.
This sounds like you have to configure something in ssh to use a VPN. The reality is that ssh (and ktest) do absolutely not care at all where the target is and your paragraph is giving that wrong impression.
"you must have a network setup that allows an ssh connection between the dev device and the target" could confuse them
I've never seen anyone confused by anything remotely close to this. In fact it's so obvious that you need a functioning network between A and B to connect ssh between A and B that it's not even worth mentioning.
What is/are the specific concerns here, firewalls maybe? In that case be more specific and say something like "any firewall between A and B mush allow ssh". Firewalls are found on local networks too, in fact Ubuntu and Fedora tend to have firewalling enabled by default.
Finally, the concept of a "local network" is vague and difficult to map to anything real in a large company network. Are you trying to refer to an Ethernet broadcast domain maybe?
.. code-block:: bash | ||
|
||
sudo dnf install fedpkg | ||
fedpkg clone -a kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's what Fedora recommends then great then but please cite your references, here and in elsewhere. Not just to avoid readers wondering why the Fedora section is very different from the Ubuntu section but also to make readers fully aware that this guide is just a (useful) summary of information from somewhere else and to provide a backup when this guide becomes out of date again.
grub-reboot 4; grub-editenv list | ||
=> saved_entry=6 | ||
=> next_entry=4 | ||
grub-reboot 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(maybe it's a convention I'm unaware of)
It's not, feel free to replace with a better format.
It's pretty clear that grub-reboot will just set the next boot entry.
This is a "getting familiar with something dangerous" section in a guide supposedly aimed at complete beginners so removing sample output does not make sense to me.
# Use your editor of choice. | ||
sudo emacs /etc/ssh/sshd_config | ||
|
||
Replace ``PermitRootLogin without-password`` with ``PermitRootLogin yes`` | ||
and save. | ||
Replace ``#PermitRootLogin prohibit-password`` with ``PermitRootLogin yes`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It avoids this "temporary" panic attack: "Wow, SOF wants me to make my system permanently insecure, violating my company policies!".
|
||
.. note:: | ||
|
||
If you are still prompted for a password, it's likely your distro doesn't automatically remember the proper key to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, then please name-drop ssh-agent once.
If you are still prompted for a password, it's likely your distro doesn't automatically remember the proper key to use. | |
In most cases `ssh-agent` should automatically manage your password(s). If you are still prompted for a password, it's likely your distro doesn't automatically remember the proper key to use. |
In case of a problem with ssh-agent this gives people both options:
- Your quick .ssh/config hack
- Googling "ssh-agent" and trying harder if they're willing to spend the time for a more permanent and scalable solution
detection only works with a UART, not over SSH, so you will have to | ||
``Control-C`` manually when the console is not enabled. | ||
detection only works with a UART, not over SSH, so you will get an error if the console is not enabled. In this case, you'll have to | ||
``Control-C`` manually. Even though the script reports an error, the new kernel should still be installed and your target device should boot it. Check which kernel is booted by running ``uname -r``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
bcd1795
to
a3545db
Compare
@marc-hb @ranj063 thanks for the reviews! Unless I missed something, all of your suggestions are either applied, or I left a comment explaining why I left it as-is. Let me know if there are any outstanding issues you still have. Also, I'm not really sure what this means, @marc-hb could you elaborate a bit more?
|
I didn't find the time to read everything again but after a quick look I think all my concerns were addressed, thanks! |
I'm not sure how to elaborate... do you know these are different things? Github does not care about commit messages but we do. Your Github description has a ton of useful information, your commit message is currently 3 lines long. Can you please make a useful git commit message out of the Github description? |
@nklayman what @marc-hb is saying is that we want to keep the description of your changes in plain text in the commit message, instead of the nice markup added for the PR. The reason is that we are still based more on git that github, and we won't look at the PR ever again while git log/blame are very useful tools. |
I do look at PRs again when I need it but yes: I doubt many people do that and in any case it's so much more convenient to find the information in git directly. Also, who knows what forge we'll be using 5 years from now. |
a3545db
to
f41f798
Compare
Ahh I see now, I just wasn't sure what you meant by the '("proprietary") Github description.' I've never heard it called that before. I replaced the old commit description with the one I wrote for the PR, should be good now. |
Well it was between quotes and parentheses so clearly not the main point. git is free and open-source, so data in git is forever safe. Github is different. |
f41f798
to
080924b
Compare
6c579e2
to
2f1dbb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @nklayman!
@marc-hb can I bother you to take a look at the updated version. We have quite a few people that are asking for directions to install the latest linux kernel PR #3338 and those instructions are urgently needed. Thanks! |
2f1dbb3
to
e2de6bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 3 minor comments after a fairly thorough review.
.. code-block:: bash | ||
|
||
mkdir ~/sof | ||
cd ~/sof |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SOF section defines a SOF_WORKSPACE=~/work/sof
indirection so users can always copy/paste the commands. Would be good to do this here too for consistency and to have everything in the same top-level directory.
PS: I think ~/sof/
sucks because it leads to a double ~/sof/sof/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it doesn't lead to ~/sof/sof
, as the repository is cloned to linux
. I'll still change it to use the SOF_WORKSPACE
anyways, that's a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it doesn't lead to ~/sof/sof, as the repository is cloned to linux.
The idea is to have the same SOF_WORKSPACE for both kernel and firmware.
I'll still change it to use the SOF_WORKSPACE anyways, that's a good idea.
Thanks!
@@ -20,13 +20,12 @@ Prerequisites on the target device | |||
********************************** | |||
|
|||
The target device can be any of the SOF-supported platforms, | |||
e.g. MinnowBoard, Up^2, Asus T100, Chromebooks) | |||
e.g. MinnowBoard, Up^2, Asus T100, Chromebooks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please fix line 15 in the introduction? It says "though only one branch can be checked out at a time" whereas the FIRST line of git help worktree
says "... allowing you to check out more than one branch at a time"
I still can't understand how Github can clears this at the top right but not at the bottom
- Change the organization of the getting started section to make it easier to determine which guide to follow. - Add instructions for building a custom kernel with SOF config so that it can be booted on that same device. This is useful for people without dedicated test hardware who just want to try out the latest code, such as when they are running into issues with new hardware. - Revamp the ktest setup documentation. This was very out of date and had tons of errors, especially for using Fedora. It has now been validated to work on both Fedora and Ubuntu target devices, from a Ubuntu dev machine (but Fedora should work just fine too). Signed-off-by: Noah Klayman <[email protected]>
e2de6bd
to
f137917
Compare
@anton-intel over to you now :) |
@anton-intel can we prioritize this, we have a flurry of issues reported by the community and not everyone knows how to build a kernel. We need to publish this PR in the official docs, thanks! |
Revamp getting started docs
add page for same device build/test
fix issues in ktest setup
improve organization of pages
Signed-off-by: Noah Klayman [email protected]
This PR accomplishes three things:
There are two semi-blocking issues:
The same-device docs assume that kconfig: add two scripts to update distro config to sof and sof-dev kconfig#68 is merged.(workaround found)ktest.pl
has a bug where it doesn't set theKERNEL_VERSION
when loadingsof-dev.conf
the first time it gets run. This results in an invalid configuration when it gets run for the first time, ie:More info relating to issue 2
Config:
First run:
Subsequent runs:
Note that
5.16.0-rc1test+
is not present in the first run. This causes a failed boot.Update:
Found a workaround, see https://github.com/thesofproject/sof-docs/pull/404/files#diff-00b6d5e2ac08bb291487662715996a41a742a9b9b026c0145350cd1151f7fddcR296-R297