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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Sep 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
22bc456
Improved Sming installation
berhoel Sep 22, 2024
39bf1c7
Fixing Codacy Static Code Analysis
berhoel Sep 22, 2024
32cdb63
Fixing Codacy Static Code Analysis again
berhoel Sep 22, 2024
7aae7e1
Merge pull request #1 from SmingHub/develop
berhoel Sep 23, 2024
772f25a
Reverted changes regarding tool installation paths
berhoel Sep 23, 2024
d3d2490
Add shell functions to check for installed tools
berhoel Sep 23, 2024
b9b4426
Applied latest code review comments
berhoel Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,9 @@ GPATH
.submodule
*.built
*.completed

/esp-idf*
/esp-quick-toolchain/
/esp32/
/rp2040/
berhoel marked this conversation as resolved.
Show resolved Hide resolved
/samples/Basic_Serial/files/
berhoel marked this conversation as resolved.
Show resolved Hide resolved
23 changes: 23 additions & 0 deletions Sming/Arch/Esp32/Tools/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,29 @@ case $DIST in
darwin)
;;

*)
_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=()
;;

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

esac

$PKG_INSTALL "${PACKAGES[@]}"
Expand Down
11 changes: 6 additions & 5 deletions Tools/export.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ if [ -z "$SMING_HOME" ]; then
else
_SOURCEDIR=$(dirname "${BASH_SOURCE[0]}")
fi
SMING_HOME=$(realpath "$_SOURCEDIR/../Sming")
_SMIG_BASE=$(realpath "$_SOURCEDIR/..")
SMING_HOME=${_SMIG_BASE}"/Sming"
export SMING_HOME
echo "SMING_HOME: $SMING_HOME"
fi
Expand All @@ -33,14 +34,14 @@ fi
export PYTHON=${PYTHON:=$(which python3)}

# Esp8266
export ESP_HOME=${ESP_HOME:=/opt/esp-quick-toolchain}
export ESP_HOME=${ESP_HOME:=${_SMIG_BASE}/esp-quick-toolchain}

# Esp32
export IDF_PATH=${IDF_PATH:=/opt/esp-idf}
export IDF_TOOLS_PATH=${IDF_TOOLS_PATH:=/opt/esp32}
export IDF_PATH=${IDF_PATH:=${_SMIG_BASE}/esp-idf}
export IDF_TOOLS_PATH=${IDF_TOOLS_PATH:=${_SMIG_BASE}/esp32}

# 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Expand Down
24 changes: 24 additions & 0 deletions Tools/install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,34 @@ elif [ -n "$(command -v dnf)" ]; then
DIST=fedora
PKG_INSTALL="sudo dnf install -y"
else
_OK=1
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rework this.

echo "Unsupported distribution"
_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
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into implementing the pre-check function.

if [ $_OK != 1 ]; then
if [ $sourced = 1 ]; then
return 1
else
exit 1
fi
fi
fi

# Common install
Expand Down Expand Up @@ -183,9 +205,11 @@ case $DIST in

esac

if [ $(/usr/bin/python -c "import sys;print(sys.version_info[0])") != 3 ]; then
berhoel marked this conversation as resolved.
Show resolved Hide resolved
if [ "$DIST" != "darwin" ]; then
sudo update-alternatives --install /usr/bin/python python /usr/bin/python3 100
fi
fi

set -e

Expand Down