refactor(devshell): improve devshell experience #1977
Conversation
yvan-sraka
force-pushed
the
update-nixpkgs
branch
from
31a6d6b to
0649fff
Compare
yvan-sraka
force-pushed
the
update-nixpkgs
branch
2 times, most recently
from
b48e4c3 to
3df6c4a
Compare
samrose
left a comment
samrose
left a comment
There was a problem hiding this comment.
let's hold off on this one until we complete nixpkgs update and some of our other critical items. We can definitely try to do this one, but we may need adjust a few things, and document it a bit more as well
samrose
force-pushed
the
update-nixpkgs
branch
3 times, most recently
from
771718f to
4e32d5b
Compare
jfroche
force-pushed
the
update-nixpkgs
branch
from
acf3ba4 to
42b0c29
Compare
samrose
force-pushed
the
update-nixpkgs
branch
from
fa26b67 to
0131d17
Compare
The current development shell include a large number of packages which leads to a very large closure size (5 Gb) and slow startup times. To improve the situation we defer installing large packages to devshell commands which only get installed when needed. We introduce the use of devshell to define the commands in a declarative way as well as to improve the overall developer experience. Included changes: - Add organized command menu with categories (check, ami, extension, postgres) - Add watch command using watchexec + nix-fast-build for continuous checking - Expose common tasks as named commands: fmt, check, lint, watch - Add aws-vault to development tools
`writeShellApplication` avoids substitute commands, provides the correct package metadata and make sure that all scripts are shellcheked.
We don't need to maintain our own packer package anymore since we are using a recently updated nixpkgs that includes packer.
jfroche
force-pushed
the
fix/default-devshell
branch
from
2188ce0 to
efc2e26
Compare
WalkthroughAdds a Changes
Sequence Diagram(s)(Skipped — changes are primarily packaging/refactor and do not introduce a new multi-component runtime control flow requiring diagramming.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
nix/tools/sync-exts-versions.sh.in (1)
105-118: Missing error handling whennix buildsucceeds but expected to fail.The
update_cargo_vendor_hashfunction intentionally setscargoHashto empty to trigger a build failure and scrape the correct hash. However, if the build unexpectedly succeeds (line 116), the function silently continues without updating the hash. This might leave the Nix file in an inconsistent state with an emptycargoHash.Consider restoring the original hash or erroring out if the build succeeds unexpectedly after clearing the hash.
nix/packages/build-test-ami.nix (3)
118-124: Chain traps so EC2 cleanup still runs.The second trap overrides the earlier one, so
cleanup_instancesmay never execute.🐛 Proposed fix
- trap 'rm -rf "$VENV_DIR"' EXIT HUP INT QUIT TERM + trap 'cleanup_instances; rm -rf "$VENV_DIR"' EXIT HUP INT QUIT TERM
68-77: Validate postgres-version before using it.Without a required argument (or if it’s unknown),
PG_VERSIONcan be empty/null and the Packer build will fail later.🐛 Proposed fix
POSTGRES_VERSION="$1" + if [ -z "$POSTGRES_VERSION" ]; then + echo "Error: postgres-version argument is required" + exit 1 + fi @@ - PG_VERSION=$(yq -r ".postgres_release[\"postgres$POSTGRES_VERSION\"]" ansible/vars.yml) + PG_VERSION=$(yq -r ".postgres_release[\"postgres$POSTGRES_VERSION\"]" ansible/vars.yml) + if [ -z "$PG_VERSION" ] || [ "$PG_VERSION" = "null" ]; then + echo "Error: unknown postgres-version '$POSTGRES_VERSION'" + exit 1 + fi
61-65: Use the correct AWS Vault profile variable.The script validates
AWS_VAULT(line 62) but then tries to use the undefinedAWS_VAULT_PROFILEwhen running tests. Since the script expectsAWS_VAULTto contain the profile name, use that variable consistently.🐛 Proposed fix
- echo "Running tests for AMI: $RANDOM_STRING using AWS Vault profile: $AWS_VAULT_PROFILE" - aws-vault exec "$AWS_VAULT_PROFILE" -- pytest -vv -s testinfra/test_ami_nix.py + echo "Running tests for AMI: $RANDOM_STRING using AWS Vault profile: $AWS_VAULT" + aws-vault exec "$AWS_VAULT" -- pytest -vv -s testinfra/test_ami_nix.py
🤖 Fix all issues with AI agents
In `@nix/apps.nix`:
- Line 30: The app declaration uses the old two-argument form of mkApp; update
the supascan entry to call mkApp with a single argument (remove the second
"supascan" argument) so it matches the refactored signature of mkApp and returns
the expected attribute set (i.e., change the line referencing mkApp "supascan"
"supascan" to a single-argument call to mkApp "supascan").
In `@nix/packages/dbmate-tool.nix`:
- Around line 27-29: The default PSQL_VERSION is set to "ALL" but the build
logic expects "all"; change the default to PSQL_VERSION="all" (and update the
other occurrences of PSQL_VERSION="ALL" at the other default block referenced)
so path checks and the psql_${PSQL_VERSION} branch match the lowercase "all"
branch; leave PORTNO="${defaults.port}" as-is.
- Around line 290-291: The script currently ignores failures from perform_dump
which can leave a missing/partial schema; update the call after echo "Running
dbmate dump with $PSQLBIN" to check perform_dump's exit status and fail fast
(e.g., run perform_dump && true || { echo "dbmate dump failed"; exit 1; } or
simply if ! perform_dump; then echo "perform_dump failed" >&2; exit 1; fi),
ensuring any non-zero return from perform_dump causes the script to exit with
error; reference perform_dump and the surrounding echo "Running dbmate dump with
$PSQLBIN" to locate where to add this check.
- Around line 56-57: The cleanup function currently calls pg_ctl without a
versioned path which can pick up a mismatched system binary; change cleanup to
invoke the pg_ctl from the selected Postgres bindir (use PSQLBIN so it runs as
"$PSQLBIN/pg_ctl" with DATDIR) and ensure PSQLBIN is exported or set as a global
variable before any call to migrate_version(); alternatively, move cleanup to be
a nested function inside migrate_version() so it closes over the correct
PSQLBIN—update references to the cleanup and migrate_version functions and
ensure PSQLBIN is initialized prior to their use.
In `@nix/packages/migrate-tool.nix`:
- Around line 30-33: The error message inside the conditional that checks if [[
$2 == /nix/store* ]] incorrectly references $1; update the echo to reference $2
so the reported path matches the validated value (i.e., change the message in
the block that checks for "$2/receipt.json" to echo "ERROR: $2 does not look
like a valid Postgres install" and keep the exit 1).
In `@nix/packages/start-replica.nix`:
- Line 36: Fix the typo in the user-facing message: locate the echo statement
that reads echo "NOTE: runing pg_basebackup for server on port $MASTER_PORTNO"
and correct "runing" to "running" so the message becomes "NOTE: running
pg_basebackup for server on port $MASTER_PORTNO".
In `@nix/tools/sync-exts-versions.sh.in`:
- Around line 188-192: The case pattern "ptap_release" in the case statement
doesn't match the variable name varname="pgtap_release", so the pgtap extension
branch never runs; update the case label from "ptap_release" to "pgtap_release"
to match the ansible var key and ensure the branch calls run_sync "$varname"
"$package_name" for pgtap_release.
- Around line 44-49: The echo uses an undefined variable $key inside the
run_sync scope; replace $key with the existing $varname (or if $key was
intended, add $key as a parameter to run_sync). Locate the block around run_sync
where version is computed (look for version=$(echo "$ansible_vars" | jq -r
".$varname")), change the echo to reference $varname (echo "$varname: $version")
so the correct variable is printed, or alternatively update the function
signature to accept $key and use that consistently when calling sync_version
"$package_name" "$version" "$hash".
- Around line 94-99: The echo in run_sync_fetchurl references an undefined $key
(same bug as run_sync); update the function to either derive key or simply echo
the package identifier already in scope—replace the use of $key with
$package_name (or set key="$package_name" before use) so that the line echo
"$key: $version" becomes valid; touch the run_sync_fetchurl function and ensure
other uses in that block (version, package_name, url, hash,
sync_version_fetchurl) remain unchanged.
🧹 Nitpick comments (6)
nix/tools/sync-exts-versions.sh.in (1)
7-26: Consider consolidating nearly identical functions.
fetch_source_urlandfetchurl_source_urlare almost identical — the only difference is thatfetch_source_urluses--unpackflag whilefetchurl_source_urldoesn't. Similarly,sync_versionvssync_version_fetchurldiffer only in the attribute path (src.hashvssrc.sha256), andrun_syncvsrun_sync_fetchurldiffer only in which helper functions they call.Consider parameterizing these functions to reduce duplication and maintenance burden.
♻️ Example consolidation for fetch functions
+fetch_source_url_impl() { + local source_url=${1//\"/} + source_url=${source_url//\'/} + local unpack_flag=${2:-""} + + if [ -z "$source_url" ]; then + echo "Usage: fetch_source_url_impl <source_url> [--unpack]" + return 1 + fi + + echo "$source_url" + local initial_hash + initial_hash=$(nix-prefetch-url --type sha256 "$source_url" $unpack_flag | cut -d ' ' -f 2) + local final_hash + final_hash=$(nix hash to-sri --type sha256 "$initial_hash") + echo "$final_hash" +} + fetch_source_url() { - # ... full implementation + fetch_source_url_impl "$1" "--unpack" } + fetchurl_source_url() { - # ... full implementation + fetch_source_url_impl "$1" "" }Also applies to: 56-75
nix/packages/start-replica.nix (2)
33-34: Redundantmkdir -paftermktemp -d.
mktemp -dalready creates the directory, so the subsequentmkdir -p "$DATDIR"is unnecessary.♻️ Proposed fix
REPLICA_SLOT="replica_$RANDOM" DATDIR=$(mktemp -d) - mkdir -p "$DATDIR"
11-25: Consider simplifying version handling or adding support for other versions.Versions 16 and orioledb-16 are handled but immediately error out. If these versions are not intended to be supported, consider either:
- Removing these branches and letting the
elseclause handle unknown versions- Adding TODO comments indicating future support plans
- Actually implementing support (the function signature only receives
psql_15)Currently, the explicit error messages are more helpful than a generic "invalid version" message, so this is a minor observation.
nix/packages/start-client.nix (1)
95-101: Consider removing unused environment variables.
POSTGRES_DBandPOSTGRES_HOSTare exported but thepsqlcommand on line 101 uses explicit arguments (-h localhostand positionalpostgres). These exports appear unused unless intended for other tooling.🔧 Suggested cleanup
# Set up environment for psql export PATH="$BINDIR/bin:$PATH" - export POSTGRES_DB=postgres - export POSTGRES_HOST=localhost # Start interactive psql session exec psql -U "$PSQL_USER" -p "$PORTNO" -h localhost postgresnix/packages/cleanup-ami.nix (1)
27-38: Consider making the region configurable.The region is hardcoded to
ap-southeast-1. If AMIs need to be cleaned up in other regions, consider adding an optional--regionflag or defaulting toAWS_DEFAULT_REGIONif set.🔧 Suggested enhancement
AMI_NAME="$1" - REGION="ap-southeast-1" + REGION="''${AWS_DEFAULT_REGION:-ap-southeast-1}" # Deregister AMIsnix/packages/migrate-tool.nix (1)
1-10: Include coreutils/gnused in runtimeInputs for hermetic execution.The script relies on external utilities (mktemp/sed/cat/rm). Adding explicit runtime inputs avoids host PATH coupling.
♻️ Suggested update
@@ -{ writeShellApplication, psql_15 }: +{ writeShellApplication, psql_15, coreutils, gnused }: @@ - runtimeInputs = [ ]; + runtimeInputs = [ coreutils gnused ];
| # Default values | ||
| PSQL_VERSION="ALL" | ||
| PORTNO="${defaults.port}" |
There was a problem hiding this comment.
Default PSQL_VERSION should match the “all” branch.
With PSQL_VERSION="ALL", the default path won’t match the all check and will attempt to build psql_ALL.
🐛 Proposed fix
- PSQL_VERSION="ALL"
+ PSQL_VERSION="all"Also applies to: 315-316
🤖 Prompt for AI Agents
In `@nix/packages/dbmate-tool.nix` around lines 27 - 29, The default PSQL_VERSION
is set to "ALL" but the build logic expects "all"; change the default to
PSQL_VERSION="all" (and update the other occurrences of PSQL_VERSION="ALL" at
the other default block referenced) so path checks and the psql_${PSQL_VERSION}
branch match the lowercase "all" branch; leave PORTNO="${defaults.port}" as-is.
| # Use pg_ctl to stop PostgreSQL | ||
| pg_ctl -D "$DATDIR" stop |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n nix/packages/dbmate-tool.nixRepository: supabase/postgres
Length of output: 14235
Fix pg_ctl version mismatch in cleanup function.
The cleanup function uses pg_ctl without a versioned path. It may run before PATH is updated (line 241) or after migrate_version() returns (lines 321, 326), potentially picking a mismatched or system pg_ctl.
Consider exporting PSQLBIN as a global variable before calling migrate_version(), or wrapping cleanup() as a nested function inside migrate_version() to access its scope.
🤖 Prompt for AI Agents
In `@nix/packages/dbmate-tool.nix` around lines 56 - 57, The cleanup function
currently calls pg_ctl without a versioned path which can pick up a mismatched
system binary; change cleanup to invoke the pg_ctl from the selected Postgres
bindir (use PSQLBIN so it runs as "$PSQLBIN/pg_ctl" with DATDIR) and ensure
PSQLBIN is exported or set as a global variable before any call to
migrate_version(); alternatively, move cleanup to be a nested function inside
migrate_version() so it closes over the correct PSQLBIN—update references to the
cleanup and migrate_version functions and ensure PSQLBIN is initialized prior to
their use.
| echo "Running dbmate dump with $PSQLBIN" | ||
| perform_dump |
There was a problem hiding this comment.
Fail fast when pg_dump retries exhaust.
perform_dump failure is ignored, which can propagate a missing/partial schema file.
🐛 Proposed fix
- perform_dump
+ if ! perform_dump; then
+ echo "Error: pg_dump failed"
+ exit 1
+ fi🤖 Prompt for AI Agents
In `@nix/packages/dbmate-tool.nix` around lines 290 - 291, The script currently
ignores failures from perform_dump which can leave a missing/partial schema;
update the call after echo "Running dbmate dump with $PSQLBIN" to check
perform_dump's exit status and fail fast (e.g., run perform_dump && true || {
echo "dbmate dump failed"; exit 1; } or simply if ! perform_dump; then echo
"perform_dump failed" >&2; exit 1; fi), ensuring any non-zero return from
perform_dump causes the script to exit with error; reference perform_dump and
the surrounding echo "Running dbmate dump with $PSQLBIN" to locate where to add
this check.
| if [[ $2 == /nix/store* ]]; then | ||
| if [ ! -L "$2/receipt.json" ] || [ ! -e "$2/receipt.json" ]; then | ||
| echo "ERROR: $1 does not look like a valid Postgres install" | ||
| exit 1 |
There was a problem hiding this comment.
Fix the new-version validation error message.
The error currently references $1, so invalid $2 paths are misreported.
🐛 Proposed fix
- echo "ERROR: $1 does not look like a valid Postgres install"
+ echo "ERROR: $2 does not look like a valid Postgres install"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ $2 == /nix/store* ]]; then | |
| if [ ! -L "$2/receipt.json" ] || [ ! -e "$2/receipt.json" ]; then | |
| echo "ERROR: $1 does not look like a valid Postgres install" | |
| exit 1 | |
| if [[ $2 == /nix/store* ]]; then | |
| if [ ! -L "$2/receipt.json" ] || [ ! -e "$2/receipt.json" ]; then | |
| echo "ERROR: $2 does not look like a valid Postgres install" | |
| exit 1 |
🤖 Prompt for AI Agents
In `@nix/packages/migrate-tool.nix` around lines 30 - 33, The error message inside
the conditional that checks if [[ $2 == /nix/store* ]] incorrectly references
$1; update the echo to reference $2 so the reported path matches the validated
value (i.e., change the message in the block that checks for "$2/receipt.json"
to echo "ERROR: $2 does not look like a valid Postgres install" and keep the
exit 1).
| DATDIR=$(mktemp -d) | ||
| mkdir -p "$DATDIR" | ||
|
|
||
| echo "NOTE: runing pg_basebackup for server on port $MASTER_PORTNO" |
There was a problem hiding this comment.
Typo: "runing" should be "running".
Minor typo in user-facing message.
📝 Proposed fix
- echo "NOTE: runing pg_basebackup for server on port $MASTER_PORTNO"
+ echo "NOTE: running pg_basebackup for server on port $MASTER_PORTNO"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "NOTE: runing pg_basebackup for server on port $MASTER_PORTNO" | |
| echo "NOTE: running pg_basebackup for server on port $MASTER_PORTNO" |
🤖 Prompt for AI Agents
In `@nix/packages/start-replica.nix` at line 36, Fix the typo in the user-facing
message: locate the echo statement that reads echo "NOTE: runing pg_basebackup
for server on port $MASTER_PORTNO" and correct "runing" to "running" so the
message becomes "NOTE: running pg_basebackup for server on port $MASTER_PORTNO".
| version=$(echo "$ansible_vars" | jq -r ".$varname") | ||
| echo "$key: $version" | ||
| url=$($_nix eval .#psql_15/exts/$package_name.src.url) | ||
| hash=$(fetchurl_source_url $url | tail -n 1) | ||
| $(sync_version_fetchurl $package_name $version $hash) | ||
| url=$(nix eval ".#psql_15/exts/$package_name.src.url") | ||
| hash=$(fetchurl_source_url "$url" | tail -n 1) | ||
| sync_version_fetchurl "$package_name" "$version" "$hash" | ||
| echo "synced $package_name to version $version with hash $hash" |
There was a problem hiding this comment.
Same undefined variable issue in run_sync_fetchurl.
Line 95 has the same problem as run_sync — $key is not defined in this function's scope.
🐛 Proposed fix
run_sync_fetchurl() {
local varname=$1
local package_name=$2
version=$(echo "$ansible_vars" | jq -r ".$varname")
- echo "$key: $version"
+ echo "$varname: $version"
url=$(nix eval ".#psql_15/exts/$package_name.src.url")
hash=$(fetchurl_source_url "$url" | tail -n 1)
sync_version_fetchurl "$package_name" "$version" "$hash"
echo "synced $package_name to version $version with hash $hash"
}🤖 Prompt for AI Agents
In `@nix/tools/sync-exts-versions.sh.in` around lines 94 - 99, The echo in
run_sync_fetchurl references an undefined $key (same bug as run_sync); update
the function to either derive key or simply echo the package identifier already
in scope—replace the use of $key with $package_name (or set key="$package_name"
before use) so that the line echo "$key: $version" becomes valid; touch the
run_sync_fetchurl function and ensure other uses in that block (version,
package_name, url, hash, sync_version_fetchurl) remain unchanged.
| "ptap_release") | ||
| varname="pgtap_release" | ||
| package_name="pgtap" | ||
| run_sync $varname $package_name | ||
| run_sync "$varname" "$package_name" | ||
| ;; |
There was a problem hiding this comment.
Typo in case pattern: ptap_release should be pgtap_release.
The case pattern on line 188 is "ptap_release" but the varname on line 189 is "pgtap_release". This mismatch means the pgtap extension will never be synced since the case pattern won't match the ansible var key.
🐛 Proposed fix
- "ptap_release")
+ "pgtap_release")
varname="pgtap_release"
package_name="pgtap"
run_sync "$varname" "$package_name"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "ptap_release") | |
| varname="pgtap_release" | |
| package_name="pgtap" | |
| run_sync $varname $package_name | |
| run_sync "$varname" "$package_name" | |
| ;; | |
| "pgtap_release") | |
| varname="pgtap_release" | |
| package_name="pgtap" | |
| run_sync "$varname" "$package_name" | |
| ;; |
🤖 Prompt for AI Agents
In `@nix/tools/sync-exts-versions.sh.in` around lines 188 - 192, The case pattern
"ptap_release" in the case statement doesn't match the variable name
varname="pgtap_release", so the pgtap extension branch never runs; update the
case label from "ptap_release" to "pgtap_release" to match the ansible var key
and ensure the branch calls run_sync "$varname" "$package_name" for
pgtap_release.
There was a problem hiding this comment.
We're going to remove this sync-extensions so this is not an issue
There was a problem hiding this comment.
@samrose, understood! If the script is being removed, then there's no need to fix the typo. Thanks for the context.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The current development shell include a large number of packages which
leads to a very large closure size (5 Gb) and slow startup times.
To improve the situation we defer installing large packages to devshell
commands which only get installed when needed.
We introduce the use of devshell to define the commands in a
declarative way as well as to improve the overall developer experience.
The closure of the default devshell is now down to 2 Gb (which still could be improved later on).
Included changes:
Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.