Skip to content

postgresqlPackages.omnigres: init at 0-unstable-2025-05-16#404027

Merged
wolfgangwalther merged 1 commit intoNixOS:masterfrom
schonfinkel:init-omnigres
May 20, 2025
Merged

postgresqlPackages.omnigres: init at 0-unstable-2025-05-16#404027
wolfgangwalther merged 1 commit intoNixOS:masterfrom
schonfinkel:init-omnigres

Conversation

@schonfinkel
Copy link
Member

@schonfinkel schonfinkel commented May 4, 2025 •
edited
Loading

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels May 4, 2025
wolfgangwalther
wolfgangwalther previously requested changes May 4, 2025
View reviewed changes
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Fully aware that this is a draft, but found this PR by chance and wanted to give some feedback anyway.

schonfinkel
schonfinkel commented May 4, 2025
View reviewed changes
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. and removed 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. labels May 4, 2025
yrashk
yrashk reviewed May 4, 2025
View reviewed changes
yrashk
yrashk reviewed May 4, 2025
View reviewed changes
yrashk
yrashk reviewed May 4, 2025
View reviewed changes
yrashk
yrashk reviewed May 5, 2025
View reviewed changes
@github-actions github-actions bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels May 5, 2025
schonfinkel
schonfinkel commented May 6, 2025
View reviewed changes
schonfinkel
schonfinkel commented May 6, 2025
View reviewed changes
wolfgangwalther
wolfgangwalther reviewed May 8, 2025
View reviewed changes
@yrashk
Copy link
Contributor

yrashk commented May 8, 2025

@wolfgangwalther

To me, this looks like the upstream build system needs changes - or we need to know about different options that it has, that we're not using, yet. TLDR: We need to be able to install into a different directory than we consume the existing dependency extensions from.

I can work on this upstream.

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 9, 2025

@wolfgangwalther

To me, this looks like the upstream build system needs changes - or we need to know about different options that it has, that we're not using, yet. TLDR: We need to be able to install into a different directory than we consume the existing dependency extensions from.

I can work on this upstream.

Cool. Support for DESTDIR would probably be the easiest solution that works. That means, you already get some absolute paths from pg_config like /nix/store/xxxx-postgresql (multiple different store paths for the different things). And instead of trying to install into that location, you just prepend DESTDIR to the absolute path. For us, DESTDIR will be another /nix/store/yyyy-omnigres path. The end result will be a path that you're installing to like /nix/store/yyyy-omnigres/nix/store/xxxx-postgresql etc.

postgresqlBuildExtension then moves the files in the output around to get rid of that second nix/store part.

The alternative would be a proper prefix for installation, that might be different than the paths from where you consume things. But that might be harder to implement and get right.

@yrashk
Copy link
Contributor

yrashk commented May 9, 2025

How about this? omnigres/omnigres@00dccfb

@schonfinkel
Copy link
Member Author

schonfinkel commented May 10, 2025

How about this? omnigres/omnigres@00dccfb

Can you check my latest commit? What is supposed to be set in PostgreSQL_TARGET_PACKAGE_LIBRARY_DIR

@yrashk
Copy link
Contributor

yrashk commented May 10, 2025 via email •
edited by wolfgangwalther
Loading

@schonfinkel schonfinkel self-assigned this May 11, 2025
@schonfinkel schonfinkel marked this pull request as ready for review May 11, 2025 22:55
@schonfinkel
Copy link
Member Author

schonfinkel commented May 11, 2025

@yrashk @wolfgangwalther finally I got something to build without issues.

nix-build -A postgresql17Packages.omnigres

@nix-owners nix-owners bot requested review from Ma27 and thoughtpolice May 11, 2025 22:56
@wolfgangwalther wolfgangwalther changed the title omnigres: init postgresqlPackages.omnigres: init May 14, 2025
@schonfinkel schonfinkel marked this pull request as ready for review May 16, 2025 23:25
@schonfinkel schonfinkel changed the title postgresqlPackages.omnigres: init at 0-unstable-2025-05-09 postgresqlPackages.omnigres: init at 0-unstable-2025-05-16 May 17, 2025
@ebellani
Copy link

ebellani commented May 17, 2025

❤️

@schonfinkel schonfinkel force-pushed the init-omnigres branch 2 times, most recently from 2dfb03d to a357ed2 Compare May 17, 2025 01:20
wolfgangwalther
wolfgangwalther reviewed May 17, 2025
View reviewed changes
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

Nice progress! Now, that it builds - let's reduce the code to the minimum required.

After addressing all the comments, also don't forget to remove then-unused arguments from the argument list.

I removed doxygen entirely, because it doesn't seem to make a different for the output of the package anyway.

Also, we have a lot of cmake warnings, that it can't find pg_regress - maybe we can look into that and try to fix it, enabling us to run all those tests?

@wolfgangwalther wolfgangwalther mentioned this pull request May 17, 2025
Closed
2 tasks
wolfgangwalther
wolfgangwalther reviewed May 17, 2025
View reviewed changes
pkgs/servers/sql/postgresql/ext/omnigres.nix Outdated
Comment on lines 86 to 88
Copy link
Contributor

@wolfgangwalther wolfgangwalther May 17, 2025

Choose a reason for hiding this comment

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

I looked into why pg_config and the two directories are required to be passed as flags here - this is because of a bug in how we create our own pg_config for postgresql.withPackages. Fix for that in #407920.

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 18, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 404027

x86_64-linux

✅ 5 packages built:
  • postgresql13Packages.omnigres
  • postgresql14Packages.omnigres
  • postgresql15Packages.omnigres
  • postgresql16Packages.omnigres
  • postgresqlPackages.omnigres (postgresql17Packages.omnigres)

aarch64-linux

✅ 5 packages built:
  • postgresql13Packages.omnigres
  • postgresql14Packages.omnigres
  • postgresql15Packages.omnigres
  • postgresql16Packages.omnigres
  • postgresqlPackages.omnigres (postgresql17Packages.omnigres)

@wolfgangwalther wolfgangwalther dismissed their stale review May 18, 2025 12:56

the early blocker had been resolved

@schonfinkel schonfinkel mentioned this pull request May 19, 2025
Closed
@schonfinkel schonfinkel force-pushed the init-omnigres branch 2 times, most recently from 0389333 to fc9db12 Compare May 19, 2025 20:37
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels May 19, 2025
@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 20, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 404027

x86_64-linux

✅ 5 packages built:
  • postgresql13Packages.omnigres
  • postgresql14Packages.omnigres
  • postgresql15Packages.omnigres
  • postgresql16Packages.omnigres
  • postgresqlPackages.omnigres (postgresql17Packages.omnigres)

aarch64-linux

✅ 5 packages built:
  • postgresql13Packages.omnigres
  • postgresql14Packages.omnigres
  • postgresql15Packages.omnigres
  • postgresql16Packages.omnigres
  • postgresqlPackages.omnigres (postgresql17Packages.omnigres)

x86_64-darwin

✅ 5 packages built:
  • postgresql13Packages.omnigres
  • postgresql14Packages.omnigres
  • postgresql15Packages.omnigres
  • postgresql16Packages.omnigres
  • postgresqlPackages.omnigres (postgresql17Packages.omnigres)

aarch64-darwin

✅ 5 packages built:
  • postgresql13Packages.omnigres
  • postgresql14Packages.omnigres
  • postgresql15Packages.omnigres
  • postgresql16Packages.omnigres
  • postgresqlPackages.omnigres (postgresql17Packages.omnigres)

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 20, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 404027 --package postgresql14Packages.omnigres.tests --package postgresql16Packages.omnigres.tests --package postgresqlPackages.omnigres.tests --package postgresql15Packages.omnigres.tests --package postgresql13Packages.omnigres.tests

x86_64-linux

❌ 1 package failed to build:
  • postgresql13Packages.omnigres.tests.extension
✅ 4 packages built:
  • postgresql14Packages.omnigres.tests.extension
  • postgresql15Packages.omnigres.tests.extension
  • postgresql16Packages.omnigres.tests.extension
  • postgresqlPackages.omnigres.tests.extension

Test failure for PG13 is:

CREATE EXTENSION omni_id;
CREATE EXTENSION
SELECT identity_type('user_id');
2025-05-20 06:10:00.988 UTC [55] ERROR:  syntax error at or near "return" at character 85
2025-05-20 06:10:00.988 UTC [55] QUERY:  create function user_id(value int8) returns user_id  immutable strict parallel safe return value::public.user_id
2025-05-20 06:10:00.988 UTC [55] CONTEXT:  PL/pgSQL function identity_type(name,regtype,text,boolean,bigint,bigint,bigint,bigint,boolean,text,boolean,name,regp>
2025-05-20 06:10:00.988 UTC [55] STATEMENT:  SELECT identity_type('user_id');
psql:/build/.attr-1zahchz786hvby68kl3vmvah1rsw0d0ia4g24b3mi9y848rcbisf:4: ERROR:  syntax error at or near "return"
LINE 1: ...) returns user_id  immutable strict parallel safe return val...
                                                             ^
QUERY:  create function user_id(value int8) returns user_id  immutable strict parallel safe return value::public.user_id
CONTEXT:  PL/pgSQL function identity_type(name,regtype,text,boolean,bigint,bigint,bigint,bigint,boolean,text,boolean,name,regprocedure) line 184 at EXECUTE
stopping postgresql

Seems like the syntax is used is not supported by PG13.

Can you please mark this extension as broken for this version?

@schonfinkel @wolfgangwalther @yrashk
omnigres: init at 0-unstable-2025-05-16
c4381da 
---------

Co-authored-by: Wolfgang Walther <[email protected]>
Co-authored-by: Yurii Rashkovskii <[email protected]>
@schonfinkel
Copy link
Member Author

schonfinkel commented May 20, 2025

@wolfgangwalther done.

@yrashk
Copy link
Contributor

yrashk commented May 20, 2025

We recently removed support for pg13 in Omnigres

@wolfgangwalther
Copy link
Contributor

wolfgangwalther commented May 20, 2025

Nice work!

@wolfgangwalther wolfgangwalther merged commit dcb6e63 into NixOS:master May 20, 2025
19 of 21 checks passed
@schonfinkel schonfinkel deleted the init-omnigres branch May 20, 2025 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yrashk yrashk Awaiting requested review from yrashk

@Ma27 Ma27 Awaiting requested review from Ma27

@thoughtpolice thoughtpolice Awaiting requested review from thoughtpolice

@wolfgangwalther wolfgangwalther Awaiting requested review from wolfgangwalther

Assignees

@schonfinkel schonfinkel

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@schonfinkel @yrashk @wolfgangwalther @ebellani