-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
timescaledb_toolkit: init at 1.14.0 #186276
timescaledb_toolkit: init at 1.14.0 #186276
Conversation
@winterqt @Hoverbear Maybe you want to take a look again. |
fd6d7eb
to
49144a4
Compare
49144a4
to
ea2e52e
Compare
4f9fe6c
to
472689e
Compare
0d3c219
to
8c42ec4
Compare
@marsam can you please take a look at this? Also please squash the review commits to follow the contributing guide. |
Cross compilation still fails:
it somehow uses |
Here are some evil hacks with which cargo-pgx/postgresql run at build time, but it now fails at a different place: Code: typetetris@fa14c5e Failure:
Again I am at a loss. |
c28d7b6
to
b2dc46f
Compare
Partially done. Kept two different commits for cargo-pgx changes and adding timescaledb_toolkit. I would have done two pull requests, but the second one would bring the first tests for the first one, which seems awkward. |
Doing this in one PR is fine. The nixos test should also get its own commit. I think the changes look good from someone that has never worked with postgres plugins. |
b2dc46f
to
c48e061
Compare
Done. I also fixed the comment on |
c48e061
to
79bb303
Compare
79bb303
to
51f9d74
Compare
Updated to 1.13.1 so cargo-pgx 0.5.4 isn't needed any more. This is open now for quite some time. |
Thanks a lot for working on this and sorry for the delay. Unfortunately, I've been awful busy with life and work. I'm hoping to review this PR the next week. |
thank you @typetetris for your effort! I was able to wrap |
51f9d74
to
05852b2
Compare
hi everyone, wondering if there are any blockers here preventing the merge? Thanks. |
05852b2
to
0da6d8b
Compare
Rebased on master. |
0da6d8b
to
9dc7a05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay. LGTM overall, thanks a lot for working on this!
export USER="$(whoami)" | ||
pg_ctl start | ||
createuser -h localhost --superuser --createdb "$USER" || true | ||
pg_ctl stop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the pg_ctl stop to the postCheck
phase? I'm thinking of starting the server in preBuild and shutting it down in postCheck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The server is only started for the createuser
command. Afterwards it is not started again by the build script. I would have to check, whether it is started by cargo pgx
.
|
||
nativeBuildInputs = (args.nativeBuildInputs or [ ]) ++ [ | ||
cargo-pgx | ||
postgresql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think postgresql should be in buildInputs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Maybe timescaledb-toolkit isn't easily cross compiled. cargo pgx
wants to call pg_config
so we need the postgresql derivation having the timescaledb-toolkits build platform as host platform.
But on the other hand, it builds a library, which hopefully isn't linked against anything from that postgresql package, but just uses the header files from that package and declares some symbols to be resolved at runtime, so it should work against postgresql compiled for a different platform, if cross compiled.
So timescaledb-toolkit shouldn't use anything from that postgresql derivation at runtime.
That is just my understanding.
I will test that, if I can. (Don't you need to by a mac to do darwin builds?)
Typos, Whitespace and Stuff. Will fixup the relevants commits later, so this commit vanishes. Co-authored-by: Mario Rodas <marsam@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank a lot for working on this!
If there is no other reviews, I'll merge it this weekend.
buildPgxExtension
to build postgresql extensions withcargo-pgx
buildPgxExtension
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes