Skip to content

refactor: deduplicate host resolution logic and observations#4288

Merged
steve-chavez merged 1 commit into
PostgREST:mainfrom
taimoorzaeem:refactor/resolveHost
Sep 1, 2025
Merged

refactor: deduplicate host resolution logic and observations#4288
steve-chavez merged 1 commit into
PostgREST:mainfrom
taimoorzaeem:refactor/resolveHost

Conversation

@taimoorzaeem
Copy link
Copy Markdown
Member

While working on #4269, I noticed some redundant code in Network and Observation modules. This commit deduplicates the code in those modules.

Comment thread src/PostgREST/Observation.hs Outdated
Comment thread src/PostgREST/Observation.hs
Comment thread src/PostgREST/Network.hs Outdated
Comment on lines +10 to +21
resolveAddress :: NS.Socket -> IO Text
resolveAddress sock = do
Copy link
Copy Markdown
Member

@steve-chavez steve-chavez Aug 28, 2025

Choose a reason for hiding this comment

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

Could we add doctests here?

Could even be another PR, to ensure this PR doesn't change the expected outcome.

Copy link
Copy Markdown
Member Author

@taimoorzaeem taimoorzaeem Aug 28, 2025

Choose a reason for hiding this comment

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

Could we add doctests here?

Yes sure.

I am getting this error when I run postgrest-test-doctests locally:

[nix-shell]$ postgrest-test-doctests
Configuration is affected by the following files:
- cabal.project
- cabal.project.freeze
<command line>: libX11.so.6: cannot open shared object file: No such file or directory
doctests: fd:15: hGetLine: end of file

@wolfgangwalther Could this be related to #3865 (comment)?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Weird, that runs for me:

$ postgrest-test-doctests
Downloading the latest package list from hackage.haskell.org
Package list of hackage.haskell.org has been updated.
The index-state is set to 2025-09-01T07:55:10Z.
To revert to previous state run:
    cabal v2-update 'hackage.haskell.org,2025-07-17T02:51:41Z'
Resolving dependencies...
...
Examples: 166  Tried: 166  Errors: 0  Failures: 0

Still, it's very weird that we get "Downloading.." when using Nix, as we're supposed to be in deterministic land.

@wolfgangwalther Can you remind me how is the cabal.project/cabal.project.freeze helping? Can we kill those and prevent the "Downloading..."?


@taimoorzaeem Btw, to enable doctest on the Network you need to add it here:

main :: IO ()
main =
doctest
[ "-XOverloadedStrings"
, "-XNoImplicitPrelude"
, "-XStandaloneDeriving"
, "-XDuplicateRecordFields"
, "-isrc"
, "src/PostgREST/ApiRequest/Preferences.hs"
, "src/PostgREST/ApiRequest/QueryParams.hs"
, "src/PostgREST/Config.hs"
, "src/PostgREST/Error.hs"
, "src/PostgREST/MediaType.hs"
, "src/PostgREST/Plan.hs"
, "src/PostgREST/Query/SqlFragment.hs"
, "src/PostgREST/Response.hs"
, "src/PostgREST/Response/Performance.hs"
]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you remind me how is the cabal.project/cabal.project.freeze helping? Can we kill those and prevent the "Downloading..."?

The freeze file makes out cabal CI at least a little bit predictable. Without it, new dependencies appearing on hackage could break CI, when some packages didn't specify proper bounds.

You should not get this download anymore, it has been moved to a separate command in 25e56f3. Are you doing this on an old checkout of main?

Copy link
Copy Markdown
Member

@wolfgangwalther wolfgangwalther Sep 1, 2025

Choose a reason for hiding this comment

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

doc tests work for me, too, btw. I have a wayland-based window manager, so maybe that's why libX11.so.6 doesn't play a role for me?

(not that I'd understand why it would try to access any window-manager related libs anyway...)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you say that only matters on non-Nix builds?

Define "nix build" and "non nix build".

It certainly does not matter when you do nix-build -A postgrestPackage (or the static build).

It certainly does matter in the above GitHub Actions snippet.

I'm not 100% sure about which effects it might have on building with postgrest-build inside the nix-shell (and all the other similar tools like tests etc.). I think it might have an effect on the local cabal build cache, not invalidating it too early, but not sure.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Define "nix build" and "non nix build".

nix build - the one that uses Haskell packages included on nixpkgs

hasql = lib.dontCheck (lib.doJailbreak prev.hasql_1_6_4_4);
hasql-dynamic-statements = lib.dontCheck prev.hasql-dynamic-statements_0_3_1_5;
hasql-implicits = lib.dontCheck prev.hasql-implicits_0_1_1_3;
hasql-notifications = lib.dontCheck prev.hasql-notifications_0_2_2_2;
hasql-pool = lib.dontCheck prev.hasql-pool_1_0_1;
hasql-transaction = lib.dontCheck prev.hasql-transaction_1_1_0_1;
postgresql-binary = lib.dontCheck (lib.doJailbreak prev.postgresql-binary_0_13_1_3);

non-nix build, the one which uses Hackage or Stackage

Is that correct? This might have a place in a markdown file somewhere.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, as I wrote above we essentially have 3 levels of builds: fully nix in the sandbox (the first case), entirely not nix with hackage/stackage (the second case) and the last case, which is kind of a mix, because while cabal is going to use the nix-provided dependencies, the file might still have an effect on how cabal plans the build and invalidates the cache.

Copy link
Copy Markdown
Member

@steve-chavez steve-chavez Sep 3, 2025

Choose a reason for hiding this comment

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

I'm seeing an error similar to the above, although this is when doing postgrest-build:

64-linux/ghc-9.4.8/postgrest-13.1/t/spec/build/spec/spec-tmp/SpecHelper.dyn_o ) [Source file changed]

<no location info>: error: [-Wmissed-extra-shared-lib, -Werror=missed-extra-shared-lib]
    libHSpostgrest-13.1-inplace.so: cannot open shared object file: No such file or directory
    It's OK if you don't want to use symbols from it directly.
    (the package DLL is loaded by the system linker
     which manages dependencies by itself).
[11 of 51] Compiling Feature.RollbackSpec ( test/spec/Feature/RollbackSpec.hs, dist-newstyle/build/x86_64-linux/ghc-9.4.8/postgrest-13.1/t/spec/build/spec/spec-tmp/Feature/RollbackSpec.o, 

The compiling still finishes though.


Another transient error when compiling:

<no location info>: error:
    dist-newstyle/build/x86_64-linux/ghc-9.4.8/postgrest-13.1/build/PostgREST/Query.o.tmp: renameFile:renamePath:rename: does not exist (No such file or directory)
Error: [Cabal-7125]
Failed to build postgrest-13.1 (which is required by test:spec from postgrest-13.1, exe:postgrest from postgrest-13.1 and others).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These seem similar to #3865 (comment).

Comment thread src/PostgREST/Network.hs Outdated
@taimoorzaeem taimoorzaeem marked this pull request as draft August 29, 2025 06:37
@taimoorzaeem taimoorzaeem marked this pull request as ready for review August 30, 2025 07:12
@taimoorzaeem taimoorzaeem marked this pull request as draft August 30, 2025 09:04
@taimoorzaeem taimoorzaeem marked this pull request as ready for review September 1, 2025 05:08
Replaces the "API server listening on unix socket" with simpler
"API server listening on " observation. This allows refactoring
redundant code.

Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
@steve-chavez steve-chavez merged commit 41b6ebe into PostgREST:main Sep 1, 2025
33 checks passed
@taimoorzaeem taimoorzaeem deleted the refactor/resolveHost branch September 2, 2025 05:17
taimoorzaeem added a commit to taimoorzaeem/postgrest that referenced this pull request Nov 8, 2025
This should reduce setup time for build process.

- cache: introduced in PostgREST#2928, defunct since PostgREST#4084
- clock: introduced in PostgREST#2928, defunct since PostgREST#4084
- heredoc: introduced in PostgREST#714, defunct since PostgREST#4390
- iproute: introduced in PostgREST#3560, defunct since PostgREST#4288

Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
steve-chavez pushed a commit that referenced this pull request Nov 9, 2025
This should reduce setup time for build process.

- cache: introduced in #2928, defunct since #4084
- clock: introduced in #2928, defunct since #4084
- heredoc: introduced in #714, defunct since #4390
- iproute: introduced in #3560, defunct since #4288

Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
postgrest-ci Bot pushed a commit that referenced this pull request Nov 10, 2025
This should reduce setup time for build process.

- cache: introduced in #2928, defunct since #4084
- clock: introduced in #2928, defunct since #4084
- heredoc: introduced in #714, defunct since #4390
- iproute: introduced in #3560, defunct since #4288

Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
(cherry picked from commit 9921743)
wolfgangwalther pushed a commit that referenced this pull request Nov 10, 2025
This should reduce setup time for build process.

- cache: introduced in #2928, defunct since #4084
- clock: introduced in #2928, defunct since #4084
- heredoc: introduced in #714, defunct since #4390
- iproute: introduced in #3560, defunct since #4288

Signed-off-by: Taimoor Zaeem <taimoorzaeem@gmail.com>
(cherry picked from commit 9921743)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants