Install script improvements #1456

EmDash00 · 2024-10-11T19:40:36Z

Various improvements to the installation script. Main feature is to allow specification of the version of PhotonVision to install. Various small backend improvements. Made to address Issue #1452 so that older versions of PhotonVision may be installed for older versions of WPILibPi, allowing users more flexibility.

crschardt · 2024-10-12T23:00:48Z

Thanks for these updates. What platforms have you tested the new script on?

mcm001 · 2024-10-12T23:41:39Z

Yeah high level this looks fantastic! Maybe we should finally commit to migrating this script over to the other repo too

crschardt · 2024-10-12T23:50:30Z

Yeah high level this looks fantastic! Maybe we should finally commit to migrating this script over to the other repo too

I'm working on a PR for that and the document update.

scripts/install.sh

crschardt · 2024-10-17T02:49:47Z

scripts/install.sh

-  echo ""
-  echo "Photonvision uses NetworkManager to control networking on your device."
+
+if [[ "$DISTRO" != "Ubuntu" || -n "$DISABLE_NETWORKING" || -n "$QUIET" ]] ; then


I think that the logic here will override --install-nm=yes if the --quiet flag is also passed. That's not the current behavior and could cause unexpected results.

Ah yes you're right. Let me fix that.

…them anymore

EmDash00 · 2024-10-17T22:47:09Z

Tried to address some of the issues in the most recent version. I also caught a few typos. I'll probably try to test them tonight. What would you need from me in order to get this merged?

mcm001 · 2024-10-17T23:06:51Z

We'd wanna test it on all supported platforms is all. Maybe it makes more sense to move this all into photon image modifier now for testing and generating images?

crschardt · 2024-10-17T23:23:56Z

We'd wanna test it on all supported platforms is all. Maybe it makes more sense to move this all into photon image modifier now for testing and generating images?

I copied an earlier version of the script over and the images build. I still need to test them on coprocessors. We could merge the PR here once we've tested for manual install. After that, I'll pull it over to photon-image-modifier and PR the changes to remove it from here.

crschardt · 2024-10-17T23:48:46Z

scripts/install.sh

+# Quiet makes the default "no" instead of "ask".
+if [[ -n "$QUIET" && "$INSTALL_NETWORK_MANAGER" == "ask" ]] ; then
+  INSTALL_NETWORK_MANAGER="no"
+fi

 DISTRO=$(lsb_release -is)
-if [[ "$DISTRO" = "Ubuntu" && "$INSTALL_NETWORK_MANAGER" != "true" && -z "$QUIET" && -z "$DISABLE_NETWORKING" ]]; then
-  echo ""
-  echo "Photonvision uses NetworkManager to control networking on your device."
+
+# NetworkManager cannot be installed on non-Ubuntu distros
+# --no-networking takes precedence over --install-nm
+if [[ "$DISTRO" != "Ubuntu" || -n "$DISABLE_NETWORKING" ]] ; then
+  INSTALL_NETWORK_MANAGER="no"
+fi
+
+if [[ "$INSTALL_NETWORK_MANAGER" == "ask" ]]; then
+  debug ""
+  debug "Photonvision uses NetworkManager to control networking on your device."
  read -p "Do you want this script to install and configure NetworkManager? [y/N]: " response
  if [[ $response == [yY] || $response == [yY][eE][sS] ]]; then
-    INSTALL_NETWORK_MANAGER="true"
+    INSTALL_NETWORK_MANAGER="yes"
  fi
 fi


I still don't think this matches the original logic. The test for Ubuntu was intended to ask for permission if someone was installing it on Ubuntu to make sure that it wasn't messing up their network configuration. This logic will override --install-nm=yes if the platform isn't Ubuntu, even though the user explicitly calls for it to be installed. I'll propose this alternative:

Suggested change

# Quiet makes the default "no" instead of "ask".

if [[ -n "$QUIET" && "$INSTALL_NETWORK_MANAGER" == "ask" ]] ; then

INSTALL_NETWORK_MANAGER="no"

fi

DISTRO=$(lsb_release -is)

if [[ "$DISTRO" = "Ubuntu" && "$INSTALL_NETWORK_MANAGER" != "true" && -z "$QUIET" && -z "$DISABLE_NETWORKING" ]]; then

echo ""

echo "Photonvision uses NetworkManager to control networking on your device."

# NetworkManager cannot be installed on non-Ubuntu distros

# --no-networking takes precedence over --install-nm

if [[ "$DISTRO" != "Ubuntu" || -n "$DISABLE_NETWORKING" ]] ; then

INSTALL_NETWORK_MANAGER="no"

fi

if [[ "$INSTALL_NETWORK_MANAGER" == "ask" ]]; then

debug ""

debug "Photonvision uses NetworkManager to control networking on your device."

read -p "Do you want this script to install and configure NetworkManager? [y/N]: " response

if [[ $response == [yY] || $response == [yY][eE][sS] ]]; then

INSTALL_NETWORK_MANAGER="true"

INSTALL_NETWORK_MANAGER="yes"

fi

fi

DISTRO=$(lsb_release -is)

if [[ "$INSTALL_NETWORK_MANAGER" == "ask" ]]; then

if [[ "$DISTRO" != "Ubuntu" || -n "$DISABLE_NETWORKING" || -n "$QUIET" ]] ; then

# Only ask if it makes sense to do so

INSTALL_NETWORK_MANAGER="no"

else

debug ""

debug "Photonvision uses NetworkManager to control networking on your device."

read -p "Do you want this script to install and configure NetworkManager? [y/N]: " response

if [[ $response == [yY] || $response == [yY][eE][sS] ]]; then

INSTALL_NETWORK_MANAGER="yes"

else

INSTALL_NETWORK_MANAGER="no"

fi

fi

fi

Yeah I can make these changes. I had the impression that NetworkManager was only available on Ubuntu which is why I had that logic.

Not a huge fan of large nested ifs. I think it's more readable just to have two seperate ones, but the behavior is equivalent. You can see the most recent commit for that.

Thank you. This looks right to me.

scripts/install.sh

crschardt

Thanks for all the updates. I think this is good to go. Please merge when you are able.

mcm001 · 2024-10-27T22:07:23Z

You can also? I think? Yolo

Let's move this into photon image modifier and delete from this repo post merge

Transfer the updates from PhotonVision/photonvision#1456 to this repo.

EmDash00 added 10 commits October 11, 2024 11:25

Add debug function to respect quiet

e4512b7

Add die function to simplify error returns

a7f3eef

Edit NetworkManager install debug to be clearer

14561a8

Edit debug output for apt-get update

3613521

Simplify installation by skipping installed packages

f6a03e6

Add option for specifying architecture formally

1fa4334

Support long options

78c552c

Make -m more sensible

f6f317e

Add -l,--list-versions

75d339d

Add -v,--version

190738c

EmDash00 requested a review from a team as a code owner October 11, 2024 19:40

EmDash00 mentioned this pull request Oct 11, 2024

Unable to Load PhotonVision On Romi #1452

Open

crschardt mentioned this pull request Oct 12, 2024

Apply-updates-from-emdash00 PhotonVision/photon-image-modifier#33

Merged

crschardt reviewed Oct 13, 2024

View reviewed changes

scripts/install.sh Show resolved Hide resolved

EmDash00 added 2 commits October 13, 2024 14:44

Make 'latest' the default version to install

7466d0f

prompt user to install curl if not installed for --list-versions

a98b0b8

crschardt reviewed Oct 17, 2024

View reviewed changes

scripts/install.sh Show resolved Hide resolved

crschardt reviewed Oct 17, 2024

View reviewed changes

EmDash00 added 9 commits October 17, 2024 15:06

Fix behavior of -q,--quiet, edit/clarify help docs

d6763e5

use grep -q instead of cat and grep with -n

77bb9c3

Fix typos in version checker function

ef3064e

simplify version checking logic

1750b9a

Explicitly return 0 for clarity in is_version_available

6b71def

Drop leading v's with a parameter expansion instead of sed

3c7807b

Delete unnecessary shift for positional arguments since we don't use …

46b642f

…them anymore

Check if the cd to /opt/photonvision actually succeeds

369d4ba

Put quotes around variables

0f39a73

Use wget if get_photon_releases if curl isn't available, simplify logic

363fd3b

crschardt reviewed Oct 17, 2024

View reviewed changes

scripts/install.sh Outdated Show resolved Hide resolved

EmDash00 added 3 commits October 24, 2024 14:35

Fix -m,--install-nm behavior to match original script

c8bd8e2

Clarify why we ask to install NetworkManager on Ubuntu

fb1e6c9

Only enable cores 4-7 on Rockchip RK3588

18a8b0a

crschardt approved these changes Oct 27, 2024

View reviewed changes

mcm001 merged commit 6f52267 into PhotonVision:master Oct 27, 2024
31 checks passed

crschardt added a commit to PhotonVision/photon-image-modifier that referenced this pull request Nov 7, 2024

Apply-updates-from-emdash00 (#33)

2e5ddb6

Transfer the updates from PhotonVision/photonvision#1456 to this repo.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Install script improvements #1456

Install script improvements #1456

EmDash00 commented Oct 11, 2024

crschardt commented Oct 12, 2024

mcm001 commented Oct 12, 2024 •

edited

Loading

crschardt commented Oct 12, 2024

crschardt Oct 17, 2024 •

edited

Loading

EmDash00 Oct 17, 2024

EmDash00 commented Oct 17, 2024

mcm001 commented Oct 17, 2024

crschardt commented Oct 17, 2024

crschardt Oct 17, 2024

EmDash00 Oct 24, 2024

EmDash00 Oct 24, 2024

crschardt Oct 27, 2024

crschardt left a comment

mcm001 commented Oct 27, 2024

Install script improvements #1456

Install script improvements #1456

Conversation

EmDash00 commented Oct 11, 2024

crschardt commented Oct 12, 2024

mcm001 commented Oct 12, 2024 • edited Loading

crschardt commented Oct 12, 2024

crschardt Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

EmDash00 Oct 17, 2024

Choose a reason for hiding this comment

EmDash00 commented Oct 17, 2024

mcm001 commented Oct 17, 2024

crschardt commented Oct 17, 2024

crschardt Oct 17, 2024

Choose a reason for hiding this comment

EmDash00 Oct 24, 2024

Choose a reason for hiding this comment

EmDash00 Oct 24, 2024

Choose a reason for hiding this comment

crschardt Oct 27, 2024

Choose a reason for hiding this comment

crschardt left a comment

Choose a reason for hiding this comment

mcm001 commented Oct 27, 2024

mcm001 commented Oct 12, 2024 •

edited

Loading

crschardt Oct 17, 2024 •

edited

Loading