Improved checking for uninstalled tools for unsupported linux distributions. #2887

berhoel · 2024-09-22T19:22:35Z

Support for linux platforms not providing apt or dnf. Installation script searches for required tools and list missing.
Better support for installing outside "/opt". esp and rp2040 tools are not installed in "/opt" if Sming isn't.

I tried to install Sming on my OpenSuse Tumbleseed system. For one, I did not like the forced usage of "sudo" without knowing what was going on (I had a wrapper installed that provided "apt", using zypper in the background).

I would have liked code better that checks for required tools and asks the user to install them to install them if missing.

Second installing the tools to "/opt" did not work for me with user install. I don't want "/opt" to be user writable, and even installing the tools using "sudo" lead to fails later, when I tried to compile a project for ESP32.

My patch simply looks for required tools if neither apt nor dnf are found and lets the tools install in the Base directory Sming is installed.

- Support for linux platforms not providing apt or dnf. Installation script searches for required tools and list missing. - Better support for installing outside "/opt". esp and rp2040 tools are not installed in "/opt" if Sming isn't.

what-the-diff · 2024-09-22T19:23:07Z

PR Summary

Updated .gitignore File
The .gitignore file has been updated to ignore some new folders and files that are relevant to specific technologies (ESP32 and RP2040). This helps to avoid unnecessary tracking of changes that are not significant to our application's code.
Improved Installation Script for ESP32
The install.sh file used for setting up our ESP32 technology has been enhanced. It now checks if certain commands and headers are present before proceeding. If these are not available, the installation process is stopped. This aims to ensure that our setup process is robust and attentive to necessary components.
Dynamic Environment Variables Setup
The export.sh was altered to dynamically set environment variables like SMING_HOME, ESP_HOME, IDF_PATH, IDF_TOOLS_PATH, and PICO_TOOLCHAIN_PATH based on the script's location. This allows for a more adaptable setup, which aids in making the development process smoother regardless of the project's directory structure.
Extended Tool Verification during Installation
The install.sh script within the Tools directory has been expanded to check for several vital development tools. If these tools are missing, the process prompts for their installation and provides a clear exit process if the prerequisites are not met. This change offers a proactive remedy for missing toolset issues, which should cut down on setup troubleshooting time.
Python Version Check during Installation
A version check for Python has been added to ensure that Python 3 is being used for installation processes. This helps to maintain harmony with our codebase that relies on Python 3 technologies, ensuring compatibility and reducing the likelihood of version-related issues during the setup process.

Fix build warnings/errors in C++20 (SmingHub#2883)

berhoel · 2024-09-23T10:34:56Z

I will rework my pull request taking into account the failed CI run.

mikee47

Thank you for this. I do have some reservations though...

mikee47 · 2024-09-23T11:29:27Z

Tools/install.sh

@@ -99,12 +99,34 @@ elif [ -n "$(command -v dnf)" ]; then
    DIST=fedora
    PKG_INSTALL="sudo dnf install -y"
 else
+    _OK=1


OK is a bit terse. TOOLS_MISSING would be better.
Please don't use _ idenfiers - follow the style of existing script.

I will rework this.

mikee47 · 2024-09-23T11:30:28Z

Tools/install.sh

+    _REQUIRED_TOOLS=(
+            ccache \
+            cmake \
+            curl \
+            git \
+            make \
+            ninja \
+            unzip \
+            g++ \
+            python3 \
+            pip3 \
+	    wget \
+	    )
+    for _TOOL in "${_REQUIRED_TOOLS[@]}"; do
+	if ! [ -x $(command -v "${_TOOL}") ]; then
+	    _OK=0
+	    echo "Install required tool ${_TOOL}"
+	fi
+    done


This is a good idea. I'm wondering if this could be added as a general step earlier in the script as a pre-check for all platforms?

AND provide some generic script functions which can be called from the esp32 install.sh with a list.

I will look into implementing the pre-check function.

mikee47 · 2024-09-23T11:32:25Z

Sming/Arch/Esp32/Tools/install.sh

+    *)
+	_OK=1
+	_COMMANDS=(dfu-util bison flex gperf)
+	for _COMMAND in "${_COMMANDS[@]}"; do
+	    if ! [ -x $(command -v "${_COMMAND}") ]; then
+		_OK=0
+		echo "Install programm ${_COMMAND}"
+	    fi
+	done
+	_INCLUDES=("/usr/include/ffi.h" "/usr/include/ssl/ssl.h")
+	for _INCLUDE in "${_INCLUDES[@]}"; do
+	    if ! [ -f "${_INCLUDE}" ]; then
+		_OK=0
+		echo "Install development package providing ${_INCLUDE}"
+	    fi
+	done
+	if [ $_OK != 1 ]; then
+	    echo "ABORTING"
+	    exit 1
+	fi
+	PACKAGES=()
+        ;;
+


Seems reasonable, though is an awful lot of script for an edge case. How does Espressif handle non-standard installs of their IDF?

The environment variables ESP_HOME, IDF_PATH, and IDF_TOOLS_PATH should be used for non-standard installs of Expressif IDF (ESP32 and ESP8266).

mikee47 · 2024-09-23T11:34:04Z

Tools/export.sh


 # Rp2040
-export PICO_TOOLCHAIN_PATH=${PICO_TOOLCHAIN_PATH:=/opt/rp2040}
+export PICO_TOOLCHAIN_PATH=${PICO_TOOLCHAIN_PATH:=${_SMIG_BASE}/rp2040}

 # Provide non-apple CLANG (e.g. for rbpf library)
 if [ -n "$GITHUB_ACTIONS" ] && [ "$(uname)" = "Darwin" ]; then


No. Using the Sming install location is worse than using /opt. Whilst there are permissions issues with /opt I do believe it is the most sensible default. See https://www.baeldung.com/linux/opt-directory and https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard.

Incidentally I never install Sming in /opt but use sandboxes on a separate ZFS volume. I specifically never put bloatey tool installations (or the IDF) there. Thus the locations of toolchains should be decoupled from the location for Sming.

The procedure for customising install directories is outlined here https://sming.readthedocs.io/en/stable/getting-started/config.html. That should meet your needs. In particular, the ESP32 stuff has separate IDF_PATH for the framework (which needs to be writeable) and IDF_TOOLS_PATH for the toolchains.

I reverted these changes.

I handle the non-standart tools installation using direnv now.

With Sming checked out to my home directory, and adding a small .envrc to this directory (and every directory with projects using Sming):

layout Sming

and adding

layout_Sming() { _SMIG_BASE=${HOME}/Sming _SMIG_TOOLS=${_SMIG_BASE}/.tools export ESP_HOME=${_SMIG_TOOLS}/esp-quick-toolchain export IDF_PATH=${_SMIG_TOOLS}/esp-idf export IDF_TOOLS_PATH=${_SMIG_TOOLS}/esp32 export PICO_TOOLCHAIN_PATH=${_SMIG_TOOLS}/rp2040 . ${_SMIG_BASE}/Tools/export.sh }

to ~/.config/direnv/direnvrc which handles the required path settings.

.gitignore

mikee47 · 2024-09-23T11:41:35Z

I tried to install Sming on my OpenSuse Tumbleseed system. For one, I did not like the forced usage of "sudo" without knowing what was going on (I had a wrapper installed that provided "apt", using zypper in the background).

Well, it's not really forced as you get a prompt to enter password. But yes I agree this aspect could be improved. Maybe just add some more messages to let the user know what's going on?

mikee47 · 2024-09-23T11:49:06Z

Also, I note there are no additional checks for rp2040 / esp8266 ? Does this mean they install as-is or that you don't use them?

Implemented comments regarding shell variable names I will handle tool installatin paths via direnv. My .envrc is simple: layout Sming With Sming extracte in my home directory my `~/.config/direnv/direnvrc` then contains: layout_Sming() { _SMIG_BASE=${HOME}/Sming _SMIG_TOOLS=${_SMIG_BASE}/.tools export ESP_HOME=${_SMIG_TOOLS}/esp-quick-toolchain export IDF_PATH=${_SMIG_TOOLS}/esp-idf export IDF_TOOLS_PATH=${_SMIG_TOOLS}/esp32 export PICO_TOOLCHAIN_PATH=${_SMIG_TOOLS}/rp2040 . ${_SMIG_BASE}/Tools/export.sh }

berhoel · 2024-09-23T12:26:35Z

Also, I note there are no additional checks for rp2040 / esp8266 ? Does this mean they install as-is or that you don't use them?

Both rp2040 and esp8266 install as they are when ESP_HOME and PICO_TOOLCHAIN_PATH are set to writable paths.

The new shell function `check_for_installed_tools` can be used to check for installed tools. the function takes a list of required executables for argument. Shell process is aborted if any of the tools is missing. All tools missing in the list are reported. Similar the the shell function `check_for_installed_files` checks for installed files, here used to make sure header files are installed.

mikee47

Sorry for being finicky, but this is good work. Thank you.

Tools/install.sh

berhoel · 2024-09-24T10:17:13Z

Sorry for being finicky,

That's what code reviews are for 👍

but this is good work. Thank you.

Improved Sming installation

22bc456

- Support for linux platforms not providing apt or dnf. Installation script searches for required tools and list missing. - Better support for installing outside "/opt". esp and rp2040 tools are not installed in "/opt" if Sming isn't.

berhoel added 2 commits September 22, 2024 21:26

Fixing Codacy Static Code Analysis

39bf1c7

Fixing Codacy Static Code Analysis again

32cdb63

berhoel changed the title ~~Improved Sming installation~~ Improved Sming non root installation Sep 22, 2024

slaff added the 3 - Review label Sep 23, 2024

slaff requested a review from mikee47 September 23, 2024 07:43

Merge pull request #1 from SmingHub/develop

7aae7e1

Fix build warnings/errors in C++20 (SmingHub#2883)

mikee47 requested changes Sep 23, 2024

View reviewed changes

berhoel changed the title ~~Improved Sming non root installation~~ Improved checking for uninstalled tools for unsupported linux distributions. Sep 23, 2024

mikee47 requested changes Sep 24, 2024

View reviewed changes

Applied latest code review comments

b9b4426

mikee47 approved these changes Sep 24, 2024

View reviewed changes

slaff added this to the 6.0.0 milestone Sep 25, 2024

slaff merged commit c34d747 into SmingHub:develop Sep 25, 2024
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved checking for uninstalled tools for unsupported linux distributions. #2887

Improved checking for uninstalled tools for unsupported linux distributions. #2887

berhoel commented Sep 22, 2024 •

edited

Loading

what-the-diff bot commented Sep 22, 2024

berhoel commented Sep 23, 2024

mikee47 left a comment

mikee47 Sep 23, 2024

berhoel Sep 23, 2024

mikee47 Sep 23, 2024

mikee47 Sep 23, 2024

berhoel Sep 23, 2024

mikee47 Sep 23, 2024

berhoel Sep 23, 2024

mikee47 Sep 23, 2024

berhoel Sep 23, 2024

mikee47 commented Sep 23, 2024

mikee47 commented Sep 23, 2024

berhoel commented Sep 23, 2024

mikee47 left a comment

berhoel commented Sep 24, 2024

Improved checking for uninstalled tools for unsupported linux distributions. #2887

Improved checking for uninstalled tools for unsupported linux distributions. #2887

Conversation

berhoel commented Sep 22, 2024 • edited Loading

what-the-diff bot commented Sep 22, 2024

PR Summary

berhoel commented Sep 23, 2024

mikee47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 commented Sep 23, 2024

mikee47 commented Sep 23, 2024

berhoel commented Sep 23, 2024

mikee47 left a comment

Choose a reason for hiding this comment

berhoel commented Sep 24, 2024

berhoel commented Sep 22, 2024 •

edited

Loading