Fix mainInParentDirectory failing when initdb is not on PATH#2398
Fix mainInParentDirectory failing when initdb is not on PATH#2398
Conversation
|
@mpscholten this is AI generated, and I don't know enough to confirm it - so needs a proper review 😄 |
|
@codex GH action failed with |
|
To use Codex here, create an environment for this repo. |
* Initial plan * Fix test assertion to check for IHPSchema.sql in any path The test was expecting IHPSchema.sql to be at a specific path in the test directory, but in the Nix build environment, getDataFileName resolves to the actual source directory. Changed the assertion to just check that IHPSchema.sql is loaded with -f flag, regardless of the absolute path. Co-authored-by: amitaibu <125707+amitaibu@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: amitaibu <125707+amitaibu@users.noreply.github.com>
Use #!/bin/sh instead of #!/usr/bin/env bash in fake test scripts since /usr/bin/env is not available in the nix build sandbox. Also use exec in the fake direnv wrapper for proper signal propagation, and add create_group to initDatabase for consistent process cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| [ "ihp-ide" </> path | ||
| , "ihp-ide" </> "data" </> path | ||
| , "IHP" </> "ihp-ide" </> path | ||
| , "IHP" </> "ihp-ide" </> "data" </> path | ||
| ] |
There was a problem hiding this comment.
I think we can trim this list?
IHP_LIB is always set by the dev server, so ihpLibCandidates already covers the needed paths. The fallback candidates were duplicates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Improves ihp-ide dev-mode Postgres startup reliability when running mainInParentDirectory, especially in environments where Postgres binaries (e.g. initdb) are only available via direnv-provided PATH, and fixes dev/ghci data-file resolution for IHPSchema.sql.
Changes:
- Route Postgres setup/stale-lock commands through a direnv-aware runner when
wrapWithDirenvis enabled; switch SQL imports topsql -f. - Fix
dev/Paths_ihp_ide.getDataFileNameresolution soIHPSchema.sqlis found viaIHP_LIBwhen running from a parent directory. - Add a regression test covering wrapped execution mode.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ihp-ide/ihp-ide.cabal | Registers the new Postgres regression spec in the test suite. |
| ihp-ide/Test/Main.hs | Hooks the new Test.IDE.PostgresSpec into the test runner. |
| ihp-ide/Test/IDE/PostgresSpec.hs | Adds a regression test for direnv-wrapped Postgres setup and schema loading. |
| ihp-ide/IHP/IDE/Postgres.hs | Runs init/stop/import commands direnv-aware and uses psql -f for SQL imports. |
| dev/Paths_ihp_ide.hs | Enhances dev-mode getDataFileName to locate IHPSchema.sql via IHP_LIB. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| currentDirectory <- Directory.getCurrentDirectory | ||
| let testDir = currentDirectory <> "/build/test-postgres-direnv" | ||
| ignoreIOError (Directory.removePathForcibly testDir) |
There was a problem hiding this comment.
withTemporaryTestDirectory uses a fixed path (build/test-postgres-direnv) under the repo, which can cause flaky failures if the test suite is run concurrently (or if a prior run crashed and left state behind). Prefer using a unique temp directory (e.g. via System.IO.Temp.withSystemTempDirectory) or include a random/unique suffix in the directory name.
| commandLines `shouldSatisfy` any ("exec . initdb " `Text.isPrefixOf`) | ||
| commandLines `shouldSatisfy` any ("exec . createdb " `Text.isPrefixOf`) | ||
| commandLines `shouldSatisfy` any ("exec . psql " `Text.isPrefixOf`) | ||
| commandLines `shouldSatisfy` any (\line -> "-f " `Text.isInfixOf` line && "IHPSchema.sql" `Text.isInfixOf` line) | ||
| commandLines `shouldSatisfy` any ("-f Application/Schema.sql" `Text.isInfixOf`) | ||
| commandLines `shouldSatisfy` any ("-f Application/Fixtures.sql" `Text.isInfixOf`) |
There was a problem hiding this comment.
The assertions only check that the logged psql invocation contains IHPSchema.sql, but the stub psqlScript doesn’t validate that the -f path actually exists (or that it was resolved via IHP_LIB). This can let the regression test pass even if getDataFileName still returns the default relative path and the schema file can’t be found. Consider making the stub psql fail when the -f target doesn’t exist, or assert the exact expected resolved path from IHP_LIB in commands.log.
| if ?context.wrapWithDirenv | ||
| then Process.callProcess "direnv" (["exec", ".", commandStr] <> args) | ||
| else Process.callProcess commandStr args |
There was a problem hiding this comment.
callProcessDirenvAware duplicates the direnv-wrapping logic already implemented in procDirenvAware (in IHP.IDE.Types). Keeping two implementations increases the chance they diverge (e.g. flags/arguments/order). Consider implementing callProcessDirenvAware in terms of procDirenvAware (or moving a shared helper into IHP.IDE.Types) so all wrapped command execution follows the same code path.
| if ?context.wrapWithDirenv | |
| then Process.callProcess "direnv" (["exec", ".", commandStr] <> args) | |
| else Process.callProcess commandStr args | |
| baseProcess <- procDirenvAware commandStr args | |
| (_, _, _, processHandle) <- Process.createProcess baseProcess | |
| exitCode <- Process.waitForProcess processHandle | |
| case exitCode of | |
| ExitSuccess -> pure () | |
| ExitFailure c -> Exception.throwString ("Process " <> commandStr <> " exited with " <> show c) |
- Reimplement callProcessDirenvAware using procDirenvAware to avoid duplicating direnv-wrapping logic - Use withSystemTempDirectory instead of fixed build/ path in tests to avoid flaky failures from leftover state - Make psql stub verify -f file targets exist and assert exact IHP_LIB-resolved path for IHPSchema.sql Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Nix build requires explicit dependencies even when transitively available. Fixes CI failure from using System.IO.Temp in tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In Nix builds, Cabal's auto-generated Paths_ihp_ide resolves to the package data directory, not the IHP_LIB-derived path. The psql stub validates the file exists; the assertion now just checks for an absolute path to IHPSchema.sql. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@mpscholten ready for review |
|
bump |


Summary
Tests
Closes #2397