search

raspberrypi / pico-setup

92 stars 42 forks source link

Major rewrite of pico_setup.sh #20

Open michaelstoops opened 3 years ago

michaelstoops commented 3 years ago

This is a complete overhaul of pico-setup. I'd like to specifically point out my appreciation for the first version, which was very helpful to understand the requirements. Thank you, @liamfraser. I have also incorporated changes are in others' pull requests, but have not yet been merged. Thank you to those authors as well.

Design

The setup actions are grouped into phases. There's a fair bit going on in this script, and I think it's easier to understand with some sensible structure. The phases approximately follow the Getting Started documentation, adjusted for the non-interactive use case.

  1. Preflight check
  2. Setup minimum dev environment
  3. Setup the tutorials
  4. Setup recommended tools

This code is factored into functions, where the previous version was a straight-through script. This code certainly is longer, but should be very easy to read, understand, and modify.

Testing

This version adds some automation for testing. See test/ for details.

Documentation

This version adds README.md and LICENSE.txt. The license is copied from the pico-examples, including the American spelling of "license". :)

Compatibility

This update works with a number of OS and hardware platforms (see README.md). It should work on all up-to-date derivatives of Debian, as the key prerequisite is APT. I've tested it on a number of combinations and left notes (see test/README.md). I have have not exhaustively tested since the last modification, so it's possible that I broke something. Still, it's fair to say it supports lots of platforms.

This should resolve these issues:

Fixes for known issues

.bashrc

This version puts the environment variables in .z?profile, correctly handling both bash and zsh, which is particularly relevant as it's the default shell on macOS. https://github.com/raspberrypi/pico-setup/issues/13, https://github.com/raspberrypi/pico-setup/pull/14

Additionally, the new code handles the case where the file didn't end in a newline. https://github.com/raspberrypi/pico-setup/issues/12

Idempotence

This version is idempotent with regard to mkdir specifically (https://github.com/raspberrypi/pico-setup/issues/10, https://github.com/raspberrypi/pico-setup/pull/11) and generally picks up where it left off when rerun (https://github.com/raspberrypi/pico-setup/pull/6). If interrupted with signal or transient failure, it's fine to rerun.

Visual Studio Code

The script installs VS Code when it looks like there's a local XWindows server. https://github.com/raspberrypi/pico-setup/issues/7

But--the features around Visual Studio Code only work on Raspberry Pi OS, not Debian, Ubuntu or macOS. This is mostly because the Raspberry Pi OS repos already have VS Code and it installs nicely.

Paths with spaces

I think this version handles paths with spaces correctly, but it does download all git submodules, and one of openocd's submodules doesn't handle spaces. https://github.com/raspberrypi/pico-setup/issues/17, https://github.com/raspberrypi/pico-setup/pull/18

Features not added

UI

The design of the code should lend itself nicely to adding an interactive interface or otherwise de/selecting features, but I haven't implemented this and removed the SKIP* method because I didn't like it. I think if it's important, a better mechanism would be command line switches.

Your review is appreciated

Feedback welcome, as always. :)

michaelstoops commented 3 years ago

I'm questioning whether I should have submitted this PR. If I could get a clear ruling on whether or not the Raspberry Pi organization wants it, I'd appreciate that. If Raspberry Pi doesn't want it, please close the PR.

To be clear: I'm not meaning to be dramatic or sour-grapes about the decision. I'm just looking to keep moving forward. So please let me know. :)

Thanks, Michael

bablokb commented 3 years ago

Very nice rewrite, I will test it soon. I'm usually working on OpenSuse, and I think the clear structure and encapsulation of features makes it easy to port the script to this slightly different flavor of Linux. I do have some additional feature requests (e.g. I don't like the automated build of the examples), but I will open an issue for that in your repo.

michaelstoops commented 3 years ago

Very nice rewrite, I will test it soon. I'm usually working on OpenSuse, and I think the clear structure and encapsulation of features makes it easy to port the script to this slightly different flavor of Linux. I do have some additional feature requests (e.g. I don't like the automated build of the examples), but I will open an issue for that in your repo.

Thank you for the kind words and encouragement. 😊

What package manager are you using?

bablokb commented 3 years ago

OpenSuse uses its own manager called zypper (this is rpm based). There is a wrapper for apt on the system, this should not be a problem. But package names are usually slightly different.

liamfraser commented 3 years ago

@michaelstoops this looks really good. Thanks for your hard work. The original was just something I whipped up to stop mistakes when people were following the written instructions. From our point of view, if it's the same as our current script on a Raspbian install (so the documentation doesn't change) then I'm happy to accept it. I would just like to confirm that myself before doing so.

lurch commented 3 years ago

This is looking great @michaelstoops Please take all my comments not as a negative, but as a compliment that this is worthy of such an in-depth review :grin:

Will test it on a Pi later this evening...

@liamfraser

From our point of view, if it's the same as our current script on a Raspbian install (so the documentation doesn't change) then I'm happy to accept it.

Our docs currently say "* Define PICO_SDK_PATH, PICO_EXAMPLES_PATH, PICO_EXTRAS_PATH, and PICO_PLAYGROUND_PATH in your ~/.bashrc" however I don't think there's any good reason for setting each of those env-vars? (and of course this also now adds them to ~/.profile instead of ~/.bashrc). This is only a small docs-change though which I'm happy to make.

Hmmmm @michaelstoops given that the old pico_setup.sh wrote to ~/.bashrc, should this new script also warn if PICO_SDK_PATH is set in ~/.bashrc ?

michaelstoops commented 3 years ago

This is looking great @michaelstoops Please take all my comments not as a negative, but as a compliment that this is worthy of such an in-depth review 😁

Thanks! Don't worry, this isn't my first rodeo. 😁 Written communication can be difficult, and best read with forbearance.

Will test it on a Pi later this evening...

@liamfraser

From our point of view, if it's the same as our current script on a Raspbian install (so the documentation doesn't change) then I'm happy to accept it.

Our docs currently say "* Define PICO_SDK_PATH, PICO_EXAMPLES_PATH, PICO_EXTRAS_PATH, and PICO_PLAYGROUND_PATH in your ~/.bashrc" however I don't think there's any good reason for setting each of those env-vars? (and of course this also now adds them to ~/.profile instead of ~/.bashrc). This is only a small docs-change though which I'm happy to make.

True. I looked and couldn't find usage of the env vars other than PICO_SDK_PATH.

I do think the .bashrc bit needs to change.

Hmmmm @michaelstoops given that the old pico_setup.sh wrote to ~/.bashrc, should this new script also warn if PICO_SDK_PATH is set in ~/.bashrc ?

Indeed, that case is a bit messy. I thought through a few scenarios, and I agree that a warning is appropriate. It would be nice to fix it for the user, but in the end, we can't tell whether the user has added a hack to fix it themselves, so that path leads to madness.

I'll add a warning on the next rev.

michaelstoops commented 3 years ago

OK, here's where we are today:

I made lots of little changes per above, lightly tested, resolved the comments, and pushed a commit: https://github.com/raspberrypi/pico-setup/pull/20/commits/049289a4190aab46bd0ecbb09a13fe7f11194ffb.

If we're in agreement on the many little changes, I'll move on to the unresolved comments, which require a little more thought.

Once we've got a viable candidate for merging, I'll do a more thorough test, fix any bugs I find.

Then we'll do one last pass of review and merge. Sound good?

Thanks, Michael

lurch commented 3 years ago

I just tried running this in a Debian docker image, and it got as far as:

Entering phase 0: Preflight check
+ echo 'Verifying sudo access'
Verifying sudo access
+ sudo -v
./pico_setup.sh: line 87: sudo: command not found

I dunno if that's a standard Debian thing, or just a Debian-in-Docker thing? :shrug: I guess at the very least it's probably worth checking if the sudo command is available, and exiting with an appropriate error-message if not?

aallan commented 3 years ago

I dunno if that's a standard Debian thing, or just a Debian-in-Docker thing? 🤷 I guess at the very least it's probably worth checking if the sudo command is available, and exiting with an appropriate error-message if not?

If sudo isn't available then we have bigger problems and should probably just stop?

lurch commented 3 years ago

I side-stepped that problem by doing apt install sudo (Docker images run with the root user by default), and ran the script again, and it ran to completion :+1:

michaelstoops commented 3 years ago

I just tried running this in a Debian docker image, and it got as far as:

Entering phase 0: Preflight check
+ echo 'Verifying sudo access'
Verifying sudo access
+ sudo -v
./pico_setup.sh: line 87: sudo: command not found

I dunno if that's a standard Debian thing, or just a Debian-in-Docker thing? 🤷 I guess at the very least it's probably worth checking if the sudo command is available, and exiting with an appropriate error-message if not?

I think Docker images tend to contain as little as possible. What image were you using?

lurch commented 3 years ago

I think Docker images tend to contain as little as possible. What image were you using?

sudo docker run -it debian on my Ubuntu 18.04 x64 laptop.

lurch commented 3 years ago

FYI @michaelstoops https://github.com/msteveb/jimtcl/issues/199

michaelstoops commented 3 years ago

FYI @michaelstoops msteveb/jimtcl#199

Good guy @lurch to the rescue!

Thanks. :)

michaelstoops commented 3 years ago

I think we could support the Debian Docker image, sans sudo, by invoking sudo only if we aren't already root.

I would ask whether we want to go to such trouble to support this image, but it would enable Docker as a platform for automated builds, and I like that idea.

michaelstoops commented 3 years ago

I’m not quite following. Did I miss something?

On Apr 7, 2021, at 4:08 AM, Alasdair Allan @. @.> > wrote:

@aallan commented on this pull request.

In pico_setup.sh:

+if printenv TARGET_DIR; then

  • echo "Using target dir from \$TARGET_DIR: ${TARGET_DIR}"

Starting with macOS Catalina, Macs will now use zsh as the default login shell and interactive shell across the operating system. All newly created user accounts in macOS Catalina will use zsh by default. That said, mosts existing Mac users will be running bash. I know I am, the script will have to handle both cases.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

michaelstoops commented 3 years ago

I’ve managed to remove a few cds with git -C, and the remaining cds have been wrapped with pushd/popd, as has the script as a whole, just in case.

On Apr 7, 2021, at 4:04 AM, Andrew Scheller @. @.> > wrote:

@lurch commented on this pull request.

In pico_setup.sh:

  • Restore working directory

  • popd >> /dev/null

Would be nice to do this pushd / popd thing in the other functions too :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

michaelstoops commented 3 years ago

Status update, FYI:

This work is in a testing and test-development phase, which has some long cycles and may take a bit of wall-clock time to complete. I think the core functionality of the setup script is about where it needs to be, so moving on to testing is good progress.

I've re-framed the code that used to be called test/test as a validation process, and have integrated it into the main setup script. I did this because it really was validation of the one build, rather than a test of the code under different circumstances. I figure there's really no reason not to validate every time. Better to know if it's broken.

Testing is critical for a piece of software like this: meant to be run by end-users, working on lots of different platforms, with varying assumptions, and dependent on distinct repos that may or may not have what we expect. Not only do I need to make sure my code works this time, but it will need to be tested again after the next bug fix, the one after that, etc.

In summary, if I don't automate testing, this work will go to waste. Therefore, I'm investing effort in writing a new test suite, which runs this code through all of the supported circumstances for the host it's running on. For example, the setup script should work on bash and zsh, both on the first installation and later updates, etc. The test suite tests those scenarios.

I may or may not automate testing across host types. I am comfortable with spreading these tests out to EC2 instances, but I suspect that most members of this community aren't comfortable with EC2. If there's a more appropriate tool among the potential developers of this code, that might be better. VirtualBox comes to mind, although that isn't particularly convenient for me, running on a MacBook Air M1. None of the virtual host technologies work on M1 as far as I know.

This is all a bit more work than I meant to do, but I think it's a good cause and will make a difference to people. Probably a good piece of work for my portfolio too.

🙂 Michael

ndabas commented 3 years ago

Regarding testing, GitHub Actions might be worth considering, because they give us access to Windows, Ubuntu, and macOS hosts, and all for free -- plus, the testing can be fully integrated into the GitHub workflow itself -- when somebody submits a PR, their code will be tested on these platforms automatically. The downside is that the machines are not clean -- they have lots of dev tools preinstalled, but that will still give us some level of validation.

The only platform not covered there would be Raspberry Pi hardware itself. Another option that covers that somewhat would be Docker, which can be used to test Windows, Ubuntu, and Raspbian x86-64 (not sure how close that is to the Pi distros though.) Docker leaves out macOS though.

Personally I'd be happy with an EC2-based solution -- even though the macOS bit is a pain to manage because of the 24-hour minimum -- it's really up to the Pi folks to figure out what would work for them in the long term in terms of maintaining this code, reviewing PRs, and so on.

ndabas commented 3 years ago

This is all a bit more work than I meant to do, but I think it's a good cause and will make a difference to people. Probably a good piece of work for my portfolio too.

I think what you're doing is fantastic.

I was thinking about making some radical changes to the setup process myself, but you actually put in the hard work and produced something usable right now. Which is worth a lot more than just ideas.

What were those ideas?

bablokb commented 3 years ago

Instead of using pushd/popd, you could just run the relevant commands in a subshell (enclose commands in parenthesis ( ... )). This also has the advantage that the commands don't have any unwanted side-effects, e.g. manipulating variables.

But my suggestion is: just merge the pull request. It has been polished more than most of the other stuff for pico. And once that is done, we can have incremental pull-requests for the missing functions/cleanups/etc.

michaelstoops commented 3 years ago

I spent a couple days away from this code. I'm back but I would like to wrap it up.

As discussed, I have moved the validation code into the setup script. test/test_local.sh is now the primary test. I have tested it on Raspberry Pi OS on a Raspberry Pi 4, and Debian and Ubuntu on x85 and Graviton2. I have not tested the current state against macOS because it's expensive and I'd like to do it just once more.

The installer hasn't changed much from earlier revisions, but the testing code is new and I suspect I'll get some feedback on it. Let's do one more round of review, then do the final tests on all the platforms, and then merge the change.

aallan commented 3 years ago

But my suggestion is: just merge the pull request.

I think we were waiting for @liamfraser to run a clean room test to confirm that it performed the same as his original script.

lurch commented 3 years ago

Sorry I disappeared off the radar on this after all my earlier feedback - I had other stuff to work on. I've just tested this on a fresh Raspberry Pi OS (32-bit) SD-card (running on a Pi400), and it gets as far as:

You must run sudo reboot to finish UART setup
+ validate_uart
+ dpkg-query -s minicom
+ grep -q enable_uart=1 /boot/config.txt
+ grep -q console=serial0 /boot/cmdline.txt

and I only get a pico-sdk directory in ~/pico, with no pico-examples directory. I ran the script again, and it stopped at the same place, and when I cat ~/.profile the export "PICO_SDK_PATH=/home/pi/pico/pico-sdk" line is in there twice! :confused:

ndabas commented 3 years ago

Perhaps we can use this for testing on macOS? At least for a quick smoke test? https://github.com/sickcodes/Docker-OSX/

aallan commented 3 years ago

@michaelstoops Did you see @lurch's comment?