Nix Flake Cleanup#272
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the development environment by adding arduino-cli, automating ESP32 core installation, and silencing pip version checks. Feedback was provided to include the necessary Espressif board manager URL to prevent installation failures and to optimize the Python package installation process by only running pip install when requirements.txt is modified.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the development environment by moving shell initialization logic from flake.nix into a dedicated nix-hook.sh script, while also adding arduino-cli to the environment and updating the project license. The review identifies critical issues with the new script being sourced into interactive shells, specifically the use of set -e and exit which would unexpectedly close the user's terminal session. Additionally, it recommends removing unnecessary chmod calls in the Nix hook and resolving a logic discrepancy where installation messages are printed for commented-out commands.
| #!/usr/bin/env bash | ||
|
|
||
| # Exit on error | ||
| set -e |
There was a problem hiding this comment.
Using set -e in a script that is intended to be sourced into an interactive shell (as seen in flake.nix) is problematic. It modifies the behavior of the parent shell, causing the entire terminal session to exit if any subsequent command returns a non-zero status. This should be removed for scripts meant to be sourced.
| touch "$SENTINEL" | ||
| else | ||
| echo "Error: pip install failed." | ||
| exit 1 |
There was a problem hiding this comment.
The exit command will terminate the entire shell session when this script is sourced. Use return instead to stop the script's execution while keeping the terminal open.
| exit 1 | |
| return 1 |
| # Make it executable | ||
| chmod +x ./nix-hook.sh | ||
| source ./nix-hook.sh |
There was a problem hiding this comment.
Modifying file permissions with chmod +x inside a shellHook is unnecessary and can fail on read-only filesystems (e.g., in certain CI environments or Nix builds). It is better to ensure the script is committed with executable permissions in the repository.
source ./nix-hook.sh
| lsusb_mac() { system_profiler SPUSBDataType; } | ||
| export -f lsusb_mac | ||
|
|
||
| ls_stm32_dev_port() { ls /dev/cu.*; } | ||
| export -f ls_stm32_dev_port |
There was a problem hiding this comment.
The ESP32 core installation command is currently commented out, but the check and the 'Installing...' message are active. This causes the script to print the installation message every time the shell is entered without actually performing the installation. Either uncomment the installation command or remove this block to avoid the redundant message loop.
| lsusb_mac() { system_profiler SPUSBDataType; } | |
| export -f lsusb_mac | |
| ls_stm32_dev_port() { ls /dev/cu.*; } | |
| export -f ls_stm32_dev_port | |
| if ! arduino-cli core list | grep -q "esp32" > /dev/null; then | |
| echo "Installing ESP32 core..." | |
| arduino-cli core update-index > /dev/null | |
| arduino-cli core install esp32:esp32 > /dev/null | |
| fi |
1 participant
No description provided.